-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Step5 자동차 경주 (리팩토링) - PR 반영 #6121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현용님 안녕하세요!
5단계 미션 잘 진행 해 주셨네요 :)
앞에서 이미 잘 만들어주셔서 크게 변경 할 부분은 없었던 것 같아요!
코멘트를 좀 남겨 뒀는데, 다음 미션 진행 전에 확인 부탁드려요 ~
고생 많으셨고 남은 미션들도 화이팅하세요! 💯
.map(name -> new Car(name, moveConditionSupplier)) | ||
.collect(Collectors.toList()); | ||
return new Cars(cars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거는 위에서
.collect(Collectors.collectingAndThen(Collectors.toList(), Cars::new));
요런식으로도 만들 수 있어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 collectingAndThen 좋은 멤버 메소드네요. 감사합니다.
return this.filterPosition(this.getMaxPosition()); | ||
} | ||
|
||
private Cars filterPosition(Position position) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 사실 winners() 에선 그냥 호출만 하는거고 이 로직 자체가 원하는 결과를 리턴하는거같은데 굳이 나눌 필요 없지 않을까요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 코드를 읽는 개발자(본인이나 타인)가 클래스명이나 메소드명으로부터 유츄할 수 있게 제가 주로 사용하는 방법입니다.
filterPosition만으로도 코드를 조금 읽다보면 금방 이해할 수 있겠지만,
메소드명으로 우승자를 뽑는다는 "이야기" 자체를 명확하게 전달하기 위해 사용합니다.
return cars.stream() | ||
.max(comparator) | ||
.map(Car::getPosition) | ||
.orElse(new Position(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 선언하면 orElse인 경우에 fallback 값을 위해 getMaxPosition()
이 실행 될 때 마다 Position 객체가 생성됩니다. 물론 orElse 조건이 아닌 경우 그냥 만들기만 하고 넘어가구요. 따라서 메모리 누수가 발생할 수 있는데요, new Position(0)
을 매번 하지 않고 상수로 정의해서 재사용을 하시거나 orElseGet 을 이용해서 Supplier로 넘겨주시면 필요할 때만 생성 할 수 있습니다 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 조언 감사드립니다.
orElse안의 new Position의 경우 lazy evaluation이라고 생각했는데, eager evaluation이였군요!
아직 익숙하지 않아서 생각못했던 부분인데, 꼭 새겨두겠습니다.
추가로 주신 PR을 적용해보았습니다.
덕분에 재밌게 잘 진행하고 있습니다.
늦은 밤에 작성하는거라 신경을 많이 못쓴 부분이 있을 것 같습니다.
그밖에도 혹시 개선할 점이 있다면 조언 부탁드립니다.
좋은 밤 되시기 바랍니다. 감사합니다.