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
[kwj1270-issue5] 회원가입 기능 #9
Conversation
private UserJoinResponse() { } | ||
|
||
private UserJoinResponse(final User user) { | ||
System.out.println(user.email()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰할게... 사실상 없이 너무 완벽하네요 💯
build 통과같은 경우에는 BaseTimeEntity부분의 문제라 일단은 넘어가고 바로 리뷰하였습니다 :)
final을 사용하시는 부분에 대해서 엄청 감명깊게 볼 수 있었던 코드였던것 같아요!
여담이지만 통합테스트를 하신 부분들이 있는데 unit test와 integration test로 모듈을 분리해서 사용을 해보는 것도 좋을 것 같아요 :)
protected User() { | ||
} | ||
|
||
private User(final UserBuilder userBuilder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builder를 이런 형태로 반환할 수도 있겠네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 생성자 오버로딩은 많을수록 유연함을 준다고 생각해서 빌더를 넘겨서 사용하도록 했습니다! 👍
@NotEmpty @NotBlank | ||
@JsonProperty("username") | ||
private String username; | ||
|
||
@Email @NotEmpty @NotBlank | ||
@JsonProperty("email") | ||
private String email; | ||
|
||
@NotEmpty @NotBlank |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NotBlank
가 @NotEmpty
보다 더 넓은 validate를 하는데 둘을 함께 사용한 이유가 있을까요?
@Email
도 마찬가지로요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid는 살짝 무지성으로 작성했는데 겹치는 부분이 많나요?
@Email
같은 경우는 @NotBlank
였나 검증을 안한다고 들었습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 찾아서 정리해야겠네요 하루만에 한다고 막 하다보니...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
간단하게만 말씀드리자면 null, "", blank를 검증을 한다고 했을 때
@NotNull
: null
@NotEmpty
: null, ""
@NotBlank
: null, "", blank
를 검증 합니다 :) 이메일은 말씀하신대로 blank는 함께 체크를 합니다 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깔끔하게 정리해주셨군요 😳 감사합니다.
추가적으로 정리해서 다음부터 반영해야겠어요
public static final String EMAIL = "sample@email.com"; | ||
public static final String USERNAME = "username"; | ||
public static final String PASSWORD = "password"; | ||
public static final String BIO = "bio"; | ||
public static final String IMAGE = "image"; | ||
public static final User USER = userBuilder(EMAIL, USERNAME, PASSWORD, BIO, IMAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트에서 이렇게 하는 방법이 있다는 사실 처음알았네요 :) 엄청 좋은 방법인거 같다고도 생각해요 👍
근데 이렇게 만들게 되면 너무 public한 상수들이 뿌려지는데 너무 오픈된 정보가 아닐까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 한번 도전해본 것이기에 그냥 이런 방법이 있구나 하고 보시면 좋을 것 같아요
개인적으로 다른 테스트 코드에 대한 종속이 생긴거여서 나중에 문제 생기지 않을까 싶기도 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아니면 @BeforeEach
(?) 같은 걸로 테스트 전에 먼저 실행된다는 것을 활용할 수 있지 않을까요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 충분히 만들어서 사용할 수 있습니다.
이전 TDD 과정에서 비슷한 코드가 있어서 한 번 적용해봤습니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 하는게 제일 편할것 같네요.... 참고하겠습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoony-lab
참고로, 레포지토리 테스트는 따로 생성해주시는 편이 좋습니다.
전역 testEntityManager
+ static 객체
는 테스트 메서드가 끝나도 계속 메모리에 상주하고 있어서
이 때는 별도로 생성하고 넣어줘야 에러 없이 테스트가 가능합니다.
} | ||
|
||
@Valid | ||
public UserJoinResponse join(final UserJoinRequest userJoinRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Transactional
이 포함이 안되어있네요 :) 안붙인 다른 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이유는 저의 기억력입니다 😭
private String password; | ||
private String bio; | ||
private String image; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 팩토리 생성자를 사용하셨는데 그럼 private constructor가 필요하지 않을까요? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 생각도 못했네요!!! 👍 다음에 반영해보겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
칼럼명을 테이블명_컬럼명
으로 한 것은 다른 테이블의 같은 컬럼명과 혼동하지 않을려고 그런거죠 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네네 맞아요
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
} | |
} |
여담이긴 하지만 한칸 띄우는게 컨벤션이긴 해요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한 칸 띄우는 이유에 대해서 정리한 글이 있는데 추가로 댓글로 올리겠습니다.
빌드 실패는 내일 다시 push 보내겠습니다 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드 많이 배워갑니당 :blobaww:
EnableJpaAuditing 이것도 신기신기
@@ -24,4 +26,4 @@ apply from: 'test.gradle' | |||
|
|||
test { | |||
useJUnitPlatform() | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 아래 한 줄 개행이 필요할듯해요! (아래 Test code들도)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흠.. 개행 컨벤션은 여태 의식하지 않았는데 의식해야겠군요
public class BaseTimeEntity { | ||
|
||
@CreatedDate | ||
@Column(name = "created_at", updatable = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배워갑니다 👍
import javax.validation.Valid; | ||
|
||
@Service | ||
public class UserJoiningService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 네이밍 UserJoinService는 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 바꿨다고 생각했는데 못바꿨네요 수정하겠습니다 :)
this.password = password; | ||
} | ||
|
||
public final User toEntity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public final User toEntity() { | |
public final User toUser() { |
는 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 일부러 Entity라는 이름을 사용하도록 했습니다.
User라는 이름도 괜찮지만 만에 하나 이름이 바뀐다면 메서드 이름도 수정해야되기 때문이죠.
이 부분에는 정답은 없으니 다음에는 이런식으로 반영해보도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만에 하나 이름이 바뀐다면 메서드 이름도 수정해야되기 때문이죠.
오.. 그럴수도 있겠네요
말씀하신대로 장단이 있는 것 같아요!! 어떻게 하든 프로젝트 내에서의 통일성만 있다면 OK인 것 같네용
|
||
@Valid | ||
public UserJoinResponse join(final UserJoinRequest userJoinRequest) { | ||
final User entity = userRepository.save(userJoinRequest.toEntity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final User entity = userRepository.save(userJoinRequest.toEntity()); | |
final User user = userRepository.save(userJoinRequest.toEntity()); |
가 조금 더 명확하지 않을까요?
@@ -0,0 +1,31 @@ | |||
package com.study.realworld.domain.user.ui; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 잘 몰라서 그러는데,
물론 패키지 명은 엄청난 제약이 없겠지만,,, ui는 뭔가 '응?' 하게 되네요!
이렇게도 원래 쓰나요? 저는 presentation, controller만 봐서...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 ddd start와 nextStep DDD 기반으로 디렉토리를 구성했습니다.
실무적으로 더 좋은 방법이 있다면 반영할게요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 거기서 ui로 썼으면 그렇게 써도 되는 것 같아요!
저는 application layer는 application (우지님이 쓰신 것처럼)
presentation layer는 presentation으로만 써봤어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DDD 초보자여서 사실 어느 방법으로 써야 되는지 많이 헷갈리더라고요
하지만 확실히 제가 쓴 ui 보다 presentation이 보다 의미 전달이 잘 될 것 같네요!! 하나 배워갑니다 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찬스님 말씀처럼 패키지명에 제약은 없지만 ui로 패키지명은 프론트의 그 ui와 헷갈려하는 분들이 있을 수도 있다고 생각합니다. 근데 사실 백엔드 개발자가 보기엔 ui, controller, presentation 모두 같은 의미를 담고 있다고 생각할 수도 있으니 요건 고려만 해보셔요
|
||
@PostMapping("/api/users") | ||
public ResponseEntity<UserJoinResponse> join(@Valid @RequestBody final UserJoinRequest userJoinRequest) { | ||
log.info(userJoinRequest.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 들어오는 request의 기록용일듯한데, 굳이 여기에 로깅을 한다면 이게 정확히 뭐가 들어온 것인지 표시 해주는 것이 좋을 것 같아요.
log.info(userJoinRequest.toString()); | |
log.info("join request: {}", userJoinRequest.toString()); |
이런 느낌으로...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로깅도 처음 적용해봤는데 찬스님 방법이 좋을 것 같네요 👍
@Column(name = "delete_at") | ||
private LocalDateTime deletedAt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 deleted_at 입니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오타 😭
private UserRestController userRestController; | ||
|
||
private MockMvc mockMvc; | ||
private ObjectMapper objectMapper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 위에서는 @Autowired 하셨는데 여기서는 setUp에 쓰셨네요
그냥 Autowired로 써도 되지 않을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 스프링 IOC 컨테이너 없이 작성한 단위 테스트여서 그렇습니다.
@Autowired는 IOC 컨테이너 기반으로 돌기에 이를 setUp 에서 처리해줬어요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아... 이 부분 또한 배워갑니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가 설명:
@Autowired
는 스프링 부트 프로젝트에서 의존성 주입을 자동으로 해주기 위해 존재하는 것으로, 현재 테스트 클래스는 스프링 부트에 대한 의존성이 없기 때문에 의존성들을 직접 주입하는 방식을 사용해야 합니다.
|
||
import javax.validation.Valid; | ||
|
||
@Controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RestController 가 맞지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 이런 실수를..?!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰를 먼저 하고 Comment/Approve
로 Submit review
를 하는걸까요 ㅎㅎ
private String password; | ||
private String bio; | ||
private String image; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
칼럼명을 테이블명_컬럼명
으로 한 것은 다른 테이블의 같은 컬럼명과 혼동하지 않을려고 그런거죠 ?
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface UserRepository extends CrudRepository<User, Long> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CrudRepository
를 찾아보니까 아래처럼 되어 있네요 ! (참고)
JpaRepository
extends PagingAndSortingRepository
extends CrudRepository
extends Repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Repository
는 구현체에 있지 않나용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 감사합니다 :)
현 요구사항 단계에서는 JpaRepository 까지 구현하는건 스펙 오버인 것 같고
이후 점진적으로 구현하면서 필요하다 판단되면 추가하려고요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DolphaGo
이 부분에 대해서는 명시적으로 붙여줘야 될지 말아야 될지 고민이 되어서 붙여서 작성해봤습니다.
다음에는 제거해서 작성해볼게요 👍
public static final String EMAIL = "sample@email.com"; | ||
public static final String USERNAME = "username"; | ||
public static final String PASSWORD = "password"; | ||
public static final String BIO = "bio"; | ||
public static final String IMAGE = "image"; | ||
public static final User USER = userBuilder(EMAIL, USERNAME, PASSWORD, BIO, IMAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아니면 @BeforeEach
(?) 같은 걸로 테스트 전에 먼저 실행된다는 것을 활용할 수 있지 않을까요 ?
build.gradle
Outdated
implementation 'org.springframework.boot:spring-boot-starter-data-jdbc' | ||
implementation 'org.springframework.boot:spring-boot-starter-data-jpa' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 ! 저도 지워야겠군요
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface UserRepository extends CrudRepository<User, Long> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Repository
는 구현체에 있지 않나용?
Codecov Report
@@ Coverage Diff @@
## kwj1270 #9 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 2 26 +24
===========================================
Files 1 8 +7
Lines 3 65 +62
===========================================
+ Hits 3 65 +62
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
역시 잘짜시네요 👍
의견만 몇가지 드렸습니다.
@Valid | ||
public UserJoinResponse join(final UserJoinRequest userJoinRequest) { | ||
final User user = userRepository.save(userJoinRequest.toUser()); | ||
return UserJoinResponse.fromUser(user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적인 견해지만 여기서 그냥 User를 Controller에 Return해줘도 좋지 않을까? 하는 생각이 있습니다~
표현 영역에서 데이터에 대한 표현을 수정하는 게 맞다고 보는데 우지님은 어떻게 생각하시는지 궁금합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 비슷한 의견인데, DDD에서는 application 단에서는 presentation 단의 객체를 가지지 않는 것이 좋아요
presentation 에서 내려보낼 때는 가공해서 내려보내고
application 단에서 올려보낼 때는 그냥 바로 올려보내고 presentation 단에서 가공하는게 좋습니다 :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@povia , @chance0523
사실, 저번 스터디에서 언급했듯이 저도 presentation 영역에서 바꿔서 진행을 하는게 좋다고 생각해요.
앞서 찬스님이 언급해주신 내용과 더불어 제 개인적인 견해를 덧붙이자면
Service가 Dto에 의존되어 버리면, 다른 Controller에서도 해당 Dto 형식을 맞춰서 사용해야 하기 때문이죠
(다양한 컨트롤러에서 해당 서비스 사용하기 힘들어짐)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 스터디에서 피드백을 통해 사용하면 좋을 것, 사용하기 조금 껄끄러운 것들을 정의하기 위해서
조금 다양하게 시도해보려 하는데 dto 사용은 확실히 좋지 않을 것 같네요 :) 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service가 Dto에 의존되어 버리면, 다른 Controller에서도 해당 Dto 형식을 맞춰서 사용해야 하기 때문이죠
(다양한 컨트롤러에서 해당 서비스 사용하기 힘들어짐)
오 이렇게 생각은 못해봤는데 좋은 생각이네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chance0523
DTO 공부할 때 참고했던 사이트 인데 여기서 인사이트 얻었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service가 Dto에 의존되어 버리면, 다른 Controller에서도 해당 Dto 형식을 맞춰서 사용해야 하기 때문이죠
(다양한 컨트롤러에서 해당 서비스 사용하기 힘들어짐)
간결하게 모든걸 정리해줄 수 있는 좋은 문장인것 같아요 :)
@@ -0,0 +1,31 @@ | |||
package com.study.realworld.domain.user.ui; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찬스님 말씀처럼 패키지명에 제약은 없지만 ui로 패키지명은 프론트의 그 ui와 헷갈려하는 분들이 있을 수도 있다고 생각합니다. 근데 사실 백엔드 개발자가 보기엔 ui, controller, presentation 모두 같은 의미를 담고 있다고 생각할 수도 있으니 요건 고려만 해보셔요
@PostMapping("/api/users") | ||
public ResponseEntity<UserJoinResponse> join(@Valid @RequestBody final UserJoinRequest userJoinRequest) { | ||
log.info(JOIN_REQUEST_LOG_MESSAGE, userJoinRequest.toString()); | ||
return ResponseEntity.ok().body(userJoinService.join(userJoinRequest)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 방식으로 넘길 수도 있었네요 저도 반영하겠습니다~ 👍
private UserRestController userRestController; | ||
|
||
private MockMvc mockMvc; | ||
private ObjectMapper objectMapper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가 설명:
@Autowired
는 스프링 부트 프로젝트에서 의존성 주입을 자동으로 해주기 위해 존재하는 것으로, 현재 테스트 클래스는 스프링 부트에 대한 의존성이 없기 때문에 의존성들을 직접 주입하는 방식을 사용해야 합니다.
Issue: #5
작업 내용
생성/변경 로직
개인 코멘트