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

[TI-22] 회원 상세 정보 조회 기능 구현 #11

Merged
merged 3 commits into from
Oct 29, 2021

Conversation

sangmin7648
Copy link
Collaborator

No description provided.

@GetMapping("/{userId}")
public ApiResponse<UserResponse> getUserById(@PathVariable Long userId) {
return ApiResponse.ok(
"/v1/users/" + userId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"/v1/users/" 부분을 public static final String 으로 관리하는건 어떨까요? 🤹‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어차피 "/v1/users/"는 컨트롤러 클래스에 한번만 사용해서 별도 변수로 관리했을떄의 장점을 잘 모르겠는데요. 어떤 장점이 있나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 "/v1/users/" 라는 String이 RequestMapping과 ApiResponse.ok의 인자, 총 두 군데에 사용되고 있습니다.
이럴 경우 나중에 이 부분을 변경하게 되는 경우에 휴먼 에러가 발생하기 쉽다고 해서 하나의 스트링 상수를 만들어서 사용하라고 앨런멘토님께 리뷰를 받았던 적이 있습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 "/v1/users/" 라는 String이 RequestMapping과 ApiResponse.ok의 인자, 총 두 군데에 사용되고 있습니다.
이럴 경우 나중에 이 부분을 변경하게 되는 경우에 휴먼 에러가 발생하기 쉽다고 해서 하나의 스트링 상수를 만들어서 사용하라고 앨런멘토님께 리뷰를 받았던 적이 있습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정하겠습니다!

// When Then
mockMvc
// When
var result = mockMvc
Copy link
Collaborator

Choose a reason for hiding this comment

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

var 불편합니다...! 코딩컨벤션으로 명확하게 하면 좋을 것 같습니다..!😮

Copy link
Collaborator

Choose a reason for hiding this comment

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

인텔리제이 숏컷으로 명확한 반환타입을 가진 변수를 추출해낼 수 있습니다. (Linux -> Ctrl + Alt + V)
var 보다는 이 기능을 이용하면 좋을 것 같습니다.
저도 개인적으로 var는 많이 불편하게 느껴집니다! 😅

public record UserResponse(
Long id,

@JsonProperty("username")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JsonProperty("username") 이랑 @JsonProperty("userName")랑 차이가 있나요? 😮

Copy link
Collaborator

Choose a reason for hiding this comment

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

응답으로 나갈때 Json 필드에서 키값이 의도 한 대로 달라지는지 한번 확인해보면 좋을 것 같습니다.

Copy link
Collaborator

@HwiNyeonKim HwiNyeonKim left a comment

Choose a reason for hiding this comment

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

확인했습니다! 😃
approve이지만 그래도 한번 확인 부탁드립니다.

public ApiResponse<UserIdResponse> createUser(
@Validated @RequestBody
@Valid @RequestBody
CreateUserRequest request
Copy link
Collaborator

Choose a reason for hiding this comment

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

이부분 3줄이 아니라, 1줄 혹은 2줄로 하면 많이 긴가요?

public record UserResponse(
Long id,

@JsonProperty("username")
Copy link
Collaborator

Choose a reason for hiding this comment

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

응답으로 나갈때 Json 필드에서 키값이 의도 한 대로 달라지는지 한번 확인해보면 좋을 것 같습니다.

// When Then
mockMvc
// When
var result = mockMvc
Copy link
Collaborator

Choose a reason for hiding this comment

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

인텔리제이 숏컷으로 명확한 반환타입을 가진 변수를 추출해낼 수 있습니다. (Linux -> Ctrl + Alt + V)
var 보다는 이 기능을 이용하면 좋을 것 같습니다.
저도 개인적으로 var는 많이 불편하게 느껴집니다! 😅

@HwiNyeonKim
Copy link
Collaborator

  • 일단 작업 진행을 위해서 먼저 머지했습니다.
    • 몸 상태 괜찮으시면 한 번 확인 해 주세요.

@HwiNyeonKim HwiNyeonKim merged commit 585dde3 into develop Oct 29, 2021
@sangmin7648
Copy link
Collaborator Author

언급해주신부분들을 수정해서 바로 푸쉬하도록 하겠습니다

@sangmin7648 sangmin7648 deleted the feature/TI-22 branch October 30, 2021 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1차 스프린트 뀨팀 1차 스프린트
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants