-
Notifications
You must be signed in to change notification settings - Fork 3
[4단계 - DB 적용] 이유빈 미션 제출합니다 #11
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
base: leeshin20
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,17 @@ | |||
| package chess.utils; | |||
|
|
|||
| import io.github.cdimascio.dotenv.Dotenv; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 라이브러리를 선택한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DB 비밀번호 같은 민감한 정보를 Dotenv로 관리하는 것이 좋다고 알고 있었기에 선택했습니다.
해당 경우에 다른 선택지가 있는지는 자세히 알아보지는 않았습니다
| private static final String URL = dotenv.get("DB_URL"); | ||
| private static final String USER = dotenv.get("DB_USER"); | ||
| private static final String PASSWORD = dotenv.get("DB_PASSWORD"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
라이브러리 사용 이유와는 별개로 DB 정보를 숨긴 것은 너무 좋네요 👍👍
질문
-
dotenv.get 은 해당 키값이 없을 경우 null 이 저장되나요?
-
만약 dotenv 라이브러리가 지원이 멈춰서, 혹은 취약점이 발견되어서 다른 라이브러리로 변경된다면 이 코드는 변경에 대응하기 쉬운 코드일까요?
-
협업하는 사람들에게 dotenv 파일은 어떻게 전달해줄 수 있나요?
| distributionBase=GRADLE_USER_HOME | ||
| distributionPath=wrapper/dists | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-6.8.1-bin.zip | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-7.2-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gradle 버전을 업데이트 한 이유는 뭔가요?
| public class DatabaseUtilTest { | ||
| @DisplayName("데이터베이스에 성공적으로 연결되는지 테스트") | ||
| @Test | ||
| void testDatabaseConnection() { | ||
| try (Connection connection = DatabaseUtil.getConnection()) { | ||
| assertNotNull(connection, "연결이 null일 수 없습니다"); | ||
| assertFalse(connection.isClosed(), "연결이 닫혀 있습니다"); | ||
| } catch (SQLException e) { | ||
| fail("연결 실패 : " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github action이나 jenkins로 자동 배포 프로세스가 구축되어 있다고 할 때, ./gradlew build 와 같은 명령어로 jar 파일을 build하면 빌드 전 테스트가 실행됩니다.
만약 테스트가 실패하면 빌드가 실패하는데, github action에서 제공하는 가상 머신이나 jenkins 가 설치된 가상 머신에 dotenv 파일이 없으면 테스트가 실패할 것 같아요.
이런 경우 어떻게 대처할 수 있을까요?
| Piece piece; | ||
| int index = 0; | ||
| for (int rank = Position.LAST_RANK; rank >= Position.FIRST_RANK; rank--) { | ||
| for (int file = Position.FIRST_FILE; file <= Position.LAST_FILE; file++) { | ||
| char symbol = boardState.charAt(index++); | ||
| piece = PieceSymbol.convertSymbolToPiece(symbol); | ||
| Position position = new Position(file, rank); | ||
| board.put(position, piece); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Piece를 for문 안에서 사용하는데 미리 선언한 이유가 있나요?
- index는 무엇을 의미하나요? file과 rank를 조합한 값 같은데 굳이 하나의 변수가 더 필요한가요?
- DB에서 불러온 값을 온전히 믿을 수 있나요? 혹시 모든 DB의 값이 QQQQQQQQQQQ.. 처럼 되어있으면 서버에서는 어떤 행동을 취하는 게 옳을까요?
| public Team getTurnByBinary(int n) { | ||
| if (n == 0) return Team.BLACK; | ||
| return Team.WHITE; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- google style guide에서는 한줄의 문법이어도 중괄호를 사용하기를 권합니다.
- n값이 -7, 6 처럼 이상한 값일때 검증이 필요하지 않을까요?
| public void createNewRoom() { | ||
| String query = "INSERT INTO game (boardState, turn, status) VALUES (?, ?, ?)"; | ||
| Board board = new Board(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 데이터베이스와 직접 소통하는 기능에 대한 테스트는 왜 없나요?
어떻게 테스트할 수 있을까요?
| } | ||
|
|
||
| public String convertBoardStateToString() { | ||
| StringBuilder stringBuilder = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringBuilder 사용 👍👍
String과 어떤 차이가 있나요?
| } | ||
|
|
||
|
|
||
| public void loadBoardState(String boardState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Board는 현재 실직적인 체스의 비즈니스 로직(기물들의 움직임 규칙, 게임 턴 관리...)을 담고 있는 객체로 보이는데, 보드의 상태를 문자열로 받아서 초기화 하는 기능이 여기에 있어도 괜찮을까요?
지금 형태라면 비즈니스 로직과 상관없이 DB에 저장하는 형태가 변경되는데도 (ex. 각 기물들이 기물 테이블에 개별 행으로 저장됨) 비즈니스 로직을 담당하고 있는 Board 객체도 같이 변경이 일어나야할 것 같아요.
| } | ||
|
|
||
| public int selectRoom(int gameId) { | ||
| String query = "SELECT status FROM game WHERE gameId = ?"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 보드 상태를 하나의 문자열로 쭉 이어서 저장하는 방법이 너무 참신하네요ㅋㅋㅋ
그런데 하나의 컬럼이 체스판 전체 데이터를 저장하고 있는 건 데이터베이스의 제 1 정규화 규칙을 어긴 것 같기도 한데 어떻게 생각하나요?
이 방식에는 다른 방식과 비교했을 때 어떤 장단점이 있을까요?
No description provided.