-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feat(#31) 현재 진행 중인 질문 목록 조회 #51
The head ref may contain hidden characters: "feature/#31-feat-\uD604\uC7AC-\uC9C4\uD589-\uC911\uC778-\uC9C8\uBB38-\uBAA9\uB85D-\uC870\uD68C"
Conversation
Test Results52 tests 51 ✅ 3s ⏱️ Results for commit 573ca0c. ♻️ This comment has been updated with latest results. |
jaknaeso-core/src/test/java/org/nexters/jaknaesocore/domain/survey/model/SurveyBundleTest.java
Outdated
Show resolved
Hide resolved
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.
수고하셨습니다!!
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/common/model/BaseTimeEntity.java
Show resolved
Hide resolved
.../main/java/org/nexters/jaknaesocore/domain/survey/repository/SurveySubmissionRepository.java
Outdated
Show resolved
Hide resolved
...eso-core/src/main/java/org/nexters/jaknaesocore/domain/survey/dto/SurveyHistoryResponse.java
Show resolved
Hide resolved
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/domain/survey/service/SurveyService.java
Show resolved
Hide resolved
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/domain/survey/service/SurveyService.java
Outdated
Show resolved
Hide resolved
|
||
import java.util.List; | ||
|
||
public record SurveyHistoryResponse( |
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.
p3.
@Builder
어노테이션 추가하는 건 어떠신가요?!
서비스 계층에서 해당 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.
예전에 저도 record
를 쓰면서 @Builder
를 같이 쓰려고했는데 권장하지 않는다고 하나..? 그랬던 것 같아요. 저는 사실 그래서 개인적으로 record
보다 class
에 @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.
흠.. builder를 사용할 지, 정적 팩토리 메소드를 사용할지 정하는 게 좋겠네요.
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/domain/survey/service/SurveyService.java
Show resolved
Hide resolved
1e809eb
to
b459fcd
Compare
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.
고생하셨어요. 홈페이지 작업은 수민 님이 진행하기로 하셔서 api 스펙 직접 전달해서 확인해보시면 좋을 것 같습니다!
|
||
import java.util.List; | ||
|
||
public record SurveyHistoryResponse( |
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를 사용할 지, 정적 팩토리 메소드를 사용할지 정하는 게 좋겠네요.
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/domain/survey/service/SurveyService.java
Outdated
Show resolved
Hide resolved
jaknaeso-core/src/main/java/org/nexters/jaknaesocore/domain/survey/service/SurveyService.java
Outdated
Show resolved
Hide resolved
jaknaeso-core/src/test/java/org/nexters/jaknaesocore/domain/survey/model/SurveyBundleTest.java
Outdated
Show resolved
Hide resolved
surveyRepository.saveAll(List.of(survey1, survey2, survey3, survey4)); | ||
List<KeywordScore> scores = | ||
List.of( | ||
KeywordScore.builder().keyword(Keyword.ACHIEVEMENT).score(BigDecimal.ONE).build(), |
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를 오용할 수도 있겠다고 말씀해주셨는데, 저처럼 Builder를 호출할 수 있는 Fixture를 만드는 거에 대해선 어떻게 생각하시나요? (AppleIdTokenFixture
클래스 참고바람)
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.
근데 저는 사실 테스트 Only를 상정하고 만든게 아니라 저는 기본적으로 생성자 대신 빌더를 쓰는걸 선호해서 빌더로 사용하였습니다.
AppleIdToken
의 @Builder
를 지우고 TestFixture
부분으로 @Builder
를 가져간다고 하더라도 사실은 생성자를 막은게 아니라서... new
를 통해서 실수하나 builder().build()
를 통해서 실수하나 동일한 부분이라고 생각합니다.
전 오히려 @Builder
를 사용해서 주의해야 할 것은 id
와 같이 생성되어선 안될 필드가 생성자를 통해서 생성되는 부분이라고 생각해서 이 부분은 사용해도 괜찮다고 생각합니다.
오히려 해당 부분을 @Builder
를 사용하지 않으면 createKeywordFixture(Keyword.ACHIEVEMENT, BIGDECIMAL.ONE)
이렇게 사용하거나 new KeywordScore(Keyword.ACHIEVEMENT, BIGDECIMAL.ONE)
로 사용하게 될 것 같은데 builder의 장점인 해당 필드의 어떤 값이 들어갈 수 있는지 확인할 수 있는 이점을 잃는 것이라고 생각합니다.
결론적으로 생성자 대신 @Builder
를 사용하고, 해당 @Builder
를 우리가 생성자를 조심히 사용하는 것 처럼 사용한다면 문제될건 없다고 생각합니다. 오히려 Fixture를 만들어내는게 안해도될 고생을 한다고 생각해요. AppleIdTokenFixture
또한 Fixture가 아닌 new AppleIdTokenFixture(토큰값)
이런식으로 해결해도 되는거 아니었을까요 ??
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.
ㅇㅎ 빌더쓰는 걸 선호하셨군요! 그건 제가 생각을 못했네요.
AppleIdToken의
@Builder
를 지우고 TestFixture 부분으로@Builder
를 가져간다고 하더라도 사실은 생성자를 막은게 아니라서... new 를 통해서 실수하나 builder().build()를 통해서 실수하나 동일한 부분이라고 생각합니다.
AppleIdToken
의 생성자가 package private이기 때문에@Builder
보다 실수가 나올 가능성이 훨씬 적다고 생각합니다.
오히려 해당 부분을
@Builder
를 사용하지 않으면 createKeywordFixture(Keyword.ACHIEVEMENT, BIGDECIMAL.ONE)이렇게 사용하거나 new KeywordScore(Keyword.ACHIEVEMENT, BIGDECIMAL.ONE) 로 사용하게 될 것 같은데 builder의 장점인 해당 필드의 어떤 값이 들어갈 수 있는지 확인할 수 있는 이점을 잃는 것이라고 생각합니다.
- 제가 말한 Fixture는 builder 패턴을 쓰기 때문에 helper method를 사용하지 않고, 케이스에서 빌더를 사용한다면 어떤 값을 넣어주는지 확인할 수 있습니다. 아래 코드 확인되신 게 맞을까요?
class AppleIdTokenFixture {
@Builder
private static AppleIdToken createAppleIdToken(final String idToken) {
return new AppleIdToken(idToken);
}
}
AppleIdTokenFixture 또한 Fixture가 아닌 new AppleIdTokenFixture(토큰값) 이런식으로 해결해도 되는거 아니었을까요 ??
- 이 부분 또한 공감하는 부분이지만, 만약
AppleIdTokenFixture
는 필드가 적어서 그냥 생성자를 사용하는 것도 괜찮다고 생각합니다. 다만, 필드가 많은 객체라면 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.
- 저는 저희 컨벤션이 정적 팩토리 메소드를 사용하는 거라고 생각해서 빌더를 붙이지 않았습니다.
- 이 부분은 @kimyu0218 님과 상의해서 통일이 필요할 것 같네요.
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
사용하는게 좋다고 생각합니다. 생성자가 빌더에 비해 이점이 크게 있다고 생각하지 않아서요.
정적팩토리 메서드를 사용하더라도 생성자를 빌더로 정의해두고 정적팩토리 메서드를 사용하는게 전 좋을 것 같다고 생각합니다.
public class KeywordScore {
private Keyword keyword;
private BigDecimal score;
@Builder
private KeywordScore(final Keyword keyword, final BigDecimal score) {
this.keyword = keyword;
this.score = score;
}
public static KeywordScore create(Keyword keyword, BigDecimal score) {
return KeywordScore.builder()
.keyword(keyword)
.score(score)
.build();
}
}
@Builder
를 사용하면 public하게 사용되어서 남용될 수 있다는점도 인정하고 인지하고 있지만, 그렇다고 한다면 저는 오히려 생성자를 private으로 바꾸고 온전히 정적 팩토리메서드를 통해서만 생성해야된다고 생각해요. 이렇게 하지않으면 @Builder
를 사용하지 않는것과 다른게 없다고 여겨지거든요. 말씀하시는 부분도 이렇게 하자는걸로 보이고요
이점이 크냐 손실이 크냐를 비교했을 땐 전 이점이 훨씬 크다고 생각하고있고, 정적 팩토리메서드를 사용한다고 하면 기존의 코드들은 생성자대신 정적팩토리메서드로만 변경하면 된다고 생각합니다.
물론 휴먼에러를 줄이기 위해서 이런구조를 가져가는건 좋다고 생각합니다만.... 생성자 남용하는거 막자고 돌아서돌아서 가는거라고 느껴진달까요. 결국 테스트하기위해서는 모든 객체에 대해서 Fixture를 만들어야된다는 뜻아닌가요?
테스트코드 살펴보니 기존의 프로덕션의 Of 메서드가 있음에도 Fixture의 builder를 재정의해서 쓰신이유는 of자체가 하나의 메서드라 생각하고, 생성자를 통해서 given데이터를 셋업해야된다고 여겨서 했다고 생각하는데 이렇게 되면 모든 객체에 대해서 Fixture를 만들어가고 오히려 너무 불편한 상황을 야기하지않을까도 걱정이되긴합니다.
작업 개요
현재 진행중인 질문 목록 조회 API 구현
작업 사항
BaseTimeEntity
에AuditingListener
가 적용되어있지 않아서 시간이 들어가고있지 않아서 추가하였습니다.고민한 점들(필수 X)
테스트에서
@Transactional
사용회원이_제출한_가장_마지막_설문을_조회한다
에서@Transactional
을 사용하고 있는데 사용하지 않으면 테스트에서 지연로딩을 활용한 테스트를 할 수 없어서 사용하였는데 이 경우에는 어떻게 하는게 좋을지 의견있으시면 주시면 감사하겠습니다.해당 메서드를 통해서
bundleId
를 검증하려고 하는게 테스트하면서 욕심인건지..? 궁금합니다.네이밍 쿼리 이렇게 사용해도 괜찮은지도 알려주시면 감사하겠습니다 !
남은 작업