-
Notifications
You must be signed in to change notification settings - Fork 178
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
[Spring 체스 - 3단계] 춘식(안예장) 미션 제출합니다. #288
Conversation
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.
3단계 구현과 피드백 반영해주신 내용 잘 확인하였습니다 👍
소소한 피드백 남겨두었으니 확인 후 반영해주세요!
import java.sql.SQLException; | ||
import org.springframework.jdbc.core.RowMapper; | ||
|
||
public class RoomRequestMapper implements RowMapper<Room> { |
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.
RowMapper 분리 👍
src/main/java/chess/dao/MoveDao.java
Outdated
public void deleteAll() { | ||
String query = "delete from move"; | ||
public void deleteAllById(int id) { | ||
String query = "delete from move where room_id=" + id; |
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.
String을 직접 더해 로 쿼리를 만드는 것은 SQL Injection에 당할 위험이 있기 때문에 지양해보아요
@@ -19,6 +20,6 @@ public Player currentPlayer(PieceColor currentColor) { | |||
return players.stream() | |||
.filter(player -> player.isColor(currentColor)) | |||
.findAny() | |||
.orElseThrow(() -> new IllegalArgumentException("해당 색을 가진 플레이어가 존재하지 않습니다.")); | |||
.orElseThrow(() -> new InvalidPlayerException("해당 색을 가진 플레이어가 존재하지 않습니다.")); |
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.
CustomException으로 분리하였다면 예외처리 메시지를 Exception 내부로 옮겨보면 어떨까요?
|
||
private final String name; | ||
|
||
@ConstructorProperties({"name"}) |
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.
ConstructorProperties는 어떠한 이유로 작성하셨을까요!?
|
||
private final RoomDao roomDao; | ||
|
||
@Transactional(readOnly = true) |
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.
readOnly 👍
|
||
private final ChessService service; | ||
|
||
@GetMapping(value = {"/init"}) |
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.
아래 restart와 어떠한 차이가 있을까요?
|
||
private final RoomService service; | ||
|
||
@GetMapping("/") |
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.
@GetMapping("/") | |
@GetMapping("/rooms") |
해당 Path도 변경해야하지 않을까요!?
@@ -55,7 +80,7 @@ void getNewBoard() { | |||
void isEnd() { | |||
Boolean response = RestAssured.given().log().all() | |||
.accept(MediaType.APPLICATION_JSON_VALUE) | |||
.when().get("/board/status") | |||
.when().get("/rooms/1/board/status") |
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.
id가 1이라는 것을 하드코딩으로 보장할 수 있을지 고민해보아요
build.gradle
Outdated
@@ -14,7 +14,10 @@ repositories { | |||
|
|||
dependencies { | |||
|
|||
implementation('org.springframework.boot:spring-boot-starter-data-jpa:') | |||
implementation'org.projectlombok:lombok' |
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.
implementation'org.projectlombok:lombok' | |
implementation 'org.projectlombok:lombok' | |
annotationProcessor 'org.projectlombok:lombok' |
Gradle 5.x 이후 버전부터는 annotationProcessor 사용해주셔야 합니다!
안녕하세요 재연링!! 디엠으로 이것저것 많이 물어봤는데도 친절하게 답변 주셔서 감사합니다 😇 이번에도 잘 부탁드립니다!! 감사합니다 🙇 🙇 |
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.
수 많은 피드백 반영하시느라 고생 많으셨어요 👍
해당 미션은 머지하도록 하겠습니다!
다음 단계도 화이팅입니다 😄
학습 로그SQL injection
태그
@ConstructorProperties, Jacksonzied
태그
SQL auto increment
태그
Path Variable, Query Parameter
태그
|
반가워요 재연링!!
3단계를 진행하면서 REST와 스프링프레임워크에 대해 공부하면서 진행하다보니 피알이 조금 늦어버렸네요 😇
그래도 1,2 단계를 진행할 때보다 프레임워크 흐름이 이해되고 있는 것 같아요!!
또한 이동욱님의 '스프링 부트와 aws로 혼자 구현하는 웹 서비스'를 참고하며 리팩토링도 해보았습니다!!
jpa 관련 annotation도 써보고 싶었는데 아직은 어렵게 느껴져서 적용을 못해본게 아쉽네요ㅠ
이번 단계도 잘 부탁드립니다!! 따끔한 피드백은 언제나 환영이에요 💘 💘
(+) 1,2 단계가 머지되고 rebase를 깜박해서 3단계 구현을 마치고 체리픽을 하다가 1,2 단계 때 한 커밋들도 들어가게 되었습니다..
3단계는 a3ac28f 커밋부터 보시면 됩니다!!