사다리-함수형 프로그래밍 #22
사다리-함수형 프로그래밍 #22
Conversation
|
왜 �Layer 아키텍쳐를 사용하셨는지 이유를 알 수 있을까요? |
There was a problem hiding this comment.
리뷰를 놓쳤네요... 정말 죄송합니다
테스트를 위해서 추상화를 하는 것은 상황에 따라서 정말 필요하다는 것에는 너무 동의하고, 저도 그렇게 많이 쓰고 있습니다
이제 이 부분은 아시는 것 같다보니 어떤 경우에 테스트가 필요한 것인지 고민을 해보고, 추상화를 한다면 직접 테스트를 만들어보시는 것도 좋을 것 같아요
무조건 service -> interface, repository -> interface 로 두게 되다보면 보는 입장에서 코드를 읽기가 힘들 것 같아요
비슷한 방식으로 hexagonal architecture 를 저희 회사에서는 쓰지 말자라는 내부적인 컨벤션이 생길 정도니까요
이런 부분을 시도해보시면 좋을 것 같아요
- interface 를 어떤 순간에 적는 것이 좋을지에 대한 고민 후 interface 를 사용한다면 직접 테스트를 만들어보기
- 문자열 출력 부분은 view 쪽으로 전부 넘겨보기 (순수 데이터만 java service 로 넘기기)
너무 늦게 봐서 정말 죄송합니다
일단 approve 를 드릴테니, 추가적으로 궁금하거나 한 것이 있다면, 미션이 끝나고도 괜찮으니 언제든 dm 주시고 막 요청해주세요
| } | ||
|
|
||
| private void printLadderResult(final Users users, final Results results) { | ||
| while (true) { |
There was a problem hiding this comment.
while(true) 로 되어있는 구문은 break 문을 무조건 바로 보이도록 적어주시면 좋을 것 같아요
그리고 while(true) 의 경우에는 break 가 있더라도 무한 루프가 될 수 있다는 것때문에 최대한 지양하는 것이 좋을 것 같아요
There was a problem hiding this comment.
따로 종료 조건을 명시한 부분을 찾을 수 없어서 break와 같은 반복 종료문을 설정하지 않았습니다..!! 또한 횟수가 정해지지 않아서 while(true)를 사용했었는데 혹시 추천하시는 방법이 있으신가요?!
There was a problem hiding this comment.
이 부분의 경우에는 최대 반복 횟수가 사다리 최대 길이로 볼 수 있을 것 같아요!
직접 정의한 숫자를 최대 길이로 잡아도 좋을 것 같아요!
| } | ||
| } | ||
|
|
||
| private boolean checkUserName(final String wantedUserName) { |
There was a problem hiding this comment.
1줄정도의 코드의 경우에는 함수로 분리하지 않는 것이 좋을 것 같아요!
물론 indent 룰을 적용해보시는 것은 아주 좋다고 생각합니다
1줄이 함수가 되려면 맥락이 코드로 나타낼 수가 없는 경우가 많은데요 함수의 이름을 명확하게 짓거나 주석을 통해서 자세한 설명을 왕창 달아주어야 하는 경우에만 이렇게 쓰고 있어요
There was a problem hiding this comment.
아하...!! 안 그래도 최대한 기능을 쪼갠다는 생각때문에 함수를 많이 분리하는 과정에서 나중에 다른 팀원이 봤을 땐 "이 함수는 또 뭐지"라는 상황이 올 수도 있을 것 같다고 생각했던 참이었습니다! 감사합니다 : )
| private static final String SPLIT_CHARACTERS = ","; | ||
|
|
||
| public static List<String> splitMessage(final String message) { | ||
| return new ArrayList<String>(List.of(message.split(SPLIT_CHARACTERS))); |
There was a problem hiding this comment.
Util 의 성격의 함수는 보통 언제나 재활용이 되는 코드인 경우가 대부분일텐데요
만약 제가 코드를 작성한다면 이런 형태가 될 것 같아요
splitMessage(final String message, final Sring tokenizer){}
으로 쪼개는 구분자를 전부 받거나
"," 기준으로 쪼개는 것들이 많아보인다면
splitMessageWithComma(String message) 이런 형태로 , 로 쪼갠다는 것을 명확하게 적어볼 것 같아요
다른 사람이 재활용하기에 뭘로 쪼개는지 모르다보니 직접 찾아오기 귀찮을 것 같아서요
There was a problem hiding this comment.
아하 조금 더 메소드 명으로 구분을 주면 더 명확해 진다는 말씀이시군요..!! 확실히 맞는 말이라고 생각합니다! 이 문제에서는 ","라는 명확한 구분자가 있어서 따로 받지는 않았지만, 이후 여러 구분자로 구별을 해야하는 상황이 있다면 말씀해주신 것처럼 구분자를 받아오는 형식으로 하는 것이 좋을 것 같습니다! 감사합니다 : )
|
|
||
| import java.util.List; | ||
|
|
||
| public interface UserService { |
There was a problem hiding this comment.
혹시 이 인터페이스가 있는 이유가 어떤 것일까요?
하나의 구현체만 있는 경우에는 인터페이스를 사용하지 않는 것이 더 좋을 것 같아요
There was a problem hiding this comment.
말씀하신 것이 맞는 것 같습니다!! 다르게 구현할 부분이 없는 경우는 인터페이스가 없는 것이 더 좋은 것 같습니다 감사합니다: )
|
|
||
| import java.util.List; | ||
|
|
||
| public record Users( |
There was a problem hiding this comment.
약간 개인 취향에 가까운데요
가이드라인에서는 분명히 일급 컬렉션을 되게 중요시 여기는 것을 알고 있어서 그 부분을 적용해주시는 것은 아주 좋다고 생각합니다!
해봐야 어떤 부분이 좋은지 명확하게 알다보니까요
단순하게 .users() 형태로 꺼내오기만 하게 되면 이 클래스의 존재 의미가 없을 것 같아요
만약 이 클래스를 쓰게 된다면 조금 더 메소드를 넣어서 이 클래스의 기능을 추가해보는 것이 좋을 것 같아요
There was a problem hiding this comment.
맞는 것 같습니다! 여기서는 "User의 모음"이라는 부분을 강조하기 위해 Users를 만들었는데, 이후 유저에 관한 작업이 명시되지 않는 다면 리팩토링하면 좋을 것 같습니다 !! 감사합니다 : )
| import domain.ladder.LadderLineConnection; | ||
| import repository.ladder.LadderRepository; | ||
|
|
||
| import java.util.*; |
There was a problem hiding this comment.
자바 컨벤션에서 wild card import 를 지양하라는 문구가 있을텐데요
https://reiphiel.tistory.com/entry/Intellij-java-prevent-wildcard-import
이쪽을 참고해서 적용해주시면 좋을 것 같아요
There was a problem hiding this comment.
자동 import여서 차마 이 부분까지는 신경쓰지 못했던 것 같습니다..!! 감사합니다 : )
| } | ||
| } | ||
|
|
||
| private String makeLine(final List<Boolean> connections) { |
There was a problem hiding this comment.
이 부분은 view 쪽으로 가면 좋을 것 같아요!
개인적으로는 이런 방향이 가장 좋을 것 같아요
graph TD;
view --순수_데이터만_전달-->controller--로직_처리_위임-->service
service--로직_결과_반환-->controller--순수_데이터_전달-->view
출력의 결과같은 부분은 서비스에 있는 것보다는 이 관점에서는 view 쪽이 더 적절할 것 같고요
There was a problem hiding this comment.
아하..!! 말씀하신 것처럼 출력과 관련된 로직은 view에 위임하는 것이 더 역할분리가 잘 된 것처럼 보이는 것 같습니다!! 감사합니다 : )
| private List<Boolean> makeConnections(final int width) { | ||
| List<Boolean> connections = new ArrayList<Boolean>(); | ||
| Random random = new Random(); | ||
|
|
||
| while (connections.size() < width - 1) { | ||
| addConnections(connections, random.nextBoolean()); | ||
| } | ||
|
|
||
| return connections; | ||
| } |
There was a problem hiding this comment.
힘드실 것 같긴 하지만 이 부분에 대한 테스트가 있으면 좋을 것 같아요!
대부분 가장 많이 실수하는 경우가 처음 생성 로직이 예상과는 다르게 진행되는 것인데요
이 부분을 테스트를 통해서 보완하는 것이 가장 경험적으로 실수가 적었던 것 같아요
There was a problem hiding this comment.
랜덤에 대한 테스트는 필요하다고 생각했었습니다..!! 한 번 테스트 해보도록 하겠습니다 감사합니다 : )
|
|
||
| @Override | ||
| public void makeResults(final List<String> results) { | ||
| resultsRepository.setResults(new Results(results)); |
There was a problem hiding this comment.
이 메소드가 약간 직관적이지 않은 것 같아요
makeResult 는 반환 타입이 Results 가 되어야 할 것 같아요!
There was a problem hiding this comment.
맞는 것 같습니다!! save-로 메소드명을 바꾸는 등 로직에 맞게 수정할 필요가 있어보입니다..!! 감사합니다 : )
| public static LadderLineConnection of(final boolean isConnected) { | ||
| return Arrays.stream(values()) | ||
| .filter(ladderLineConnection -> ladderLineConnection.isConnected == isConnected) | ||
| .findAny() | ||
| .orElseThrow(); | ||
| } | ||
| } |
There was a problem hiding this comment.
물론 이렇게 stream 을 쓰는 것도 학습의 목적에서는 좋습니다!
하지만 그냥 이렇게 해도 보는 입장에서 편할 것 같아요
if(isConnected){
return CONNECT_LADDER;
}
return NOT_CONNECT_LADDER;
There was a problem hiding this comment.
여기서는 true, false 두 가지 경우만 있다는 사실을 간과했던 것 같습니다! 감사합니다 : )
안녕하세요 리뷰어님들!! 늦은 PR작성 정말 죄송합니다... 잘 부탁드리구, 많이 배워가겠습니다!! : )
고민사항
Record vs Class
각 클래스에 인스턴스 변수를 2개이하로 줄이기 위해 도메인을 분리하다 보니 final로 값을 변경할 수 없는 도메인이 많이 만들어 졌습니다. 그렇기 때문에 좀 더 깔끔한 코드를 위해서 상태가 변하는 position변수를 가진 User 클래스를 제외한 나머지 도메인에 Record를 사용해보았습니다.
이후 Repository에 DB에 Entity로 저장이 될 때는 Record를 사용하는 것이 적합하지 않다고 생각했습니다. (기본 생성자의 부재, getter&setter의 속성 부재 등) 하지만 지금 미션에서는 비즈니스로직에 사용되는 도메인이라는 것에 초점을 맞추어 이를 사용해보기로 결정했습니다.
https://thorben-janssen.com/java-records-hibernate-jpa/
https://docs.jboss.org/hibernate/stable/search/reference/en-US/html_single/#configuration-bean-frameworks-mapper-orm
Service 추상화
필요사항
각 상황에 맞는 예외처리가 필요해 보입니다. (입력 값 등)
비즈니스 로직을 테스트 하는 것이 필요합니다.