-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor: vo,엔티티 리팩터링 #483
The head ref may contain hidden characters: "refactor/482-VO,\uC5D4\uD2F0\uD2F0_\uB9AC\uD329\uD130\uB9C1"
Conversation
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.
고생하셨습니다~
머지 전에 자잘한 부분 코멘트확인만 부탁드릴게요!!!
@Column(name = "special_manage_info") | ||
private String specialManageInfo; | ||
@Embedded | ||
private Property property; |
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.
A
Property로 묶으니까 확실히 깔끔하네요! 👍
|
||
@Embedded | ||
private WaterCycle waterCycle; | ||
private CareDetail careDetail; |
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.
A
CareDetail 이름 좋은데요? 👍
@Column(name = "last_water_date", nullable = false) | ||
private LocalDate lastWaterDate; | ||
@Embedded | ||
private WaterDate waterDate; |
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.
A
WaterDate도 좋은 것 같은데 물주기 요소들이 묶여있다는 의미가 와닿지 않는것 같습니다. (@Embedded
느낌보다는 하나의 Column같은 느낌이네요)
위에서 선언했던 CareDetail이라는 명칭처럼 WaterDetail은 어떨까요?
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.
오호 좋습니다 ! 반영하게융
return null; | ||
return Optional.empty(); |
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.
A
null 반환 대신 Optional.empty 반환 좋네요 👍
@@ -6,5 +6,5 @@ | |||
|
|||
public interface DictionaryPlantRepository extends JpaRepository<DictionaryPlant, Long> { | |||
|
|||
List<DictionaryPlant> findDictionaryPlantsByNameContains(String name); | |||
List<DictionaryPlant> findDictionaryPlantsByClassification_NameContains(String name); |
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.
A
_
가 들어가있는게 다소 어색해보이긴한데 CRUDRepository 명명 규약이여서 어쩔수 없으려나요 😅
메소드가 다소 지저분해보여서 되려 사용하는 쪽에서 가독성이 떨어질 수도 있겠다는 생각이 들어요 🥺
JPQL을 사용하는 방식은 어떤가요?
// 예시코드
@Query("SELECT dp FROM DictionaryPlant dp WHERE dp.name LIKE %:name%")
List<DictionaryPlant> searchDictionaryPlantsByName(String name);
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.
하마드, 그레이 고생 많으셨습니다!
컨벤션 관련 코멘트랑 궁금한 점 조금 남겨봤어요.
필요하다고 느껴지시면 수정하시고 merge 하시면 될 것 같습니다!
고생하셨어요 ~
그리고 다들 새복많~~
PetPlantResponse petPlantResponse = petPlantService.create(request, multipartFile, member); | ||
return ResponseEntity.created(URI.create("/pet-plants/" + petPlantResponse.getId())).build(); | ||
} | ||
|
||
@GetMapping | ||
public ResponseEntity<DataResponse<List<SinglePetPlantResponse>>> readAll( | ||
@Auth Member member) { | ||
public ResponseEntity<DataResponse<List<SinglePetPlantResponse>>> readAll(@Auth Member member) { |
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.
c
controller 파라미터 컨벤션을 맞추고 계신 것 같은데, 여기는 다른 변경 사항들이랑 반대로 된 것 같아요!
파라미터가 한 개라 이렇게 수정했나 했는데, 다른 컨트롤러 보니까 아니네유
public ResponseEntity<DataResponse<List<SinglePetPlantResponse>>> readAll(@Auth Member member) { | |
public ResponseEntity<DataResponse<List<SinglePetPlantResponse>>> readAll( | |
@Auth Member member | |
) { |
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.
String requireHumidity, String postingPlace, String specialManageInfo, | ||
WaterCycle waterCycle) { | ||
this.name = name; | ||
public DictionaryPlant( |
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.
A
접근제어자를 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.
넵 반영하겠슴다 !
private WaterCycle waterCycle; | ||
|
||
@Builder | ||
public CareDetail( |
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 familyName; | ||
|
||
@Builder | ||
public Classification(String name, String familyName) { |
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.
A
동일함다
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 PetPlantState(String location, String flowerpot, String light, String wind) { | ||
validateEmptyValue(location); | ||
validateEmptyValue(flowerpot); | ||
validateEmptyValue(light); | ||
validateEmptyValue(wind); |
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.
A
각 필드에 @NotBlank
애너테이션을 붙여주고 nullable =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.
@NotBlank
는 영속성 컨텍스트에 객체가 들어가야 검증이 되는 부분이었습니다 ㅎㅎ
그래서 애플리케이션 내 순수한 도메인 검증도 함께 해주기 위해 작성했었습니다 😀
private String growSpeed; | ||
|
||
@Builder | ||
public Property(String smell, String poison, String manageLevel, String growSpeed) { |
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.
A
여기두영
private String minimumTemp; | ||
|
||
@Builder | ||
public Temperature(String requireTemp, String minimumTemp) { |
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.
A
여기두영
return CareDetail.builder() | ||
.temperature( | ||
toTemperature() | ||
) | ||
.requireHumidity( | ||
this.requireHumidity | ||
) | ||
.postingPlace( | ||
this.postingPlace | ||
) | ||
.specialManageInfo( | ||
this.specialManageInfo | ||
) | ||
.waterCycle( | ||
toWaterCycle() | ||
) | ||
.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.
A
이건 개인적인 궁금증인데유. 왜 이 친구들은 줄바꿈들이 되어있나요?? ㅎㅎ
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.
리팩터링하다가 빼먹었네욤 ㅎㅎ 통일시키겠씁니다 !!!
@Embedded | ||
private Classification classification; | ||
|
||
@Column(name = "special_manage_info") | ||
private String specialManageInfo; | ||
@Embedded | ||
private Property property; | ||
|
||
@Embedded | ||
private WaterCycle waterCycle; | ||
private CareDetail careDetail; |
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.
A
이렇게 묶으니까 코드 리뷰하는 입장에서 정말 좋네요!! 코드도 깔끔해지고 가독성 완전 좋아요. 근데 리팩토링하면서 힘들었을 것 같아요 ㅋㅋㅋㅠ 고생하셨습니다 ㅎㅎ
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.
소중한 리뷰 너무 감사드리옵니다 🙇🏻
※ 해당 PR 및 커밋은 이건회(@rawfishthelgh )와 함께 몹 프로그래밍으로 작성했습니다
🔥 연관 이슈
🚀 작업 내용
리팩토링을 진행했습니다 🌱
곧 패키지 구조도 함께 변경할 계획입니다.
💬 리뷰 중점사항
큰 변경사항은 없습니다. 🙇🏻