Skip to content
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

2단계 - QueryBuilder DDL #225

Open
wants to merge 6 commits into
base: parkseoldev
Choose a base branch
from

Conversation

ParkSeolDev
Copy link

코드 리뷰 감사합니다.

Copy link

@leeyohan93 leeyohan93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 박설님! 😄
2단계 미션을 잘 구현해주셨습니다!
쿼리 빌더가 난이도가 높은 미션 인데 끝까지 힘내주셔서 감사해요! 💯
개선해보면 좋을 만한 부분에 코멘트를 남겨두었으니 확인부탁드리고
어려운 점이나 궁금한 점이 있으면 DM으로 많이 물어봐주세요! 감사합니다 🙏

Comment on lines +10 to +20
@Test
void createDDL() throws
InvocationTargetException,
NoSuchMethodException,
InstantiationException,
IllegalAccessException {
CreateQueryBuilder createQueryBuilder = new CreateQueryBuilder();
String createTableSql = createQueryBuilder.getCreateTableSql();

assertEquals(createTableSql, "CREATE TABLE person (id INT AUTO_INCREMENT PRIMARY KEY, name VARCHAR(50) NOT NULL, age INT)");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소하지만 테스트 시에는 아래와 같이 예외의 상위 클래스인 Exception을 사용하면 잘 정리할 수 있습니다!

    @Test
    void createDDL() throws Exception {

그리고 Person 클래스를 선언할 때 아래와 같이 데이터베이스에 사용되는 테이블 명이나 컬럼 명을 설정 해주었었는데요!
아래와 같이 쿼리 문에 함께 반영되면 좋을 것 같습니다! 😄

CREATE TABLE users (
    id BIGINT AUTO_INCREMENT PRIMARY KEY,
    nick_name VARCHAR(255),
    old INTEGER,
    email VARCHAR(255) NOT NULL
);


import domain.Person;

public class DropQueryBuilder {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DropQueryBuilder 에 대한 테스트도 함께 작성되어도 좋을 것 같아요 😄


Class<Person> personClass = Person.class;

Person person = personClass.getConstructor().newInstance();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용되지 않는 변수를 제거해주시면 좋을 것 같아요! 🙏

Comment on lines +23 to +29
public String getCreateTableSql() throws
NoSuchMethodException,
InvocationTargetException,
InstantiationException,
IllegalAccessException {

Class<Person> personClass = Person.class;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금은 해당 쿼리를 만드는 메서드의 기능이 Person 클래스에 한정되어있는데요!

파라미터로 클래스를 받아서 다른 클래스에 대해서도 기능이 동작하도록 개선해보는건 어떨까요!?

public String getCreateTableSql(Class<?> clazz)  {
    // 내부 구현..
}

Comment on lines +124 to +127
// DB에서 가져와야겠다 ai를 //
private static String generateValue() {
return "AUTO_INCREMENT";
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

데이터베이스 마다 해당 기능에 대한 쿼리가 다를 것이라고 생각하고 고민해주신 것 같아요! 👍 👍

다만 지금은 하나의 DB 라고 생각하고 구현 범위를 좁혀서 개발해보시는 것을 추천드립니다!

그렇다면 해당 문자열은 아래 처럼 상수로 만들어 사용할 수도 있겠네요!

private static final AUTO_INCREMENT_QUERY = "AUTO_INCREMENT"

getPrimaryKey()도 동일한 피드백입니다! 😄

Comment on lines +129 to +134
private static String isNull(boolean isNullable) {
if (!isNullable) {
return "NOT NULL";
}
return "";
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static을 제거하고 사용해도 좋을 것 같아요! getDeclaredFields() 도 동일한 피드백입니다!

Suggested change
private static String isNull(boolean isNullable) {
if (!isNullable) {
return "NOT NULL";
}
return "";
}
private String isNull(boolean isNullable) {
if (!isNullable) {
return "NOT NULL";
}
return "";
}

Comment on lines +78 to +89
// email VARCHAR(50) NOT NULL,
Field emailField = Arrays.stream(getDeclaredFields(personClass))
.filter(x -> x.isAnnotationPresent(Column.class))
.filter(x -> !x.getAnnotation(Column.class).nullable())
.findFirst().get();
String email = emailField.getName();
String emailType = javaClassTypeToDbTypes.get(emailField.getType());
boolean isNullable = emailField.getAnnotation(Column.class).nullable();

String emialNullable = isNull(isNullable);

String emailCombi = String.format("%s %s %s", email, emailType, emialNullable);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구현해주신 여러 필드로 쿼리를 만들때 모두 아래와 같은 동일한 과정을 거치고 있는데요!
각각의 필드를 별도로 수행하지 않고 함께 추상화해서 기능을 수행할 수 있도록 개선해보면 좋을 것 같아요! 🙏

  1. 클래스의 모든 필드를 조회한다.
  2. 조회한 필드 중 @Column 이 선언된 필드를 조회한다.
  3. @Transient 를 포함하는 필드를 제외한다
  4. 필드의 이름, 타입, 옵션(nullalbe 여부 등) 정보를 조회한다.
  5. 쿼리를 만든다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants