-
Notifications
You must be signed in to change notification settings - Fork 45
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
1단계 - OneToMany (FetchType.EAGER) #128
base: tmddnrdl333
Are you sure you want to change the base?
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.
안녕하세요 승욱님 😃
1단계 진행 해주셨네요.
요구사항 진행이 안된 것으로 보이는데, README 와 같은 부분에 어떤 생각을 가지시고, 어떤 것들을 진행했는지 나타내주시면 리뷰하기 좀 더 좋을 것 같아요 👍
만들어 주신 부분을 실행시켜보려 하니, 컴파일 에러가 나는데 확인 부탁드립니다 🙇
미션 진행하시며 궁금한 사항이 있으시면 편하게 코멘트 또는 DM 주세요! 😃
import example.entity.Order; | ||
import example.entity.OrderItem; |
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.
어쩌다 누락됐나보네요ㅜㅜ 추가했습니다!
public class EntityUtils { | ||
public static ColumnInfo getIdColumn(Class<?> clazz) { |
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.
EntityUtils
정적 메서드들을 가진 클래스인데 정적클래스가 아닌, 특정 객체에게 책임을 주는것은 어떻게 생각하시나요?
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.
여러가지로 생각하다가 그래도 리뷰어님 의견이 좋은 것 같아서 TableInfo 에게 역할을 위임했습니다!
public class ColumnInfo { | ||
private TableInfo tableInfo; | ||
private Class<?> columnType; | ||
private String columnName; |
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.
ColumnInfo
가 TableInfo
를 가지고 있네요. 구현 외적으로 단순히 이름만 보자면, 개인적으로 TableInfo
가 ColumnInfo
들을 가지고 있는게 더 자연스럽다 생각하는데 어떻게 생각하시나요?
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.
저도 이부분 약간 고민을 했는데,
where 절과 같이 컬럼들 끼리 비교를 하는 부분을 빌더 패턴으로 만드려고 하다보니
column에 해당하는 객체들(ColumnInfo해당) 만 넣고 싶은데
table 정보가 없으면 정상적인 쿼리 생성이 안된다고 생각했습니다.
실제 쿼리에서도 [테이블명].[컬럼명] 과 같이 작성되기도 하고,
컬럼이 어떤 테이블의 컬럼인지 까지 가지고 있으면 좋을 것 같아서 이렇게 설계/구현 해봤는데
테이블 정보를 따로 관리하는 게 더 나을까요?
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 에 관련된 기능들이 만들어진다 생각합니다 :)
쿼리 빌더가 메인 기능뿐만 아니라 각 entity 정보들을 토대로 다양한 기능을 가진 ORM 을 만드는게 목적이니 entity 정보를 다루고 관리하는 클래스가 있으면 좋지 않을까요?
public Class<?> getColumnType() { | ||
return columnType; | ||
} |
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 ColumnInfo onConditionColumn1; | ||
private ColumnInfo onConditionColumn2; |
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.
단순히 1, 2 라는 변수 이름을 보고 바로 명확하게 알기 어려울 것 같아요 😢
public String toString() { | ||
StringBuilder stringBuilder = new StringBuilder(); | ||
stringBuilder | ||
.append("select * from ") |
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.
select * 이 아닌, 필요한 필드명들을 나열해서 쿼리가 나오도록 만들어주시면 감사하겠습니다 🙇
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.
일단 이 부분을 남겨두되 컬럼별로도 조회할 수 있게 추가했습니다...!
logger.debug("query : {}", selectQuery); | ||
String query = selectQuery.toString(); | ||
logger.debug(query); | ||
assertThat(query).isEqualTo("select * from orders where id = 1 inner join order_items on orders.order_id = order_items.id;"); |
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.
혹시 해당 쿼리가 db 에서 실행이 가능한 정상적인 쿼리가 맞을까요?
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.
ㅜㅜ 그렇군요 수정했습니다!
엔티티화 하고 insert 하는 쪽은 아직 못했는데ㅜㅜ |
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.
안녕하세요 승욱님 😃
만들어 주신 부분들에 대한 몇 가지 코멘트 남겨보았습니다. :)
마지막까지 화이팅입니다 🔥
미션 진행하시며 궁금한 사항이 있으시면 편하게 코멘트 또는 DM 주세요! 😃
@Column(name = "order_id") | ||
private Long orderId; |
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.
Order
와 OrderItem
클래스를 추가해주셨네요 👍
다만, 기존 요구사항에는 OrderItem
클래스에는 orderId
필드가 존재하지 않아요 😢
실제 JPA 를 사용할 때, 부모 entity 에는 정보가 있지만 자식 entity 에는 부모 정보가 없거나 반대의 경우도 있는데요! 해당 부분을 해결하려면 어떻게 해야할까요?
public String getFullName() { | ||
return tableInfo.getTableName() + "." + NameUtils.getColumnName(columnField); | ||
} |
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.
ColumnInfo
라는 클래스는 Column 에 대한 정보를 제공해주는 책임을 가질 것 으로 생각이 되는데, SQL 에 필요한 이름을 반환하는 메서드를 가지고 있네요 🤔
만약 SQL 에 대한 정책이 바뀔경우, Column 정보를 제공해주는 클래스와 쿼리빌더 양쪽 다 수정이 될 가능성이 생김으로써 단일책임원칙(SRP)가 위배 될 수 있다고 생각이 들어요.
ColumnInfo
는 단순히 Column 에 대한 정보만 제공해주고, SQL 에 필요한 부분은 쿼리빌더에게 책임을 맡겨보는건 어떨까요? :)
public TableInfo getTableInfo() { | ||
return tableInfo; | ||
} | ||
|
||
public Field getColumnField() { | ||
return columnField; | ||
} |
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.
그런 의미에서 단순히 getter 를 이용하는것보단 좀 더 책임을 줘볼 수 있을 것 같아요 :)
stringBuilder | ||
.append(joinCondition.getJoinType().getValue()) | ||
.append(" ") | ||
.append(joinCondition.getTableInfo().getTableName()) |
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.
예를들면 이런 메시지들을 통해 객체 간 상호작용을 만들어 보는거죠 :)
joinCondition.getTableName();
아실 수도 있지만, 디미터의 법칙 에 대해 읽어보셔도 좋을 것 같아요 👍
@@ -0,0 +1,31 @@ | |||
package persistence.sql.component; | |||
|
|||
public class JoinCondition { |
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.
만들어주신 객체들이 단순히 getter 를 제공하는 경우가 많은데 해당 부분도 개선해보시는건 어떨까요? :)
@DisplayName("test") | ||
void test() { |
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.
조금 더 의미있는 DisplayName
과 메서드명 지어주시면 감사하겠습니다 🙇
public ColumnInfo getIdColumn() { | ||
Field idColumnField = Arrays.stream(entityClass.getDeclaredFields()) | ||
.filter(field -> field.isAnnotationPresent(Id.class)) | ||
.findAny() | ||
.orElseThrow(); | ||
return ColumnInfo.of(this, idColumnField); | ||
} |
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.
getIdColumn
을 TableInfo
의 책임으로 옮겨주셨네요 💯
TableInfo
가 만들어지고 나면, getIdColumn
은 매번 같은 값을 반환할텐데, 상태로 가져보는건 어떨까요?
TableInfo
라는 클래스명을 가진 객체가 IdColumn
을 가지고 있는게 좀 더 자연스럽다 생각해요 :)
public class ColumnInfo { | ||
private TableInfo tableInfo; | ||
private Class<?> columnType; | ||
private String columnName; |
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 에 관련된 기능들이 만들어진다 생각합니다 :)
쿼리 빌더가 메인 기능뿐만 아니라 각 entity 정보들을 토대로 다양한 기능을 가진 ORM 을 만드는게 목적이니 entity 정보를 다루고 관리하는 클래스가 있으면 좋지 않을까요?
public JoinCondition(JoinType joinType, TableInfo tableInfo, ColumnInfo onConditionColumn1, ColumnInfo onConditionColumn2) { | ||
this.joinType = joinType; | ||
this.tableInfo = tableInfo; | ||
this.sourceColumnInfo = onConditionColumn1; | ||
this.targetColumnInfo = onConditionColumn2; | ||
} |
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.
요기도 변수명이 1,2 이렇게 나오고 있네요 😭
구조를 많이 바꾸다보니 작업량이 많아질 것 같아서 일단 PR 올려봅니다..!
계속 진행해나가겠습니다.