-
Notifications
You must be signed in to change notification settings - Fork 28
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
[이노] step 5. XHR과 AJAX #136
base: eNoLJ
Are you sure you want to change the base?
Conversation
질문 생성 api를 테스트하기 위해 ApiAnswerController 클래스 생성
AnswerController 클래스에 deleteAnswer 메소드의 logger 내용이 맞지않아 수정
User 클래스의 password 멤버변수와 Question 클래스의 answers 멤버변수는 Json 내용에 포함시키지 않기 위해 @JsonIgnore 어노테이션 추가
답변하기 버튼 클릭 시 form 태그에 의한 요청이 아닌 이벤트를 걸어 post 요청을 할 수 있도록 이벤트 생성
AnswerService 클래스의 save 메소드를 api요청에 대한 응답으로 하기위해 return 타입을 void에서 Answer로 수정
답변하기에 요청 url을 api 요청 url로 변경
…eptionHandler class 로그인 하지 않은 유저가 로그인이 필요한 서비스를 요청 했을 때 401 상태코드를 내려주도록 HttpStatus.UNAUTHORIZED 설정
질문 삭제 기능에 Api를 사용하기 위해 url 수정
답변 삭제 클릭 시 api 요청을 통해 질문을 삭제할 수 있도록 기능 구현
질문 삭제 api를 위한 deleteAnswer 메소드 생성, 삭제 된 질문을 전달하기 위해 AnswerService 클래스의 delete 메소드 수정
답변 리스트를 오름차순으로 정렬하기 위해 @orderby 어노테이션에 id를 desc로 설정
댓글 수정, 삭제 후 그에 맞춰 댓글 수를 변경하기 위해 updateAnswerCount 메소드 생성
Entity 클래스의 update 메소드에서 불필요하게 getter 메소드를 사용한 부분 수정
… 부분 수정 @PathVariable 어노테이션에서 불필요하게 query string을 지정해준 부분 제거
에러 메세지들을 상수로 관리하기 위해 ErrorMessage enum 생성
User, Question, Answer class의 중복 코드를 제거하기 위해 AbstractEntity class 생성
entity 클래스들에서 AbstractEntity 클래스를 상속받아 중복 코드 제거, date 멤버변수의 명칭이 createDate, modifiedDate로 변경 됨에 따라 html, scripts 파일 수정
Swagger2 라이브러리를 이용해 API를 문서화 하기 위해 의존성을 build.gradle 파일에 추가, QnaApplication class에 적용
matchWriterOfAnswerList 메소드의 이름을 countMismatchAnswers로 변경, 이름에 맡게 개수를 반환하도록 변경
@sonypark |
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.
비비&이노 안녕하세요! 리뷰어 소니 입니다.😊
미션4에 이어 미션5도 리뷰를 하게 되었네요.
한 번 봤던 코드라 그런지 이해가 잘 되었습니다 ㅎㅎ
코드 깔끔하게 잘 짜셨네요.
이번 미션은 크게 고칠부분은 없었습니다.
간단히 의견 남겼으니 해당 부분 읽고 생각해보시면 좋을 것 같아요~
일단 Approve 드리고, 혹시 추가 질문이나 더 개선하고 싶은 부분 있으시면 코멘트 주세요~
의견 듣고 머지하겠습니다:)
public String getCreateDate() { | ||
return formatDate(this.createDate, "yyyy-MM-dd HH:mm"); | ||
} | ||
|
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.
"yyyy-MM-dd HH:mm"
이건 상수로 빼줘도 되겠네요~
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 enum ErrorMessage { | ||
|
||
USER_NOT_FOUND("해당 유저를 찾을 수 없습니다."), | ||
QUESTION_NOT_FOUND("해당 질문을 찾을 수 없습니다."), | ||
ANSWER_NOT_FOUND("해당 답변을 찾을 수 없습니다."), | ||
NEED_LOGIN("로그인이 필요한 서비스입니다."), | ||
ILLEGAL_USER("접근 권한이 없는 유저입니다."), | ||
DUPLICATED_ID("이미 사용중인 아이디입니다."), | ||
LOGIN_FAILED("아이디 또는 비밀번호가 틀립니다. 다시 로그인 해주세요."), | ||
WRONG_PASSWORD("기존 비밀번호가 일치하지 않습니다."); | ||
|
||
private final String errorMessage; | ||
|
||
ErrorMessage(String errorMessage) { | ||
this.errorMessage = errorMessage; | ||
} | ||
|
||
public String getErrorMessage() { | ||
return errorMessage; | ||
} | ||
|
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.
👍
@MappedSuperclass | ||
@EntityListeners(AuditingEntityListener.class) | ||
public class AbstractEntity { | ||
|
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.
일반적으로 AbstractEntity
보다는 BaseEntity
라는 네이밍을 사용하는 것 같아요~
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.
그렇군요 자바지기께서 하시는걸 마냥 따라했는데 이런 부분까지 알려주시다니 감사합니다ㅎㅎ
@JsonIgnore | ||
@Column(nullable = false) | ||
private String 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.
DTO를 사용하하면 @JsonIgnore
는 필요없겠죠?
엔티티에 이런 코드가 많아지면 유지보수가 어려워지기데요.
엔티티 대신 DTO를 사용해하는 걸 권장하는 이유이기도 합니다.
참고만 하고 넘어가주세요~
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.
이번에 소니께서 DTO를 적용하는걸 추천해주셔서 많은 걸 배웠습니다. 대충 역할이나 사용법에 대해 알고는 있어서 간단히 되지 않을까?? 생각했었는데 막상 실제로 해보니 디테일한 문제점들이 생기더라구요. 그래서 좀 더 생각해볼 수 있는 기회가 되었어요. 지금 말씀하신 @JsonIgnore
도 마찬가지로 해보지 않았다면 필요없는 이유도 몰랐을텐데 정말 감사합니다. 제 과제에도 dto를 적용해보고 싶어졌어요ㅎㅎ
@@ -29,26 +34,27 @@ public void update(Long id, Answer answer, User sessionUser) { | |||
this.answerRepository.save(targetAnswer); | |||
} |
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.
this 를 명시하신 이유가 있나요?
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.
네 잘하셨습니다. 구분을 위해 명시하는게 좋은 것 같습니다 👍
String.prototype.format = function() { | ||
var args = arguments; | ||
let args = arguments; |
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.
let을 사용하신 이유가 있나요?
const와 let의 차이는 무엇인가요?
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.
const는 한번 초기화를하면 다시 초기화를 하지 못하고 let은 가능하다 정도로 알고 있습니다. 저는 작년에 프로그래밍을 처음 접했을 때 자바스크립트로 시작을 했는데 동영상이나 다른 자료들을 보면 절대 변하면 안되는 것들을 const로 정의하고 그 외엔 let으로 정의하는 느낌?을 받아 그렇게 사용을 하고 있었습니다. 혹시 이게 아니라 변하지 않을 값을 먼저 const로 정의를 하고 나중에 필요해질 때 let으로 바꿔 사용하는게 좋은 습관인가요??
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.
네 맞습니다. const로 선언하면 일단 변경 불가능한 변수임이 명시되기 때문에 더 안정적인 코드가 됩니다.
기본적으로 const를 사용하고 let으로 변경이 필요한 순간 변경하는게 좋은 습관이라고 생각합니다.
let linkDeleteArticle = document.querySelectorAll('a.link-delete-article'); | ||
let qnaCommentCount = document.querySelector(".qna-comment-count strong"); | ||
qnaCommentCount.textContent = linkDeleteArticle.length; | ||
} | ||
|
||
function addAnswerEvent() { | ||
let answerWrite = document.querySelector('form.answer-write button[type=submit]'); |
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.
여기도 다 let을 사용하셨네요~
const를 쓰면 안될까요?
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.
위 코멘트로 설명이 되었다고 생각합니다!!
현재 비비와 짝코딩을 진행하고 있습니다. 원래 계획은 비비 과제 먼저 끝내고 제 미션을 진행하려 했습니다. 그런데 생각보다 진행이 빠르지 못해 비비 미션만 진행하는 것도 빠듯할 것 같아 진행하던 제 미션을 마무리 하여 pr을 보냅니다.
미션4에서 이번 미션5로 눈에 띄게 바뀐 것은
ErrorException enum을 만들어 에러 메세지를 상수로 관리하도록 했습니다. enum 사용을 결정한 이유는 GlobalExceptionHandler 클래스의 handleUserAccountException 메서드에서 메세지에 따라 보여줄 view를 달리할 필요가 있었습니다. 그리고 다른 에러 메세지들도 enum을 만들었기에 포함시켜 리팩토링 했습니다.
답변 생성, 수정, 삭제에 있어서 자바스크립트를 이용해 변경되는 부분만 로딩이 되도록 진행했습니다. 자바지기 영상에 나와있지 않은 부분도 몇가지 추가를 했습니다. 답변에 대한 권한이 없다면 경고창을 띄우고 로그인 페이지로 이동하도록 구현했으며 ajax를 이용해 답변이 생성될 때 전체 답변 개수를 표시하는 부분도 제대로 변경이 되도록 했습니다.
엔티티의 공통부분인 id, 생성일, 수정일을 AbstractEntity 클래스를 만들어 중복을 제거 했습니다.
Swagger 라이브러리를 사용하여 api 문서를 볼 수 있도록 설정 했습니다.
이번 리뷰도 잘 부탁드려요!!
좋은 하루 되세요ㅎㅎ
배포링크 : https://enolj-spring-qna.herokuapp.com