Skip to content

Homework#1

Open
YoRangS wants to merge 2 commits into
DEVdongbaek:homeworkfrom
YoRangS:homework
Open

Homework#1
YoRangS wants to merge 2 commits into
DEVdongbaek:homeworkfrom
YoRangS:homework

Conversation

@YoRangS
Copy link
Copy Markdown

@YoRangS YoRangS commented Sep 30, 2024

Update, Delete button will be updated

}

// Daily 삭제
@PostMapping("/daily-delete/{dailyId}")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

현재 Daily 삭제간 @PostMapping을 사용해서 Delete를 수행하고 있습니다. 그렇기에 uri에 직접적으로 행위인 "delete"를 적어줄 수 밖에 없습니다. Spring에서 REST API는 행위에 대한 명세는 HTTP method를 통해 표시하는걸 권장합니다. 그렇기에 @DeleteMapping 어노테이션을 사용하여 직접적으로 삭제하는 API임을 Client에게 설명해주는 것이 좋아보입니다.

if (userDetails == null) {
throw new NotExistMemberException(MessageCode.DOES_NOT_EXIST_MEMBER.getMessage());
}
// if (userDetails == null) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

유저 권한 분기 로직을 주석 처리 한 이유가 궁금합니다!

dailyContent.updateDailyContent(requestDTO);
}

@Transactional
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

현재 저희 Daily 프로젝트에서는 Runtime시 발생하는 예외들에 대해 RestExceptionHandler 객체를 이용해 핸들링 하고 있습니다. 이 방식을 통해 RuntimeException을 상속받아 더 명시적인 Custom Exception을 만들 수 있습니다.

}
}

public void deleteDailyContent(DailyDTO.RequestDTO requestDTO) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Entity에 deleteDailyContent 메서드가 필요한 이유가 있을까요?

$.ajax({
url : '/daily-delete/' + dailyId,
data : dataJson,
method : 'POST',
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

위에서 말씀드린 REST API에 대한 부분입니다. 만약 HTTP method 자체가 delete였다면, uri를 /delete/ + {dailyId}로 더 간결하게 만들 수 있었을 것입니다.


// Daily 수정
@PutMapping("/daily/{dailyId}")
@ResponseStatus(HttpStatus.OK)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

이 때 @ResponseStatus 어노테이션을 통해 성공시 200 반환 코드를 보내주고 있습니다. 그러나 PUT 메서드의 API의 경우 201 코드를 반환 해주는 것이 더 바람직 합니다.

.build();
}

public void updateDaily(DailyDTO.RequestDTO dailyDTO) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

DailyDTO.RequestDTO 객체의 isItIsPublic 메서드의 이름의 가독성이 조금 떨어지는 것 같습니다. 간단하게 isPublic()으로 변경해도 좋아보입니다. 아니면 특별히 해당 네이밍을 하신 이유가 있으신지 궁금합니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants