- 프로젝트명 : "가시죠, 백오피스 만들기 프로젝트"
- 프로젝트 소개 : 사용자 Role에 따라 인가할 수 있는 관리자 기능을 만드는 프로젝트입니다.
- 사용 기술: #Java #Spring Boot #JPA #MySQL #Redis
GitHub: https://github.com/k-jaehyun/IForest.git
튜터님께서 제출한 팀프로젝트에 대해 피드백을 보내주셨다.
다른 사람의 코드를 하나하나 확인하며 이해하고 그에 대한 피드백을 준다는게 쉽지 않은 일인데, 이렇게 상세한 피드백을 보내주셔서 감사하다.
피드백 받은 내용 덕분에 알아가는 것과 느낀점 등을 정리해본다.
1. 테스트코드
- 테스트코드가 작성되어있지 않습니다.
- 앞으로는 하나의 기능, 또는 하나의 컴포넌트를 구현하더라도 그 기능이 정상적으로 동작하는지 테스트할 수 있는 테스트코드까지 작성해보시면 좋을 것 같습니다.
- 그리고 더 나아가서는 어떻게하면 테스트하기 쉬운 코드가 될지, 테스트하기 쉬운 설계와 구현을 고민하면서 구현해보시면 좋을 것 같습니다.
테스트코드가 이번 프로젝트에서의 요구사항은 아니라서 테스트코드는 작성하지 않았다.
이전에 테스트코드를 작성해본 경험이 있는데, "테스트하기 쉬운 코드" 라고 말씀해주신 부분을 보니 와닿는게 있었다. 단순히 코드를 논리에 맞게 작성하는 것 뿐만 아니라, 테스트코드를 만들기에도 용이하도록 (TDD라면 순서가 다르겠지만) 코드를 작성하는 것이 좋겠다는 생각이 든다. 테스트코드를 작성하기 쉬우면 그만큼 코드가 명확하다는 것이 아닐까?
다른 사람이 이해하기 쉬운 코드 뿐만이 아니라 테스트코드를 작성하기 쉬운 코드도 염두에 두고 코드를 작성해야겠다.
2. 코드 정리
Timestamped나CommonRequestDto도 어딘가 패키지로 정리해볼 수 있을까요?- 실제로 사용되지 않거나 아무 의미가 없는 코드들이 중간중간 보입니다. 의미없는 코드는 정리해주세요.
followRepository는 실제로 사용되지 않습니다.
의미없는 코드를 여럿 적어주신 것 중에 하나만 가져왔다. 이것은 캐치하지 못했던 부분이다. (나는 왠만하면 import조차 ctrl+alt+O 를 눌러 제거한다.) 하지만 이것이 그대로 제출 됐다는 점에서 마이너스적인 요소가 있는 것은 분명하다. 할당된 메모리가 쓰이지 않았으므로 그만큼 손실과 오류에 대한 위험성이 커지기 때문이다.
앞으로는 메모리 사용도 염두에 둬야겠다.
- 일반적으로 공백을 둘 이상 연속으로 넣지 않습니다.
- 들여쓰기가 부적절합니다(빨간색 부분).
공백이나 들여쓰기 부분도 체크해주셨다. 물론 다른 팀원분이 작성해주신 코드이지만, 코드 리뷰를 하면서 발견하지 못한 내 잘못도 있다. 자바는 파이썬과 달리 들여쓰기에 의해 컴퓨터가 코드를 해석하는게 달라지진 않지만, 사람은 잘못 해석 할 수 있으므로 주의해야하는 부분이다.
나만이 볼 수 있는 코드가 아니라, 다른 사람도 잘 알아볼 수 있는 코드를 작성할 것! 그래서 난 네이밍 할 때 고민을 많이 하게되더라.
들여쓰기 등 단축키는 ctrl+alt+L 로 알고있지만 이렇게 잘못된 부분을 볼 수 있다니!? 이건 어떻게 하는건지 튜터님께 여쭤봐야겠다.
3. 설계
admin라는 패키지를 별도로 나눈 것은 그리 좋아보이지 않습니다.
- 앞으로 기능이 추가됨에 따라
admin패키지는 점점 커질 것이기 때문이고 admin이 하나의 기능상 도메인이라고 보기도 애매하기 때문입니다.- 각각의 도메인이 해당 도메인과 관련된 모든 코드를 포함하도록 하는 것이 더 응집성있는 설계일 것 같습니다.
- Service layer에서 HTTP 응답을 만들어서 반환하고 있고 controller는 service를 부르는 것 외에 아무 일도 하지 않습니다. 그렇다면 service layer와 controller layer는 왜 분리되어야 하는 것일까요?
- 각 레이어의 역할에 대해 더 고민해보시는 것이 좋을 것 같습니다.
'백오피스 프로젝트'이기 때문에 admin이라는 관리자 페이지를 따로 만들고, 그곳에 기능(admin의 수정,삭제 등)을 구현하는 것으로 설계했다. 하지만 튜터님은 하나의 도메인이라고 보기 어렵기 때문에 admin 코드가 해당 도메인에 포함되도록 설계하는 것이 더 응집성 있었을 것으로 피드백 해주셨다.
그렇다면 어떻게 수정 할 수 있을까 생각해보았다.
- 확실히 CRUD 관련해서는 그렇게 만드는 것이 더 응집성이 있었을 것으로 보인다.
- 유저 삭제 및 차단 등의 기능도 관리자로 로그인했다면 User를 확인 했을 때 그 유저를 차단하거나 삭제하는 방향으로도 만들 수 있을 것이다.
- 공지글 또한 게시물 작성시에 '공지글로 전환' 항목을 선택해서 적용 할 수 있었을 것 같다.
응집성 있는 코드! 이것도 염두에 두자.
4. 구현
코드 한 줄이 너무 깁니다.
public List<FollowUserResponseDto> getUsersFollowed(Long userId) { User receiver = userRepository.findById(userId).orElseThrow(()-> new IllegalArgumentException("존재하지 않는 유저 아이디 입니다.")); return followRepository.findAllByReceiverId(receiver.getId()).stream().map(FollowUserResponseDto::new).toList(); }이 코드는 아래와 같이 적절히 줄바꿈함으로써 가독성을 높일 수 있습니다.
public List<FollowUserResponseDto> getUsersFollowed(Long userId) { User receiver = userRepository.findById(userId) .orElseThrow(()-> new IllegalArgumentException("존재하지 않는 유저 아이디 입니다.")); return followRepository.findAllByReceiverId(receiver.getId()) .stream() .map(FollowUserResponseDto::new) .toList(); }
너무나도 맞는 말이다. 코드가 길면 스크롤을 옆으로 옮겨가서 봐야하기 때문에 너도나도 확인하기 어려운게 당연한건데..!!
그저 한줄에 ; 하나~ 라고 생각했던 나 자신을 반성하게 된다.
줄바꿈 또한 염두에 두자.
삭제 -엔터티 매핑
삭제시 연관관계 매핑에서 오류가 발생했다고 말씀하셨습니다.
이는 앞서 발표한 명시적 삭제를 이용하면 조금 쉽게 해결이 가능합니다.
231209 TIL에 작성한 내용을 발표에 넣었는데, 그것에 대한 피드백도 주셨다.
그 내용은 팔로우 엔터티를 추가하면서, User와 연관관계 매핑이 안되어있어 유저 삭제시 오류가 생겼다.
그래서 1.직접적 명시, 2.Usercascade = CascadeType.REMOVE, 3.Follow@OnDelete 셋중에 고민했다. 그 결과 누구든지 알아보기도 쉽고 이미 만들어진 User 테이블에 수정을 가하지 않아도 되는 @Ondelete 을 사용했다.
하지만 튜터님은 실무에서는 데이터 하나하나가 소중해지기 때문에, 혹시라도 잘못 연관되어 삭제되지 않도록 명시적 삭제(직접적 명시)를 이용하길 권장하셨다.
처음 알게 된 내용이었다! 데이터를 소중히 하기위해 실무에서는 삭제되는 상황을 최소화 해야한다는 것으로 받아들였다. DB를 다룰때는 조심, 또 조심해야겠다.
다른 얘기지만, 밸로그도 읽기 쉬운 글을 작성하도록 노력해보자!
처음에 나만 보려고 작성했지만, 그러려면 왜 굳이 포스팅을 해야하겠는가?
내가 아는 것을 나만 보려고 포스팅 하는게 아니라, 우연히라도 누군가 내 글을 보고 얻어가는게 있으면 좋겠다.
관련 포스팅
Previouse Post
