-
Notifications
You must be signed in to change notification settings - Fork 4
2024.07.31 1차 회고 이후 코드리뷰 관련하여 확인할 만한 내용 추가 #35
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
base: code-review
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ public class ScriptController { | |
| */ | ||
| @GetMapping("/{userId}") | ||
| public ResponseEntity<List<ScriptEntity>> getScriptAll(@PathVariable Long userId){ | ||
|
|
||
| return scriptService.getScriptList(userId); | ||
| } | ||
|
|
||
|
|
@@ -31,6 +32,7 @@ public ResponseEntity<List<ScriptEntity>> getScriptAll(@PathVariable Long userId | |
| */ | ||
| @PostMapping("") | ||
| public ResponseEntity<ScriptEntity> addScript(@RequestBody ScriptEntity scriptEntity) { | ||
|
|
||
| return scriptService.addScript(scriptEntity); | ||
| } | ||
|
|
||
|
|
@@ -42,6 +44,7 @@ public ResponseEntity<ScriptEntity> addScript(@RequestBody ScriptEntity scriptEn | |
| @PutMapping("") | ||
| public ResponseEntity<ScriptEntity> updateScript( | ||
| @RequestBody ScriptEntity scriptEntity) { | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Entity를 Request 와 Response 로 쓰고 있는 부분에 대해서 각각의 Request, Response 파일로 변경도 해보시고, inner Class 를 사용하여 하는 방식으로 도 고민해봇기면 좋을것 같습니다 ! |
||
| return scriptService.updateScript(scriptEntity); | ||
| } // 업데이트 시 내용 날아갈 수 있어 수정 필요 | ||
|
|
||
|
|
@@ -53,6 +56,7 @@ public ResponseEntity<ScriptEntity> updateScript( | |
| @DeleteMapping("/{scriptId}") | ||
| public ResponseEntity<ScriptEntity> deleteScript(@PathVariable Long scriptId) { | ||
|
|
||
|
|
||
| scriptService.deleteScriptById(scriptId); | ||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import lombok.*; | ||
| import java.sql.Timestamp; | ||
|
|
||
|
|
||
| @ToString | ||
| @Builder | ||
| @Getter | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @builder를 사용하였을때의 단점에 대해서 한번 고민해보시면 좋을듯합니다.
@Setter - setter는 지양 |
||
|
|
@@ -32,4 +33,5 @@ public class ScriptEntity { | |
| @JoinColumn(name = "user_id", nullable = false) | ||
| @JsonBackReference | ||
| private UserEntity user; | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,14 +20,17 @@ public class ScriptService { | |
| private final UserRepository userRepository; | ||
|
|
||
| public ResponseEntity<List<ScriptEntity>> getScriptList(Long userId) { | ||
|
|
||
| List<ScriptEntity> scripts = scriptRepository.findByUser_UserId(userId); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 메서드명은 카멜케이스로 유지해주시는것이 어떨까요 ?! |
||
|
|
||
| if (scripts.isEmpty()) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. scripts 의 반환값이 없을 경우 SpringDataJpa에서 빈리스트를 반환합니다. |
||
| return new ResponseEntity<>(HttpStatus.OK); | ||
| } | ||
| return new ResponseEntity<>(scripts, HttpStatus.OK); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service 에서 반환하는 값을 new ResponseEntity로 묶어서 내릴 필요가 있는지에 대해서 고민해보면 좋을것 같습니다 ! |
||
| } | ||
|
|
||
| public ResponseEntity<ScriptEntity> addScript(ScriptEntity scriptEntity) { | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 메서드 파라미터및 응답값에 대해서는 Controller 쪽에 명시하여 따로 적지 않겠습니다! 현재 Builder를 통해 생성하고 있는데 해당 부분에 대해서 Mapper 클래스라던지 정적펙토리 메서드를 사용하여 변경 해보시면 어떨까요 ?!
|
||
| ScriptEntity script = ScriptEntity.builder() | ||
| .content(scriptEntity.getContent()) | ||
| .createdAt(Timestamp.valueOf(LocalDateTime.now())) | ||
|
|
@@ -40,15 +43,18 @@ public ResponseEntity<ScriptEntity> addScript(ScriptEntity scriptEntity) { | |
| } | ||
|
|
||
| public ResponseEntity<ScriptEntity> updateScript(ScriptEntity scriptEntity) { | ||
|
|
||
| if (scriptEntity.getUser().getUserId() != null) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Request 로 받은 scriptEntity에는 항상 User가 있을까요 ?! scriptId기준으로 script을 찾은다음에 코드가 처리되어야 하는 방향이 아닐까 한번 더 확인해주시면 좋을듯합니다. |
||
| UserEntity userEntity = userRepository.findById(scriptEntity.getUser().getUserId()) | ||
| .orElseThrow(() -> new RuntimeException("user not found")); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| scriptEntity.setUser(userEntity); | ||
| } | ||
|
|
||
|
|
||
| ScriptEntity script = scriptRepository.findById(scriptEntity.getScriptId()) | ||
| .orElseThrow(() -> new RuntimeException("script not found")); | ||
|
|
||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Entity에 Setter에 대한 사용은 지양 합니다. 무분별하게 @Setter를 남용하게 된다면 추후 운영 시 어떠한 값으로 업데이트되는지, 어디서 어떻게 사용하는지 파악하는데 어려움이 있을 수 있습니다 |
||
| script.setContent(scriptEntity.getContent()); | ||
| script.setModifiedAt(Timestamp.valueOf(LocalDateTime.now())); | ||
|
|
||
|
|
@@ -58,6 +64,7 @@ public ResponseEntity<ScriptEntity> updateScript(ScriptEntity scriptEntity) { | |
| } | ||
|
|
||
| public void deleteScriptById(Long scriptId) { | ||
|
|
||
| scriptRepository.deleteById(scriptId); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 삭제시 실제로 DB에서 Row를 삭제하게 되는데 삭제가 아닌 Flag를 통하여 관리하는 방법에 대해서도 학습해보시면 좋을것 같습니다 |
||
| } | ||
| } | ||
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.
Entity를 직접적으로 반환하는 부분에 대해서 Request, Response class 로 변경하는것에 대해서 고민해보시면 좋을것 같습니다