Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class ScriptController {
*/
@GetMapping("/{userId}")
public ResponseEntity<List<ScriptEntity>> getScriptAll(@PathVariable Long userId){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entity를 직접적으로 반환하는 부분에 대해서 Request, Response class 로 변경하는것에 대해서 고민해보시면 좋을것 같습니다


return scriptService.getScriptList(userId);
}

Expand All @@ -31,6 +32,7 @@ public ResponseEntity<List<ScriptEntity>> getScriptAll(@PathVariable Long userId
*/
@PostMapping("")
public ResponseEntity<ScriptEntity> addScript(@RequestBody ScriptEntity scriptEntity) {

return scriptService.addScript(scriptEntity);
}

Expand All @@ -42,6 +44,7 @@ public ResponseEntity<ScriptEntity> addScript(@RequestBody ScriptEntity scriptEn
@PutMapping("")
public ResponseEntity<ScriptEntity> updateScript(
@RequestBody ScriptEntity scriptEntity) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entity를 Request 와 Response 로 쓰고 있는 부분에 대해서 각각의 Request, Response 파일로 변경도 해보시고, inner Class 를 사용하여 하는 방식으로 도 고민해봇기면 좋을것 같습니다 !

return scriptService.updateScript(scriptEntity);
} // 업데이트 시 내용 날아갈 수 있어 수정 필요

Expand All @@ -53,6 +56,7 @@ public ResponseEntity<ScriptEntity> updateScript(
@DeleteMapping("/{scriptId}")
public ResponseEntity<ScriptEntity> deleteScript(@PathVariable Long scriptId) {


scriptService.deleteScriptById(scriptId);
return ResponseEntity.ok().build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,5 @@ public class ScriptEntity {
@JoinColumn(name = "user_id", nullable = false)
@JsonBackReference
private UserEntity user;

}
7 changes: 7 additions & 0 deletions src/main/java/com/speech/up/script/service/ScriptService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드명은 카멜케이스로 유지해주시는것이 어떨까요 ?!


if (scripts.isEmpty()) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scripts 의 반환값이 없을 경우 SpringDataJpa에서 빈리스트를 반환합니다.
해당 내용을 참고하셔서 해당 코드를 변경해보시는것은 어떨까요??
scripts.isEmpty() 가 필요할지에 대해서 한번도 고민해보시면 어떨까요 ?!

return new ResponseEntity<>(HttpStatus.OK);
}
return new ResponseEntity<>(scripts, HttpStatus.OK);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Service 에서 반환하는 값을 new ResponseEntity로 묶어서 내릴 필요가 있는지에 대해서 고민해보면 좋을것 같습니다 !

}

public ResponseEntity<ScriptEntity> addScript(ScriptEntity scriptEntity) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 파라미터및 응답값에 대해서는 Controller 쪽에 명시하여 따로 적지 않겠습니다!

현재 Builder를 통해 생성하고 있는데 해당 부분에 대해서 Mapper 클래스라던지 정적펙토리 메서드를 사용하여 변경 해보시면 어떨까요 ?!

  • 정적 펙터리 메서드 학습
  • Mapper를 정의한다면 어떻게 할지 .

ScriptEntity script = ScriptEntity.builder()
.content(scriptEntity.getContent())
.createdAt(Timestamp.valueOf(LocalDateTime.now()))
Expand All @@ -40,15 +43,18 @@ public ResponseEntity<ScriptEntity> addScript(ScriptEntity scriptEntity) {
}

public ResponseEntity<ScriptEntity> updateScript(ScriptEntity scriptEntity) {

if (scriptEntity.getUser().getUserId() != null) {
Copy link
Author

Choose a reason for hiding this comment

The 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"));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom Exception에 대해서 한번 학습 후 해당 Exception에 대해서 다시한번 정의해보시면 좋을것 같습니다 !

scriptEntity.setUser(userEntity);
}


ScriptEntity script = scriptRepository.findById(scriptEntity.getScriptId())
.orElseThrow(() -> new RuntimeException("script not found"));


Copy link
Author

Choose a reason for hiding this comment

The 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()));

Expand All @@ -58,6 +64,7 @@ public ResponseEntity<ScriptEntity> updateScript(ScriptEntity scriptEntity) {
}

public void deleteScriptById(Long scriptId) {

scriptRepository.deleteById(scriptId);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제시 실제로 DB에서 Row를 삭제하게 되는데 삭제가 아닌 Flag를 통하여 관리하는 방법에 대해서도 학습해보시면 좋을것 같습니다

}
}