Skip to content
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

정유빈 API 구축 코드 #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xxubin04
Copy link

@xxubin04 xxubin04 commented Jun 4, 2024

  1. PostFetchController: post 정보 조회
  2. PostModifyController: post에 title과 content 정보 추가 및 업데이트
  3. PostDeleteController: post 정보 삭제

Copy link
Contributor

@coke98 coke98 left a comment

Choose a reason for hiding this comment

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

LGTM 고생하셨습니다~! 앞으로 데이터 베이스도 연동하고, 서비스 코드도 작성하면 어떤 모습일지 기대되네요👍

}

public void setTitle(String title) {
this.title = title;
Copy link
Contributor

Choose a reason for hiding this comment

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

¿이부분들여쓰기가수상하네요?

private Long id;

public GetPostRequest() {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

GetPostRequest라는 네이밍보다 더 명확한 네이밍도 고민해봐도 좋을 것 같아요~

  1. 동사형으로 시작하기에 함수 네이밍(행위 중심)으로 보여짐
  2. 포스트를 가져오는 요청이라는 점에서 GET 요청에만 쓰여야할 것으로 보임(정작 get메서드에서는 안쓰이고 있음)

하지만 이미 여러 메서드와 컨트롤러에서 중복되어 사용되고 있기에(물론 요청마다 분리하는게 제일 나을 것 같아요)
나은 네이밍을 고민한다면
게시글 관련 리퀘스트 => PostRequest
속성을 본다면 게시글 자체를 나타내므로 => Post
와 같은 네이밍들도 고려해볼 수 있겠네융

public class PostDeleteController {

@DeleteMapping("/{postId}")
@ResponseStatus(HttpStatus.OK)
Copy link
Contributor

Choose a reason for hiding this comment

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

요청이 정상적으로 수행됐다면 해당 어노테이션이 없어도 200 상태코드는 반환되지 않나요? 사용하신 이유가 있었는지 궁금해요

@GetMapping
@ResponseStatus(HttpStatus.OK)
public String getPosts(@RequestParam(name = "page", required = false, defaultValue = "1") Long page) {
return "posts";
Copy link
Contributor

Choose a reason for hiding this comment

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

페이징 요청을 RequestParam으로 받아보려는 시도는 좋았어요. 해당 요청값을 활용해서 응답을 반환해줘도 좋았을 것 같네요 🙂

@GetMapping("/{postId}")
@ResponseStatus(HttpStatus.OK)
public String getPost(@PathVariable long postId) {
return "certain post";
Copy link
Contributor

Choose a reason for hiding this comment

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

s로 자원 명시를 다르게하여 구분한 점 좋았어요👍

public class PostModifyController {

@PostMapping
@ResponseStatus(HttpStatus.CREATED)
Copy link
Contributor

Choose a reason for hiding this comment

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

201로 상태 정보를 알리려는 시도도 좋았네요~!
상태코드를 사용하거나 응답을 활용하거나 여러 방법이 있는데 API 사용자에게 상태를 알려주기 위해 고민한 점이 좋았습니다~!

다만, 추후일이지만 모든 상태를 자세히 알려줄 경우 보안적으로도 취약점중 하나가 될 수도 있다는 점도 고민해봐도 좋을 것 같아요

@coke98
Copy link
Contributor

coke98 commented Jun 23, 2024

실제 개발에서는 post와 관련된 도메인은 컨트롤러 하나로 합쳐도 좋을 것 같습니다. 게시글이 아닌 추가적인 도메인으로 컨트롤러를 하나 더 만들었다면 더 좋았을 것 같네요 :)

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