2024.07.31 1차 회고 이후 코드리뷰 관련하여 확인할 만한 내용 추가 #35
Conversation
YunNote
left a comment
YunNote
left a comment
There was a problem hiding this comment.
Script에 대해서만 진행하였습니다
해당 내용을 바탕으로 다른 도메인에도 적용해보시면 좋을듯 합니다 !
| @@ -21,6 +21,7 @@ public class ScriptController { | |||
| */ | |||
| @GetMapping("/{userId}") | |||
| public ResponseEntity<List<ScriptEntity>> getScriptAll(@PathVariable Long userId){ | |||
There was a problem hiding this comment.
Entity를 직접적으로 반환하는 부분에 대해서 Request, Response class 로 변경하는것에 대해서 고민해보시면 좋을것 같습니다
| @PutMapping("") | ||
| public ResponseEntity<ScriptEntity> updateScript( | ||
| @RequestBody ScriptEntity scriptEntity) { | ||
|
|
There was a problem hiding this comment.
Entity를 Request 와 Response 로 쓰고 있는 부분에 대해서 각각의 Request, Response 파일로 변경도 해보시고, inner Class 를 사용하여 하는 방식으로 도 고민해봇기면 좋을것 같습니다 !
|
|
||
| public ResponseEntity<List<ScriptEntity>> getScriptList(Long userId) { | ||
|
|
||
| List<ScriptEntity> scripts = scriptRepository.findByUser_UserId(userId); |
|
|
||
| List<ScriptEntity> scripts = scriptRepository.findByUser_UserId(userId); | ||
|
|
||
| if (scripts.isEmpty()) { |
There was a problem hiding this comment.
scripts 의 반환값이 없을 경우 SpringDataJpa에서 빈리스트를 반환합니다.
해당 내용을 참고하셔서 해당 코드를 변경해보시는것은 어떨까요??
scripts.isEmpty() 가 필요할지에 대해서 한번도 고민해보시면 어떨까요 ?!
| if (scripts.isEmpty()) { | ||
| return new ResponseEntity<>(HttpStatus.OK); | ||
| } | ||
| return new ResponseEntity<>(scripts, HttpStatus.OK); |
There was a problem hiding this comment.
Service 에서 반환하는 값을 new ResponseEntity로 묶어서 내릴 필요가 있는지에 대해서 고민해보면 좋을것 같습니다 !
|
|
||
| public ResponseEntity<ScriptEntity> updateScript(ScriptEntity scriptEntity) { | ||
|
|
||
| if (scriptEntity.getUser().getUserId() != null) { |
There was a problem hiding this comment.
Request 로 받은 scriptEntity에는 항상 User가 있을까요 ?!
scriptId기준으로 script을 찾은다음에 코드가 처리되어야 하는 방향이 아닐까 한번 더 확인해주시면 좋을듯합니다.
|
|
||
| if (scriptEntity.getUser().getUserId() != null) { | ||
| UserEntity userEntity = userRepository.findById(scriptEntity.getUser().getUserId()) | ||
| .orElseThrow(() -> new RuntimeException("user not found")); |
There was a problem hiding this comment.
Custom Exception에 대해서 한번 학습 후 해당 Exception에 대해서 다시한번 정의해보시면 좋을것 같습니다 !
| ScriptEntity script = scriptRepository.findById(scriptEntity.getScriptId()) | ||
| .orElseThrow(() -> new RuntimeException("script not found")); | ||
|
|
||
|
|
There was a problem hiding this comment.
Entity에 Setter에 대한 사용은 지양 합니다.
무분별하게 @Setter를 남용하게 된다면 추후 운영 시 어떠한 값으로 업데이트되는지, 어디서 어떻게 사용하는지 파악하는데 어려움이 있을 수 있습니다
|
|
||
| public void deleteScriptById(Long scriptId) { | ||
|
|
||
| scriptRepository.deleteById(scriptId); |
There was a problem hiding this comment.
삭제시 실제로 DB에서 Row를 삭제하게 되는데 삭제가 아닌 Flag를 통하여 관리하는 방법에 대해서도 학습해보시면 좋을것 같습니다
|
|
||
| @ToString | ||
| @Builder | ||
| @Getter |
There was a problem hiding this comment.
@builder를 사용하였을때의 단점에 대해서 한번 고민해보시면 좋을듯합니다.
- 완벽하지 않은 상태에 대해서 컴파일시 에러가 발생하지 않음. (ex. Field 가 추가되었으나 Builder로 생성된 부분에 작성하는 부분에 놓치게 되더라도 생성됨 )
@Setter - setter는 지양
@NoArgsConstructor 사용시 AccessLevel을 통해 기본생성자를 통해 객체가 생성되지 않도록 수정
1 participant
No description provided.