feat: 4단계 - Simple Entity Object#229
feat: 4단계 - Simple Entity Object#229rolroralra wants to merge 4 commits intonext-step:rolroralrafrom
Conversation
rolroralra
commented
Mar 10, 2024
rolroralra
commented
- fix: code review 반영
- feat: 4단계 - Simple Entity Object
- 요구사항1 - find
- 요구사항2 - persist (insert)
- 요구사항3 - remove (delete)
- QueryTranslator 역할 분리 - 1차적으로는 역할별로 분리하고 위임
- 요구사항1 - find - 요구사항2 - persist (insert) - 요구사항3 - remove (delete)
leeyohan93
left a comment
leeyohan93
left a comment
There was a problem hiding this comment.
안녕하세요 신영님! 😄
피드백도 빠르게 반영해주시고 구현도 너무 잘해주셨습니다! 💯
다음 단계로 넘어가기 전에 고민해볼 만한 부분에 대해 코멘트 드렸으니 확인부탁드릴게요!
이번 한주도 화이팅입니다! 🙏
| return String.format( | ||
| "INSERT INTO %s (%s) VALUES (%s)", | ||
| tableTranslator.getTableNameFrom(entityClass), | ||
| columnTranslator.getColumnNamesClauseWithoutPrimaryKey(entityClass), | ||
| columnValueTranslator.getColumnValueClause(entity) | ||
| ); |
There was a problem hiding this comment.
아래 두 메서드는 InsertQuery를 만들기 위해서만 사용되고 있습니다!
columnTranslator.getColumnNamesClauseWithoutPrimaryKey(entityClass),
columnValueTranslator.getColumnValueClause(entity)
InsertQuery를 만드는 책임을 한 곳으로 모을 수 도 있을 것 같은데 어떻게 생각하시나요? :)
책임을 나누는 것이 더 나은 것 같다고 하신다면 어떤 이유에서인지도 궁금합니다 😄
There was a problem hiding this comment.
넵 너무 혼란스러운 구조로 해놓은 거 같습니다.
우선은 제가 생각하는 방향으로 최대한 수정해보았습니다. (ec75f60)
정확히는 어떻게 역할을 나누고, 응집력을 높이는것이 좋은지 명확하게는 떠오르지가 않습니다.
혹시 추천하시는 방향이 있다면 공유주시면 감사하겠습니다.
| public static class LazyLoadEntityRowMapperFactory { | ||
| private static final EntityRowMapperFactory INSTANCE = new EntityRowMapperFactory(); | ||
| } |
There was a problem hiding this comment.
LazyLoad 는 보통 객체가 필요할 때 생성하거나 조회하기 때문에 구현해주신 내용은 캐시라는 네이밍이 조금 더 어울리지 않을까요? 😄
There was a problem hiding this comment.
LazyLoader 패턴이어서 그렇게 기입했지만, 캐시라는 표현이 더 좋을거 같네요.
| import persistence.sql.ddl.type.DataTypeMapping; | ||
| import persistence.sql.ddl.type.impl.DefaultDataTypeMapping; | ||
|
|
||
| public class ColumnTranslator { |
There was a problem hiding this comment.
이전에 이야기드렸던 피드백을 적용해주시면서 QueryTranslator 의 책임이 잘 분배되었는데요!
다만 ColumnTranslator 와 같은 클래스들은 여러 쿼리 빌더들에서 의존하면서 여러 쿼리를 만드는 책임을 여전히 가지고 있기 때문에 해당 클래스도 책임을 적절히 위임해보셔도 좋을 것 같습니다 :)
There was a problem hiding this comment.
우선 제가 생각하는 방안으로 수정해보았습니다. (ec75f60)
확인해주시고, 높은 응집력과 약한 결합력을 더 준수하수 있는 방향이 있다면, 공유 부탁드립니다.
- 역할 분리... (솔직히 어떤게 좋을지 잘 모르겠습니다.) - 괜찮은 구조있다면, 공유해주세요~
leeyohan93
left a comment
leeyohan93
left a comment
There was a problem hiding this comment.
안녕하세요 신영님!
코드에서 많은 고민을 해보신 흔적이 느껴지네요! 👍 👍
DM으로 이야기드린 것처럼 해당 부분은 일단 고민으로 묻어두고 다음 단계를 진행하셔도 괜찮습니다!
궁금한 점이 있다면 편하게 DM 주시고 다음 단계 진행을 원하시면 바로 PR요청 보내주세요!
감사합니다 🙏
| protected Stream<Field> getColumnFieldStream(Class<?> entityClass) { | ||
| return Arrays.stream(entityClass.getDeclaredFields()) | ||
| .filter(field -> !field.isAnnotationPresent(Transient.class)) | ||
| .sorted(Comparator.comparing(field -> field.isAnnotationPresent(Id.class) ? 0 : 1)); | ||
| } |
There was a problem hiding this comment.
추상 클래스를 이용하는 방식으로 수정해보셨네요 ! 👍 👍
다만 추상클래스를 사용하게되면 자식 클래스가 부모 클래스의 구조를 알아야하는 단점이 있는 것 같습니다.
그리고 책임에 대해 생각해보면 지금 쿼리 빌더는 아래와 같이 두 가지 책임을 가지고 있는데요.
- 엔티티 클래스의 정보(어노테이션, 필드 이름 등)를 조회한다.
- 엔티티 클래스의 정보들을 토대로 SQL을 만든다.
쿼리 빌더에서 SQL 만을 만드는 책임만 남기고,
클래스 정보를 얻어오는 책임은 협력을 통해 별도의 객체를 만들어 위임해보는건 어떨까요?
3 participants
