[BE]team-11 첫번째 PR입니다.

[choitree]

안녕하세요 team-11의 로빈, 트리입니다.
PR을 이전에 보냈어야했는데, API 개발에 집중하다보니 이렇게 되었습니다...
리뷰어님 죄송합니다😨

이번 프로젝트를 진행하면서 문제가 되었던 부분에 대해서 질의드립니다.

1. 저희가 manyToMany를 oneToMany, manyToOne로 풀어서 작성하고
   조인 테이블을 entity로 분리하는 방식으로 구현을 하였습니다.
   그런데, oneToMany에서 mappedBy로 연결된 데이터가 null로 계속 조회되어서
   결국 manyToOne에 있는 데이터를 활용하는 방식으로 구현할 수 밖에 없었습니다.
   김영한님 강의를 보거나, 블로그등을 찾아보아도 mappedBy로 구현하면 읽기전용으로 데이터가 보이는 것으로 파악되는데
   저희 프로젝트에서 보이지 않는 이유를 알 수 없어서 이 부분에 대해서 문의드립니다.

2. 조인테이블 클래스의 네이밍에 대해서 여쭤봅니다.
   Assignees의 경우, 이슈에 권한이 있는 유저로 한 단어로 축약 가능한데
   IssueHasLabel 같은 경우는 딱히 이 뜻을 포괄하는 단어가 생각나지 않아서 mysql로 ERD 설계 시 자동완성된 네이밍을 사용했습니다.
   두 클래스의 이름에 통일성이 없는데 이러한 경우, 통일성을 지키는 것이 나은건지 아니면 축약 가능한 부분이라도 축약해서 사용하는 것이 맞는지
   혹은 IssueHasLabel도 Assignees처럼 포괄되는 네이밍을 짓는 것이 맞는지 확인 부탁드립니다.

3. 저희가 entity에서는 Builder의 메소드명을 createXxx()로 명명하여 사용했습니다.
   또한 controller에서 이슈/마일스톤/라벨 등을 생성할 때도 createXxx()라는 네이밍을 사용했습니다.
   두 클래스들의 경우, 분리가 되어있지만 네이밍이 유사한 상황인데 이런 식으로 작성해도 되는걸까요?

4. 빌드될 때 계속 아래 사진과 같이 warning 표시가 확인됩니다.
   조인테이블에서 나오는 문제인 것 같은데 어떤것이 잘못되어서 경고가 나오는지 아무리봐도 이해가 안가는데 힌트 주실 수 있으실까요?
   

[참고사항]
이번 프로젝트에서는 롬복을 사용하였습니다.
롬복에서 생성자와 ToString 어노테이션 사용 시 순환참조가 일어나서 오류가 발생하는 부분은
ToString을 지워서 사용한 점 참고해주세요.

PR단위가 너무 방대한 점 다시 한 번 죄송하구요ㅠ_ㅠ
잘 부탁드립니다!!🥰

🙇‍♀️🙇‍♂️🙇

[ehdrhelr]

안녕하세요 리뷰어… 아니 지나가던 시온입니다. 🥲

저도 질문4번과 같은 고민을 한 적이 있어서 학습한 내용을 공유하고자 답글 답니다.

@Builder는 초기화 표현을 무시한다고 하네요.
그래서 private List<IssuehasLabel> issueHasLabels = new ArrayList<>(); 같이 명시적으로 초기화한 개발자의 의도가 @Builder를 사용하면 어긋날 수 있어서 warning 표시를 해주는 것 같습니다.

초기화 하고 싶다면 final을 붙여주거나 @Builder.Default를 붙여주면 되는데요, @Builder는 final 필드에 대한 메서드는 만들어주지 않기 때문에 warning 표시도 발생하지 않습니다. 해당 필드 값은 final로 완전히 고정되겠지요.

@Builder.Default를 붙였을 때 빌드시 따로 값을 넣어 주지 않으면 초기화한 값이 들어갈 것이고 추후 다른 값을 넣어줄 수도 있습니다.

// 예시 1

private final String title = “안녕하세요.”;

Label label = Label.builder()
             // .title() -> 이 메서드가 없음.
                .description(“설명”)
                ...
                .build();

// label의 title은 “안녕하세요”로 고정됨.



// 예시 2

@Builder.Default
private String title = “안녕하세요”;
 
Label label = Label.builder()
                .description(“설명”)
                ...
                .build();
 
// label의 title은 “안녕하세요”

Label label = Label.builder()
                .title(“Hello”)
                .description(“설명”)
                ...
                .build();

// label의 title은 “Hello”

모쪼록 저의 답변이 도움이 되길 바라며 이만 물러가겠습니다..!

[choitree]

우와 시온짱입니다👍👍자세한 설명까지 덧붙여주셔서 이해하기 좋네요~~ 알려주신 방법으로 한번 적용해보도록 해야겠어요😊😊

[LarryJung]

안녕하세요~ 코드 잘 확인했구요, 크게 엔티티, 서비스, 컨트롤러, 에러처리 까지 짧게짧게 역할대로 잘 하신 것 같습니다ㅎㅎ

질문에 답을 드려보면요

1. OneToMany 필드값이 null 인 이유?
   -> IssueService 를 보면 oneToMany 필드인 assignees 와 issueHasLabel 을 업데이트 하는 로직에서 Issue 객체에 값들 할당하는게 빠져있는 것으로 보이는데요, 양방향 관계라면 양쪽 다 넣어줘야 합니다. JPA를 보지 마시고 객체지향 프로그래밍을 한다고 생각해보시면 양쪽 다 넣어주는게 상식적이겠죠?
   아마도 원인이지 않을까 싶은데 함 확인해보시구요!

2. 네이밍은.. 의미 있는 이름이 있다면 쓰시면 되지만 딱히 그런게 아니라면 그냥 컨벤션대로 하셔도 될듯합니다. (저도 아직 주관이 뚜렷하지 않네요 ㅋㅋ)

3. 저는 괜찮다고 생각하지만 객체를 생성하는 static 이름 컨벤션들이 있으니 (of, from 등) 찾아서 적용하셔도 될듯합니다!

4. 이 이슈는 시온이 댓글에 남겨주신 이유가 맞구요, 초기화 필드를 아예 넣지 않던지 넣고 Default를 선언하시던지 하면 됩니다~ (코틀린을 쓰면 롬복 빌더 쓸일도 없습니다만..)

이제 프로젝트 종료가 다가와서 먼저 승인 드리구요! 나머지 댓글도 꼭 확인해주시고, 다른 코드에서도 해당되는 코멘트들이니 전체적으로 피드백 적용 요청드립니다! 👍

[LarryJung]

Bearer 를 상수로 선언한 다음 substring 로직을 replace로 바꿔서 사용하면 어떨까요?

[LarryJung]

* 말고 정확하게 지정해 주는게 좋습니다~ 개발할땐 괜찮지만 과제 제출할 일이 있을때는 이런부분도 세밀하게 챙겨보세요!

[LarryJung]

롬복 사용하시는김에 slf4j 어노테이션도 적용해보세요~

[LarryJung]

이런 정도의 메세지는 디버그로도 충분합니다

[LarryJung]

equals hashCode 도 롬복으로 대체가 가능합니다.

[LarryJung]

이슈와 유저(작성자) 관계가 일대일이 맞을까요?

[LarryJung]

status를 enum으로 두면 장점이 많습니다. 타입체크도 되고.. 혹시나 이슈의 상태가 open closed 가 아닌 '보류' 등의 상태가 추가되었을때도 enum값의 추가만으로 대응이 가능하기도 하고요~ 컬럼에 boolean 은 지양해주시는게 확장성에 좋다고 생각이 듭니다!

[LarryJung]

이렇게 다 쓰지 말고 커스텀 Exception 을 하나 만들어서 상속관계를 맺어주면 ExceptionHandler의 인자로는 부모타입만 명시해도 되고, exception 이 새로 추가되어도 이쪽 핸들러의 수정없이 반영이 가능하지 않을까요~?

[LarryJung]

이부분 아마 isClientCode 와 같은 메서드가 있을텐데 함 찾아보세요~

[LarryJung]

트랜잭션 어노테이션이 빠져있네요!