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

[럿고] 체스- 스프링 실습 1단계 제출합니다. #16

Merged
merged 7 commits into from
Apr 23, 2020

Conversation

ksy90101
Copy link

잘 부탁드리겠습니다! 😀

Copy link

@ddaaac ddaaac left a comment

Choose a reason for hiding this comment

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

안녕하세요 럿고! 구현을 너무 잘해주셔서 피드백 할게 별로 없네요 💯 오히려 제가 더 많이 배웠습니다 :) 간단한 피드백 몇개 남겼으니 참고해 주세요. 다음 단계에도 좋은 코드 부탁드릴게요!
참고로, js 피드백은 제가 한 게 아니랍니다...

@@ -0,0 +1,49 @@
function move(moveInfo) {
$.ajax({
Copy link

Choose a reason for hiding this comment

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

단순히 ajax 요청을 위해서 jquery를 사용하기에는 너무 무겁다고 생각합니다. 내장된 XMLHttpRequest나 fetch를 공부해 보시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

여러가지에 대한 비동기 방식에 대해서 공부하는 시간이었습니다!
감사합니다!

dataType: 'json',
contentType: 'application/json; charset=utf-8',
success: renderPiece,
error: alertMessage
Copy link

Choose a reason for hiding this comment

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

람다식과 구조분해 할당을 사용하시면 더 간단하게 짤 수 있습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

ajax를 제거함으로써, 이 부분은 진행하지 않았습니다!

Comment on lines +19 to +23
const sourceNode = document.getElementById(fromTo[0]);
const targetNode = document.getElementById(fromTo[1]);

targetNode.innerText = sourceNode.innerText;
sourceNode.innerText = " ";
Copy link

Choose a reason for hiding this comment

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

구조분해 할당을 사용하시면 더 간단하게 짤 수 있습니다 :)

getMoveInfo();
}

init();
Copy link

Choose a reason for hiding this comment

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

getMoveInfo()init()이 필요할까요? window 객체의 onload를 공부해보시면 좋을 것 같네요 :)

}
}

public List<Piece> findAll(String gameId) {
Copy link

Choose a reason for hiding this comment

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

findAllPieces와 같은 네이밍을 사용하면 더 명확하게 하는 일을 파악할 수 있을 것 같습니다.

pstmt.setString(1, gameId);
pstmt.setString(2, position.getName());
ResultSet rs = pstmt.executeQuery();
rs.next();
Copy link

Choose a reason for hiding this comment

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

rs의 결과가 없으면 큰일나겠네요!

PreparedStatement pstmt = con.prepareStatement(query)) {
pstmt.setString(1, gameId);
ResultSet rs = pstmt.executeQuery();
rs.next();
Copy link

Choose a reason for hiding this comment

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

rs 결과가 없다면 큰일나겠네요!

.orElseThrow(() -> new IllegalArgumentException("존재 하지 않는 기물 입니다."));
}

public Piece create(Position position) {
Copy link

Choose a reason for hiding this comment

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

사용할 때 PieceFactory.of(symbol).create(position)으로만 사용하는데 of()create() 메서드를 합치는 건 어떨까요?

this.position = position;
}

public Column plus() {
Copy link

Choose a reason for hiding this comment

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

Enum의 ordinal()을 사용해보는 건 어떨까요?

}

public Map<String, String> getBoard(String gameId) {
return boardDAO.findAll(gameId)
Copy link

@ddaaac ddaaac Apr 22, 2020

Choose a reason for hiding this comment

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

리턴값을 Map<String, List <PieceDto>>로 바꾸면 어떨까요?

class PieceDto {
    private String name;
    private String symbol;
    ...
    public static List<PieceDto> listFrom(List<Piece>) {
    ...
    }
}

Copy link

@ddaaac ddaaac left a comment

Choose a reason for hiding this comment

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

어제 피드백에 빼먹은 부분이 있었네요 😭 죄송합니다!

<div id="e1" class="box">{{e1}}</div>
<div id="f1" class="box">{{f1}}</div>
<div id="g1" class="box">{{g1}}</div>
<div id="h1" class="box">{{h1}}</div>
Copy link

@ddaaac ddaaac Apr 22, 2020

Choose a reason for hiding this comment

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

위와 같이 넘겨주면 이 코드도

<#pieces>
<div id={{name}}> {{symbol}}</div>
</pieces>

와 같이 간단해지겠네요!

@gracefulBrown gracefulBrown merged commit 386f755 into woowacourse:ksy90101 Apr 23, 2020
ksy90101 added a commit to ksy90101/jwp-chess that referenced this pull request Apr 30, 2020
* Initial Commit

* refactor: DAO try - resource 적용

* refactor: BoardDAO.addPieces(), hasNotGameIn() 추가

* refactor: 방어적 복사 적용

* refactor: Name -> Symbol 변경, BoardFactory -> PiecesFactory 변경

* refactor: 세션 만료시간 추가

Co-authored-by: kouz <lastpuzly@gmail.com>
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

4 participants