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

[4, 5단계 - 체스] 라이언(권영훈) 미션 제출합니다. #235

Merged
merged 130 commits into from Apr 12, 2021

Conversation

YounghoonKwon
Copy link

@YounghoonKwon YounghoonKwon commented Apr 2, 2021

안녕하세요! 게이츠!

코드의 품질보단 우선 작동하는 코드를 목표로 잡았고, TDD와 프로그래밍 원칙등을 지키지 못했어요 ㅠ
Controller부분 응답 처리를 어떻게 해야 할지 고민중이고 아직 수정중에 있고,
필수 요구사항만 구현 해놓은 상태입니다.!

방학중에 계속 리팩터링 하면서 수정 하겠습니다!

game_id는 "1"로 고정 DB에는 하나의 Row만 존재,
다른 체스판 상태를 저장하고 싶으면 기존의 것을 UPDATE로 해서 기존의 DB를 수정합니다.
url: http://localhost:4567/

hoony and others added 30 commits March 16, 2021 15:04
Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

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

라이언 안녕하세요!
필수 요구사항 구현 잘 해주셨네요 :)

몇가지 피드백 남겼어요. 확인 부탁드려요!
궁금한점 있으면 언제든 DM주세요!

@@ -14,6 +14,10 @@ dependencies {
compile('ch.qos.logback:logback-classic:1.2.3')
testCompile('org.junit.jupiter:junit-jupiter:5.6.0')
testCompile('org.assertj:assertj-core:3.15.0')
// https://mvnrepository.com/artifact/com.google.code.gson/gson
implementation group: 'com.google.code.gson', name: 'gson', version: '2.8.6'

Choose a reason for hiding this comment

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

이건 개인적인 궁금함인데요. Jackson이 아닌 Gson 라이브러리를 사용한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

Spring 에서는 Jackson을 기본으로 내장 하는 것으로 알고있는데 spark는 따로 언급 없이 spark documentation에 Gson으로 사용하길래, 따라했어요!

조금 찾아봤는데,
대용량 파일에서는 Jackson이 GSON보다 2배가량 빠르고,
작은 파일에서는 GSON이 Jackson보다 아주 약간? 빠르네요.

하지만, GSON은 maintenance가 더이상 안되는 것으로 보이며, moshi라는 것으로 대체되는 추세인것 같습니다.

google/gson#1821
https://www.overops.com/blog/the-ultimate-json-library-json-simple-vs-gson-vs-jackson-vs-json/

Choose a reason for hiding this comment

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

추가 학습까지 하시고 이렇게 공유까지 해주시다니 정말 멋지네요 👍

이런 피드백을 드린 이유는 어떤 기준을 가지고 라이브러리를 선택했는지가 궁금했어요!
그리고 저는 설령 그 기준이 틀렸다하더라도 그런 고민하는 시간이 의미있다고 생각하거든요. ^^

import java.sql.*;

public class ChessDAO {
public Connection getConnection() {

Choose a reason for hiding this comment

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

매 연결마다 DB Connection 객체를 생성하고 소멸시키는 비용을 줄이기 위해, Connection Pool 개념(캐시 개념)도 학습해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다!

try {
chessGame.move(chessGame.grid().piece(new Position(sourcePosition)),
chessGame.grid().piece(new Position(targetPosition)));
return 200;

Choose a reason for hiding this comment

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

해당 경우에 HTTP status code가 200일 것 같은데, 꼭 숫자 200을 리턴해야만 할까요?

Copy link
Author

Choose a reason for hiding this comment

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

상수로 랩핑 해서 수정 했는데, 이것이 게이츠님이 의도하신바가 맞나요??


import java.util.Objects;

public class Chess {

Choose a reason for hiding this comment

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

현재 Chess가 dao패키지에 있는데 DAO라고 볼 수 있을까요?

  • DAO: DB를 사용해 데이터를 조회하거나 조작하는 기능을 전담하는 오브젝트

Copy link
Author

Choose a reason for hiding this comment

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

entity 패키지로 분리했어요!

근데 entity를 보면서 다소 이해가 안되는 부분이,
DTO와의 정확한 차이가 잘 와닿지를 않아서요...
구지 DTO와 entity를 분리하여 사용하는 이유가 있을까요?

Choose a reason for hiding this comment

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

Entity(도메인 객체)는 비즈니스 로직을 가진 객체라고 이해하면 좋을 것 같고, DTO는 단지 데이터를 전달하는 객체라고 이해하면 좋을 것 같아요.

  • Entity : 애플리케이셔 내부 요구사항을 반영 (비즈니스 로직)
  • DTO : 애플리케이션 외부 요구사항을 반영 (UI)

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

class ChessDAOTest {

Choose a reason for hiding this comment

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

ChessDAOTest를 하려면 테스트 DB 연결이 필요할 것 같은데요.(Integration Test)
시간이 되시면 이 부분에 대한 학습도 해보시면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

이부분은 아직 감이 잘 안잡히는데,, 공부할 부분 키워드 좀 더 주실수 있을까요??

Copy link

Choose a reason for hiding this comment

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

네. 아래 링크 참조해 보시고 어떤 식으로 테스트하는지 컨셉만 보시면 좋을 것 같아요!
https://woowabros.github.io/experience/2019/11/06/db-unit.html

그리고 해당 피드백을 통해 제가 알려드리고 싶었던 건 테스트를 위한 테스트 DB를 별도로 만들어준다를 알려드리고 싶었어요.

Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

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

체스 마지막 미션까지 구현하시고 피드백 반영하시느라 고생 많으셨어요. 💯

Connection Property 역할을하는 객체를 생성해서 관리하는 부분 인상적이었어요.

제가 드렸던 피드백이 정답은 아니지만 많은 도움이 되었으면 좋겠네요 :)

그럼 남은 미션도 화이팅하세요 🥇
감사합니다. 🙇


private ConnectionProperty connectionProperty = new ConnectionProperty();

synchronized public static ChessConnection getInstance() {

Choose a reason for hiding this comment

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

Connection 객체를 싱글톤 패턴을 이용해서 1개만 만들어 두는 식으로 구현하셨네요.
이런 방법도 좋지만, Connection 객체를 미리 여러 개 만들어 두고(Connection Pool) 가져오도록 할 수는 없을까요?

이 부분은 이런 컨셉도 있구나 정도만 알아 두시면 좋을 것 같아요.

@@ -0,0 +1,42 @@
package chess.dao;

public final class ConnectionProperty {

Choose a reason for hiding this comment

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

Connection Property 역할을 하는 객체 생성 👍 👍 👍 👍

try {
chessGame.move(chessGame.grid().piece(new Position(sourcePosition)),
chessGame.grid().piece(new Position(targetPosition)));
return SUCCESSFUL_RESPONSE;

Choose a reason for hiding this comment

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

클라이언트 입장에서 HTTP 상태 코드 200과 별개로, 응답 값으로 200이 내려오면 이게 숫자 200을 의미하는지 상태를 의미하는지 헷갈릴 수도 있지 않을까?란 피드백이었어요. ^^

@serverwizard serverwizard merged commit d58948d into woowacourse:younghoonkwon Apr 12, 2021
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.

None yet

3 participants