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

[로또 - 웹, DB] 베디 미션 제출합니다. #43

Merged
merged 65 commits into from
Jun 24, 2019

Conversation

dpudpu
Copy link

@dpudpu dpudpu commented Jun 6, 2019

클린코드 나름대로 해보려고 노력했는데
스파크 자바는 처음 써봐서 어떻게 분리해야 좋을지 모르겠습니다.
부족한 부분 많은 지적 해주시면 감사하겠습니다.
resources 폴더에 테이블 생성 쿼리하고 jdbc.properties 있습니다.

@dpudpu dpudpu changed the title [로또] 베디 미션 제출합니다. [로또 - 웹, DB] 베디 미션 제출합니다. Jun 8, 2019
@dpudpu
Copy link
Author

dpudpu commented Jun 15, 2019

수정 목록

  • mvc (controller, service 분리)
  • try with resources
  • DBUtils, PropertiesUtil -> DBConnector, DataSource 이름 변경
  • 테스트용 DB h2 메모리로 변경
  • 새로운 환경에서 추가 작업 없이 앱 실행 바로 되게 변경 (필요한 테이블 자동 생성)
  • LottoHelper.class의 generate 역할들 각 도메인으로 위임 후 삭제
  • 추상화는 너무 일이 커질거 같아서 interface까지는 못하고 서비스용, 테스트용 db 다르게 실행할 수 있게까지만 했습니다.

service, controller 등 싱글턴

이 부분에서 가장 오래 고민했는데요.

public class LottoController {
    public static final LottoController INSTANCE = new LottoController(....);

이런 싱글턴은 의존 주입이 안되고

public class LottoController {
    public static final LottoController INSTANCE = null;
    
    public static LottoController getInstance(....){
        if(INSTANCE == null){
            INSTANCE = new LottoController(...);
        }
        return INSTANCE;
    }

이 방식은 서버환경에서는 싱글톤이 하나만 만들어지는 것을 보장하지 못한다고 해서 다른 방법을 생각했습니다.

얼마 전에 알게 된 스프링의 싱글턴 레지스트리 기법을 사용하고 싶어서 시도해봤습니다.
모든 생성자를 public으로 해주고 앱을 실행할 때 모든 인스턴스를 생성한 다음에
그 인스턴스만 사용하게 구현했는데요.
문제는 하나하나 다 입력 해줘야 하는 번거로움이 있어서 자동으로 생성되게 하고 싶지만
이건 아직 영역 밖인 거 같아서 다음에 한 번 시도해보겠습니다.

public static void main(String[] args) throws Exception {

        RoundDao roundDao = new RoundDao(dbConnector);
        LottoDao lottoDao = new LottoDao(dbConnector);

        RoundService roundService = new RoundService(roundDao);
        LottoService lottoService = new LottoService(lottoDao);

좋은 피드백 해주셔서 재미있게 고칠 수 있었습니다
감사합니다!

dpudpu added 2 commits June 16, 2019 15:00
db 조회 후에 값이 없을 경우 null을 리턴해줬는데
Optional을 사용해보고 싶어서 변경하였지만, 지금 상황에서는 에러처리 외에는 딱히 방법이 없는거 같아서
제대로 활용은 못했지만, 학습용으로 한 번 해봄.
@@ -12,6 +12,11 @@ dependencies {
compile('com.sparkjava:spark-core:2.9.0')
compile('com.sparkjava:spark-template-handlebars:2.7.1')
compile('ch.qos.logback:logback-classic:1.2.3')
// runtime('mysql:mysql-connector-java:8.0.16')

Choose a reason for hiding this comment

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

불필요한 주석은 제거해주세요.


DBConnector dbConnector = new DBConnector(DataSource.getTestInstance());

createTable(dbConnector);

Choose a reason for hiding this comment

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

오호라!
그런데 프로덕션에서도 h2를 사용하네요? 🤔

public class ErrorController {

public ErrorController() {
}

Choose a reason for hiding this comment

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

어라, 기본생성자를 작성한 이유가 있을까요?


public static DataSource getTestInstance() {
return LazyHolder.INSTANCE_TEST;
}

Choose a reason for hiding this comment

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

singleton pattern을 적용해줄때 기본생성자를 private으로 작성해서 객체 생성을 막아주세요.

https://blog.seotory.com/post/2016/03/java-singleton-pattern

}

public Connection getConnection() throws SQLException {

Choose a reason for hiding this comment

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

불필요한 공백 라인 제거

if (flag) {
return;
}
flag = true;

Choose a reason for hiding this comment

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

flag를 사용하기보다 singleton pattern을 적용하고 생성자에서 create메서드를 호출하는 것은 어떨까요


public WinPrize getAllByRound(final int round) throws SQLException {
Optional<WinPrize> winPrize = winPrizeDao.findAllByRound(round);
return winPrize.orElseThrow(SQLException::new);

Choose a reason for hiding this comment

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

Exception에 메시지를 담아주지 않아서 아래 부분에서 NPE가 발생하고 HttpStatus를 500으로 리턴합니다.

message = URLEncoder.encode(exception.getMessage(), "UTF-8");

public void printRateOfProfit(final Money money, final WinPrize winPrize) {
double m = winPrize.getRateOfProfit(money.value());
public void printRateOfProfit(final WinPrize winPrize) {
double m = winPrize.getRateOfProfit();

Choose a reason for hiding this comment

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

축약어 사용은 자제해주세요.


private static String message(final WinPrize winPrize, final Rank rank) {
if (rank == Rank.MISS) {
return "";

Choose a reason for hiding this comment

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

StringUtils.EMPTY 등 이미 있는 api를 사용해서 의미를 분명히 드러내주세요.


@Test
void getLatestTest() {
assertThat(-1).isNotEqualTo(roundDao.getLatest());

Choose a reason for hiding this comment

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

-1이 무슨 의미인지 모르겠네요

}

public int getLatest() {
String sql = "SELECT * FROM round ORDER BY id DESC LIMIT 1";

Choose a reason for hiding this comment

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

jdbc를 활용하거나 sql mapper 등을 사용하여 쿼리를 직접 입력할 때는 *는 지양해주세요.
스키마의 변경으로 예상치 못한 부분에서 에러가 발생할 수 있습니다.

}

public int getLatestRound() {
return roundDao.getLatest();

Choose a reason for hiding this comment

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

이렇게 하면 update문은 어떻게 결과 데이터를 리턴할 수 있을까요?
KeyHolder를 활용하여 해결할 수도 있어요

https://www.baeldung.com/spring-jdbc-autogenerated-keys

winningLottoDao.save(winningLotto, round);
}


Choose a reason for hiding this comment

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

불필요한 공백라인 제거!

winPrizeDao.save(winPrize, round);

Optional<WinPrize> expected = winPrizeDao.findAllByRound(0);
// System.out.println(expected.orElse(new WinPrize()).getTotalPrize());

Choose a reason for hiding this comment

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

불필요한 주석 제거!

}
}

public int getLatest() {

Choose a reason for hiding this comment

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

Optional을 사용할거면 다른 Dao의 시그니처도 맞춰주어야 합니다.

ps.setInt(4, winPrize.getWinCount(Rank.THIRD));
ps.setInt(5, winPrize.getWinCount(Rank.FOURTH));
ps.setInt(6, winPrize.getWinCount(Rank.FIFTH));
ps.setInt(7, winPrize.getWinCount(Rank.MISS));

Choose a reason for hiding this comment

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

저번에 잠시 보여드렸다시피 이런 부분들은 가변인자를 활용하여 해결할 수 있습니다.

	private void setParameter(PreparedStatement pstmt, Object... parameter) throws SQLException {
		for (int i = 0; i < parameter.length; i++) {
			pstmt.setObject(i + 1, parameter[i]);
		}
	}

Copy link

@woowahanCU woowahanCU left a comment

Choose a reason for hiding this comment

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

피드백 반영해서 깔끔하게 잘 구현하셨네요.
schema.sql 로딩한 부분은 재미있었습니다. 💯
몇가지 피드백 남겼으니 확인해보시고 궁금한 점 있어도 DM 남기지 마세요.
레벨 1과정 진행하느라, 고생 많았어요~ 😈

@woowahanCU woowahanCU merged commit ba30165 into woowacourse:dpudpu Jun 24, 2019
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