Step 5 - Car racing(Refactoring) #1816
Conversation
There was a problem hiding this comment.
Hey Aparna,
It is really nice to see you again in PR and we're finally at the last step!
I’ve left some feedback on areas that could be improved before wrapping up the mission.
Hope it helps you make the final touches. Let me know if you have any questions! 😊
| @@ -1,7 +1,8 @@ | |||
| package carracing | |||
|
|
|||
| import carracing.RandomNumberGenerator.getRandom | |||
| import carracing.view.ResultView | |||
|
|
|||
| typealias RaceHistory = List<List<Car>> | |||
There was a problem hiding this comment.
Continued with #1813 (comment)
So if I understand correct, the RaceHistory will be List<List>, right?
Yes, That's right!
Just a thought: what would change if RaceHistory were its own class rather than a typealias?
Would it give you more flexibility in handling race data or expressing domain logic more clearly?
| @@ -12,6 +16,10 @@ class Car(val name: String, var position: Int = INITIAL_POSITION) { | |||
| } | |||
| } | |||
|
|
|||
| fun setAsWinner(maxPosition: Int) { | |||
There was a problem hiding this comment.
Is there a reason why Car needs a setter for isWinner?
What might be the trade-offs of exposing internal state like this versus encapsulating that logic?
| cars.forEach { it.setAsWinner(maxPosition) } | ||
| return cars.filter { it.isWinner } |
There was a problem hiding this comment.
This version with setAsWinner() makes sense in terms of marking the winner.
However do you think we might be adding more state management than necessary here?
What if the Car just told us whether it's at the winning position, rather than us updating it?
return cars.filter { it.isPositionAt(maxPosition) }
Hi Brie,
Thank you for all the valuable comments and guiding me through the challenge.
I have tried to refactor according to your comments in the previous PR.
Looking forward to your inputs for this one as well.