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단계 제출합니다 #110

Merged
merged 21 commits into from
Apr 19, 2020

Conversation

KimSeongGyu1
Copy link

@KimSeongGyu1 KimSeongGyu1 commented Apr 5, 2020

감사합니다.

Copy link

@young891221 young891221 left a comment

Choose a reason for hiding this comment

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

유안! 많은 변경없이 추가 사항을 잘 끝내셨네요ㅎㅎ
DM으로도 드렸지만 리뷰 늦어진 점 다시 한번 죄송합니다ㅠㅠ
1차 피드백 드렸으니 확인 부탁드립니다:)

} else {
document.getElementById("movableStart").value = element.id;
document.getElementById("movableCommand").value = "이동체크";
document.getElementById("movableForm").submit();

Choose a reason for hiding this comment

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

document.getElementById("movableForm").submit();
은 한번만 쓰도록 리펙토링 가능해 보이네요ㅎㅎ

document.getElementById("moveForm").submit();
} else {
document.getElementById("movableStart").value = element.id;
document.getElementById("movableCommand").value = "이동체크";

Choose a reason for hiding this comment

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

"이동", "이동체크"만 value로 묶어서 리펙토링 가능해 보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

추가로 말씀하셨던 moveStart에 대한 리펙토링은

검색할 때 한번 탐색하고
대입할 때 탐색할 수밖에 없다고 생각해 진행하지 않았습니다.

public static Map<String, Object> parseBlankBoard() {
return Position.getAllPositions()
.stream()
.collect(toMap(Position::toString, position -> "blank"));

Choose a reason for hiding this comment

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

"blank"를 모두 상수로 빼는것이 맞아보입니다.


// 드라이버 연결
try {
con = DriverManager.getConnection("jdbc:mysql://" + server + "/" + database + option, userName, password);

Choose a reason for hiding this comment

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

try resource방식으로 모두 변경해 주세요

System.out.println("정상적으로 연결되었습니다.");
} catch (SQLException e) {
System.err.println("연결 오류:" + e.getMessage());
e.printStackTrace();

Choose a reason for hiding this comment

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

e.printStackTrace();는 사용하지 말아주세요ㅎㅎ

Choose a reason for hiding this comment

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

불필요한 성능 하락을 유발합니다.


// 드라이버 연결
try {
con = DriverManager.getConnection("jdbc:mysql://" + server + "/" + database + option, userName, password);

Choose a reason for hiding this comment

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

String.format을 사용하는게 적절해 보입니다.

Comment on lines 21 to 25
String server = "localhost:13306"; // MySQL 서버 주소
String database = "db_name"; // MySQL DATABASE 이름
String option = "?useSSL=false&serverTimezone=UTC";
String userName = "root"; // MySQL 서버 아이디
String password = "root"; // MySQL 서버 비밀번호

Choose a reason for hiding this comment

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

해당 정보들은 필드 상수로 선언하는게 적절해 보입니다.

Copy link

@young891221 young891221 left a comment

Choose a reason for hiding this comment

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

리뷰 반영 잘 해주셨네요 💯
추가 개선점 피드백 드렸습니다. 확인 부탁드립니다:)

public void placePieceOn(Position position, Piece piece) throws SQLException {
String query = "INSERT INTO board (position, piece) VALUES (?, ?) ON DUPLICATE KEY UPDATE position=?, piece=?";

try (Connection con = DriverManager.getConnection(String.format("jdbc:mysql://%s/%s%s", SERVER, DATABASE, OPTION), USER_NAME, PASSWORD);

Choose a reason for hiding this comment

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

이거를 필드로 빼면 한번만 포맷팅해서 사용하면 될 듯 보입니다.

String.format("jdbc:mysql://%s/%s%s", SERVER, DATABASE, OPTION), USER_NAME, PASSWORD)

public Optional<Piece> findPieceOn(Position position) throws SQLException {
String query = "SELECT * FROM board WHERE position = ?";

try (Connection con = DriverManager.getConnection(String.format("jdbc:mysql://%s/%s%s", SERVER, DATABASE, OPTION), USER_NAME, PASSWORD);

Choose a reason for hiding this comment

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

아래와 같은 형식이 계속 반복되는 데요. 인터페이스를 열어두어 ...만 변경되고 공통된 형태는 재활용 하도록 리펙토링 부탁드립니다.

Connection con = getConnection();
PreparedStatement pstmt = con.prepareStatement(query);
...
pstmt.executeUpdate();
pstmt.close();
closeConnection(con);

Choose a reason for hiding this comment

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

다른 메서드들도 부탁드려요!

Copy link

@young891221 young891221 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다. 유안:)
머지할게요~!

@young891221 young891221 merged commit d53a81c into woowacourse:kimseonggyu1 Apr 19, 2020
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

2 participants