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

2주차 세미나 심화과제 #6

Closed
wants to merge 10 commits into from

Conversation

PgmJun
Copy link
Member

@PgmJun PgmJun commented Apr 21, 2023

[2주차 세미나 과제 이슈 링크]
#2

[API 명세서]
https://pgmjun.notion.site/2-API-e12e8f3dd46c439cbc4bdcda39e5c539

Copy link
Member

@05AM 05AM left a comment

Choose a reason for hiding this comment

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

아싸 1빠

과제 고생하셨습니다!! 👍👍

이것저것 적어봤는데 제가 많이 부족해서 모르는 것이 많습니다.
작성한 코드에 대한 명확한 이유가 있으시다면 무시하셔도 좋습니다.

다음 과제도 기대하고 있겠습니다!

Comment on lines +11 to +29
protected ResponseEntity<?> generateSaveResponseEntity(boolean saveResult) {
return (saveResult) ?
new ResponseEntity<>("정보가 성공적으로 등록 되었습니다.", HttpStatus.OK):
new ResponseEntity<>("등록에 실패하였습니다.", HttpStatus.NO_CONTENT);
}

protected abstract ResponseEntity<?> generateFindResponseEntity(Optional<?> domain, Long id);

protected ResponseEntity<?> generateUpdateResponseEntity(boolean updateResult, Long id) {
return (updateResult) ?
new ResponseEntity<>(id + "번 데이터에 대한 정보가 성공적으로 업데이트 되었습니다.", HttpStatus.OK):
new ResponseEntity<>(id + "번 데이터에 대한 정보가 존재하지 않습니다.", HttpStatus.NO_CONTENT);
}

protected ResponseEntity<?> generateDeleteResponseEntity(boolean deleteResult, Long id) {
return (deleteResult) ?
new ResponseEntity<>(id + "번 데이터에 대한 정보가 성공적으로 삭제 되었습니다.", HttpStatus.OK):
new ResponseEntity<>(id + "번 데이터에 대한 정보가 존재하지 않습니다.", HttpStatus.NO_CONTENT);
}
Copy link
Member

Choose a reason for hiding this comment

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

이런 방식은 처음 봐서 신기하네요!
왜 이렇게 구현하셨는지 궁금합니다.

다른 응답 방식이 생기면 그 때마다 메소드를 만들어야 할 것 같은데 ResponseStatus를 enum 타입으로 만들어서 필드로 id, message, httpStatus를 만드는 것은 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이런 방식은 처음 봐서 신기하네요! 왜 이렇게 구현하셨는지 궁금합니다.

다른 응답 방식이 생기면 그 때마다 메소드를 만들어야 할 것 같은데 ResponseStatus를 enum 타입으로 만들어서 필드로 id, message, httpStatus를 만드는 것은 어떤가요?

Enum타입 사용 괜찮은 것 같습니다!! 3차 세미나 때 배운 내용과 합쳐서 적용해보겠습니다:)

import java.util.Optional;

@RestController
@RequestMapping(value = "/user", produces = "application/json; charset=UTF8")
Copy link
Member

Choose a reason for hiding this comment

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

인코딩에서 문제가 생겼었나요? produces 속성을 사용하신 이유가 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

인코딩에서 문제가 생겼었나요? produces 속성을 사용하신 이유가 궁금합니다!

한글 데이터를 response 해야하는 상황에서 해당 설정이 없으면 한글이 깨지더라구요!
때문에 컨트롤러에 이런 설정을 해두었습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

배워 갑니다 ~ !!!


@GetMapping("/{userId}")
public ResponseEntity<?> findUser(@PathVariable final Long userId) {
Optional<User> findUser = userService.findUserById(userId);
Copy link
Member

Choose a reason for hiding this comment

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

optional로 return 받으시고, null일 때의 조치를 명시하지 않으신 것 같습니다.
.orElse() / .orElseGet() / orElseThrow 등 null일 시 어떤 조치를 취하실 것인지 붙이신다면 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

optional로 return 받으시고, null일 때의 조치를 명시하지 않으신 것 같습니다. .orElse() / .orElseGet() / orElseThrow 등 null일 시 어떤 조치를 취하실 것인지 붙이신다면 좋을 것 같습니다!

해당 부분에 대한 처리는 되어있습니다!

    @GetMapping("/{userId}")
    public ResponseEntity<?> findUser(@PathVariable final Long userId) {
        Optional<User> findUser = userService.findUserById(userId);

        return generateFindResponseEntity(findUser, userId);
    }


    @Override
    public ResponseEntity<?> generateFindResponseEntity(Optional<?> user, final Long userId) {
        return (user.isEmpty()) ?
                new ResponseEntity<>(userId + "번 유저에 대한 정보가 존재하지 않습니다.", HttpStatus.NO_CONTENT) :
                new ResponseEntity<>(((User)user.get()).toDto(userId), HttpStatus.OK);
    }

}

@PutMapping("/update/{userId}")
public ResponseEntity<?> updateUser(@PathVariable final Long userId, @RequestBody final UserRequestDto userRequestDto) {
Copy link
Member

Choose a reason for hiding this comment

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

매개 변수에 final 키워드를 붙이신 이유가 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

매개 변수에 final 키워드를 붙이신 이유가 궁금합니다!

입력된 값의 불변을 보장하기 위해 final을 붙여주었습니다!

@RestController
@RequestMapping(value = "/user", produces = "application/json; charset=UTF8")
@RequiredArgsConstructor
public class UserController extends ResponseEntityGenerator{
Copy link
Member

Choose a reason for hiding this comment

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

controller에 responseEntityGenerator를 상속하신 이유가 궁금합니다!
부모라고 보기에는 조금 부족한 느낌이 듭니다.
controller의 필드로 주입하여 사용하는 방식은 어떨까요?

Copy link
Member Author

@PgmJun PgmJun Apr 28, 2023

Choose a reason for hiding this comment

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

�확장성과 코드 중복을 고려해서 ResponseEntityGenerator를 상속받는 형태로 구현하였습니다!
만약 UserController 이외에 컨트롤러가 더 필요하게 된다면,
그럴 때마다 ResponseEntity 생성부를 클래스 내에 작성해주는 것은 코드의 중복이 발생하기 때문에 비효율적이라 생각하였습니다!

Comment on lines +17 to +19
public UserResponseDto toDto(Long userId) {
return new UserResponseDto(userId, name, age);
}
Copy link
Member

Choose a reason for hiding this comment

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

승준님의 코드를 보면 메소드로 분리해서 기능 구분을 정말 잘 해놓으신다는 생각이 듭니다.

그런데 위의 메소드의 경우 UserResponseDto 뿐만 아니라 유저 생성이나 일부분의 정보를 포함하는 dto를 보내야한다면 그에 따른 모든 메소드를 각각 만들어야 한다는 점이 마음에 걸리는 것 같습니다.

어떤게 좋은 방법인지는 저도 잘 모르겠습니다만, 그런 우려가 듭니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

승준님의 코드를 보면 메소드로 분리해서 기능 구분을 정말 잘 해놓으신다는 생각이 듭니다.

그런데 위의 메소드의 경우 UserResponseDto 뿐만 아니라 유저 생성이나 일부분의 정보를 포함하는 dto를 보내야한다면 그에 따른 모든 메소드를 각각 만들어야 한다는 점이 마음에 걸리는 것 같습니다.

어떤게 좋은 방법인지는 저도 잘 모르겠습니다만, 그런 우려가 듭니다!

흠 어떻게 바꾸는 것이 좋을까요,, 고민해보겠습니다!! 감사합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Android 에서 domain과 data모듈[서버와 통신하는 모듈]의 분리를 위해서 mapper를 data 모듈 쪽에서 만들었어서 찬미님 말에 공감합니다..!
그래서 이번 과제에 dto 쪽에 static 메서드를 하나 만들어서 구현을 해봤는데 이부분 어떻게 생각하시는지 다들 궁금합니다..!

Copy link
Contributor

@KWY0218 KWY0218 left a comment

Choose a reason for hiding this comment

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

심화과제까지 작업하시느라 고생하셨습니다!!
많이 알아갑니다 ~!!
LGTM ~ 😄 👍

Comment on lines +45 to +48
User updatedUser = user.get();
updatedUser.update(updateDto);

userRepository.update(userId, updatedUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

46번째 줄에서 update는 새로운 객체를 만들지 않고 기존 주소 값에 정보를 수정하고 있기 때문에
48번째 줄에서 repository update를 하지 않아도 repository 내에 있는 user도 정보가 바뀌었을 것이라고 추측이 됩니다..!

Comment on lines +17 to +19
public UserResponseDto toDto(Long userId) {
return new UserResponseDto(userId, name, age);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Android 에서 domain과 data모듈[서버와 통신하는 모듈]의 분리를 위해서 mapper를 data 모듈 쪽에서 만들었어서 찬미님 말에 공감합니다..!
그래서 이번 과제에 dto 쪽에 static 메서드를 하나 만들어서 구현을 해봤는데 이부분 어떻게 생각하시는지 다들 궁금합니다..!

Comment on lines +11 to +29
protected ResponseEntity<?> generateSaveResponseEntity(boolean saveResult) {
return (saveResult) ?
new ResponseEntity<>("정보가 성공적으로 등록 되었습니다.", HttpStatus.OK):
new ResponseEntity<>("등록에 실패하였습니다.", HttpStatus.NO_CONTENT);
}

protected abstract ResponseEntity<?> generateFindResponseEntity(Optional<?> domain, Long id);

protected ResponseEntity<?> generateUpdateResponseEntity(boolean updateResult, Long id) {
return (updateResult) ?
new ResponseEntity<>(id + "번 데이터에 대한 정보가 성공적으로 업데이트 되었습니다.", HttpStatus.OK):
new ResponseEntity<>(id + "번 데이터에 대한 정보가 존재하지 않습니다.", HttpStatus.NO_CONTENT);
}

protected ResponseEntity<?> generateDeleteResponseEntity(boolean deleteResult, Long id) {
return (deleteResult) ?
new ResponseEntity<>(id + "번 데이터에 대한 정보가 성공적으로 삭제 되었습니다.", HttpStatus.OK):
new ResponseEntity<>(id + "번 데이터에 대한 정보가 존재하지 않습니다.", HttpStatus.NO_CONTENT);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

image

정보가 존재하지 않는다면 204가 아닌 404가 아닌가 란 생각이 들었습니다..!
혹시 204를 하신 이유가 있는지 궁금합니다!

import java.util.Optional;

@RestController
@RequestMapping(value = "/user", produces = "application/json; charset=UTF8")
Copy link
Contributor

Choose a reason for hiding this comment

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

배워 갑니다 ~ !!!

@PgmJun PgmJun closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants