Step3 PR 입니다 #227
Step3 PR 입니다 #227
Conversation
There was a problem hiding this comment.
(반영하지 않으셔도 됩니다.) 지금도 충분히 잘 구현해주셔서 변경하지 않으셔도 되는데 제가 의도했던 바만 코멘트 추가로 남기겠습니다.
PkColumn와 SimpleColumn을 생각해보았을때 PkColumn은 SimpleColumn에서 추가로 id와 관련된 기능이 추가된 클래스에요. 그래서 PkColumn이 SimpleColumn을 들고 있고 그 둘을 Column이라는 인터페이스로 묶는건 어떨까하여 드린 피드백이었습니다. 구현의 차이가 있을 것 같은데 추상화를 통해서 공통된 부분을 묶는다는 컨셉은 동일해요 😄
public interface Columnpublic class PkColumn implements Column {
private final String strategy;
private final SimpleColumn;
// ...public class SimpleColumn implements Column {
private final String name;
private final Type type;
// ...| @@ -57,4 +43,23 @@ public String getDDLColumnQuery() { | |||
| if (condition.isUnique()) sb.append(" ").append("UNIQUE"); | |||
| return sb.toString(); | |||
| } | |||
|
|
|||
| public String getQueryValue(Object value) { | |||
| if (Objects.isNull(value)) return null; | |||
There was a problem hiding this comment.
QueryValue에 사용하는 정보를 반환하기 때문에 null을 반환하기 보다는 "null"로 String값을 반환하는게 좋을 것 같아요. null을 출력하는 것은 동일하지만 null 자체를 반환하는 것과 출력에 필요한 "null"을 반환하는건 기능상의 차이가 있습니다. null을 반환하여 외부에서 이를 컨트롤하도록 기대하는 것은 프로그램의 장애를 발생시키는 요인이 될 수 있어요.
| } | ||
|
|
||
| var column = columnAnnotation.get(); | ||
| var name = column.name().isBlank() ? field.getName() : column.name(); |
There was a problem hiding this comment.
(반영하지 않으셔도 됩니다.) 삼항연산자보다는 if else를 직접 표현하는 것이 가독성이 더 올라갑니다. 이정도의 기능이라면 parseColumnName()이라는 메소드로 분리해보는건 어떨까요?
private static String parseColumnName(Field field, jakarta.persistence.Column column) {
if (column.name().isBlank()) {
return field.getName();
}
return column.name();
}| public static Optional<Column> createColumn(Field field) { | ||
| boolean isPKColumn = false; | ||
| var isTransient = false; | ||
| for(var annotation : field.getAnnotations()) { | ||
| if (annotation.annotationType().equals(Id.class)) isPKColumn = true; | ||
| if (annotation.annotationType().equals(Transient.class)) isTransient = true; | ||
| } | ||
|
|
||
| if (isTransient) return Optional.empty(); | ||
| if (isPKColumn) return Optional.of(PKColumn.from(field)); | ||
| return Optional.of(SimpleColumn.from(field)); | ||
| } |
There was a problem hiding this comment.
| public static Optional<Column> createColumn(Field field) { | |
| boolean isPKColumn = false; | |
| var isTransient = false; | |
| for(var annotation : field.getAnnotations()) { | |
| if (annotation.annotationType().equals(Id.class)) isPKColumn = true; | |
| if (annotation.annotationType().equals(Transient.class)) isTransient = true; | |
| } | |
| if (isTransient) return Optional.empty(); | |
| if (isPKColumn) return Optional.of(PKColumn.from(field)); | |
| return Optional.of(SimpleColumn.from(field)); | |
| } | |
| public static Optional<Column> createColumn(Field field) { | |
| if (field.isAnnotationPresent(Transient.class)) { | |
| return Optional.empty(); | |
| } | |
| if (field.isAnnotationPresent(Id.class)) { | |
| return Optional.of(PKColumn.from(field)); | |
| } | |
| return Optional.of(SimpleColumn.from(field)); | |
| } |
로 isAnnotationPresent()를 활용하면 더 직관적으로 표현될 수 있을 것 같아요.
(반영하지 않으셔도 됩니다.) 이와는 별개로 Factory가 과연 필요할지는 고민이 필요할 것 같아요. Column에서 정적 팩토리 메소드로 들고있어도 되지 않을까요? Factory는 복잡한 로직에 대해서 생성에 대한 책임을 분산시킬수 있도록 도움을 주기도 하지만 자칫 생성에 대한 책임이 분산되어 클래스가 늘어나 클래스 관리가 더 힘들어질 수 있어요.
| .map(ColumnFactory::createColumn) | ||
| .filter(Optional::isPresent) | ||
| .map(column -> column.get().getDDLColumnQuery()) |
There was a problem hiding this comment.
entity에서 Column을 추출해서 List로 들고있어도 되지 않을까요? Columns로 일급컬랙션을 만든다면 이런 로직들이 전부 이관되고, 이렇게 List<Column>을 사용하는 다양한 클래스들에서 사용될 수 있을 것 같아요.
| private String getTableName(Class<?> clazz) { | ||
| var annotations = clazz.getAnnotations(); | ||
| return Arrays.stream(annotations) | ||
| .filter(annotation -> annotation.annotationType().equals(Table.class)) | ||
| .map(table -> ((Table) table).name()) | ||
| .findFirst() | ||
| .orElse(clazz.getSimpleName()); | ||
| } |
There was a problem hiding this comment.
Column과 마찬가지로 Class<?>를 받아 테이블 이름을 가지는 TableName 클래스가 있으면 이 책임이 거기로 이관 될 것 같네요.
public class TableName {
private final String value;
public TableName(Class<?> clazz) {
// 테이블 어노테이션이 있으면 그 값을 사용하고 아니면 클래스 명 사용
}| var object = clazz.getDeclaredConstructor().newInstance(); | ||
| idField.setAccessible(true); | ||
| idField.set(object, id); |
There was a problem hiding this comment.
Object로 넘기게 된다면 사용하는 메소드에서 타입과 관련된 정보를 알 수 없어요. 사용하는 측에서 무엇을 사용해야하는지 명확하게 하기 위해서 하나의 클래스를 만들어보는 건 어떨까요?
이미 잘 만들어주신 Column을 사용한다면 Map<Column, Object>를 가지고 있는 일급컬랙션하나를 만들 수 있을 것 같아요. 그럼 whereClause쪽 코드들의 복잡도가 내려갈 것 같은데 어떻게 생각하세요?
| private String valueClause(Object object) { | ||
| var clazz = object.getClass(); | ||
| return Arrays.stream(clazz.getDeclaredFields()) | ||
| .filter(this::isInsertQueryTarget) | ||
| .map(field -> getColumnValue(field, object)) | ||
| .filter(Optional::isPresent) | ||
| .map(Optional::get) | ||
| .collect(Collectors.joining(", ")); | ||
| } | ||
| private boolean isInsertQueryTarget(Field field) { | ||
| var isId = false; | ||
| var isGeneratedValue = false; | ||
| var isColumn = false; | ||
|
|
||
| for (Annotation annotation : field.getAnnotations()) { | ||
| if (annotation instanceof Column) { | ||
| isColumn = true; | ||
| } else if (annotation instanceof Id) { | ||
| isId = true; | ||
| } else if (annotation instanceof GeneratedValue) { | ||
| isGeneratedValue = true; | ||
| } | ||
| } | ||
|
|
||
| return isColumn || (isId && !isGeneratedValue); | ||
| } |
There was a problem hiding this comment.
이런 로직들도 모두 Column, Columns를 활용한다면 충분히 옮겨질 수 있는 로직으로 보여요. 이미 만들어진 로직과 겹치는 부분들도 보이구요.
DMLHelper는 dml 쿼리를 만드는 역할을 가지고 있고, Class<?>를 받아 쿼리를 생성하긴 하지만 리플랙션에 관련된 책임들은 모두 model패키지에 잘 만들어주신 도메인 객체들에게 넘겨주시면 좋을 것 같아요.
3단계 리뷰 요청 드립니다!!
감사합니다.