-
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 #142
base: min27604
Are you sure you want to change the base?
Conversation
…hout logging in using exception
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.
안녕하세요! 새리! 리뷰어 Dion입니다!
미션 5단계를 열심히 수행해주셨군요? 고생많으셨습니다.
자바 외의 문법을 봐야해서 헷갈리기도 했고 이전에 썼던 코드들을 뜯어고쳐야하나 남겨둬야하나 고민이 유독 많았습니다. 그래서 개인적으로 다른 미션들보다 좀 더 애를 먹었던 것 같습니다.
자바스크립트는 자바와 유사한 형태지만, 확실히 이것 저것 새롭게 알아가는 부분이 많았을 것 같네요.
제가 금요일에 말씀드렸던 바와 같이, 백엔드 개발자라도 어느정도 알아야 하는 부분이기에, 주로 사용되는 문법에 대해서는 학습하시는 것을 추천합니다.
이전에 썼던 코드를 뜯어고쳐야 하나? 라는 의문은 계속해서 들어야 정상입니다. 아직 우리는 좋은 설계를 알지 못하기 때문에, 계속해서 시행착오를 거쳐가면서 좋은 설계를 지향해야 한다고 생각해요.
따라서, 아주 좋은 고민을 하셨다고 생각됩니다.
ajax를 사용하니 이전에 이미 처리해두었던 예외들을 어떻게 해야할 지 난감했습니다. ajax로 넘어가기만하면 alert를 띄우는 방식으로 하면 될 것 같았는데 값을 어떻게 전달해야하는지가 관건이었습니다. AnswerController의 create 메서드의 경우 반환타입이 Answer인데 로그인이 되지않은 유저의 경우 null을 반환시켜 ajax의 success 펑션 내에서 if(data == null) { alert("Login First"); } 코드를 시도해보았으나 ajax내에서 데이터타입을 json으로 설정해두어 parsererror가 발생했고 찾아보니 '데이터타입 설정부분을 삭제하면 된다'는 결과가 나왔으나 이 방법은 맞지 않는 것 같다고 생각했습니다. 결국 커스텀예외를 새로 하나 만들어 핸들러에서 Result값을 반환하게 했지만 원래 Answer를 반환타입으로 가지는 메서드여서 그런지 제대로 작동하지 않았습니다..
이 부분 코멘트에도 남기긴 했지만, 에러 상황일 때, 에러가 발생하도록 하는 것이 중요합니다.
예를 들어서 로그인이 되지 않아서 ajax 요청이 실패할 경우, success의 function을 타는 것이 아니라, error의 function을 타야합니다.
이런 에러의 구분은 HTTP 상태 코드를 통해 수행해주기 때문에, HTTP 상태 코드에 대해서 학습하시고, 스프링에선 어떻게 상태코드를 응답해주는지 학습해보세요!
그리고, 에러에 대한 응답은 정형화 해서 주는 것을 추천드리겠습니다. 정형화 한다는것을 쉽게 풀어드리자면, Result와 같은 클래스를 에러 응답용으로 정의해서 사용해주시면 좋을 것 같다는 의미입니다!
AnswerService의 deleteById()의 반환타입이 Result인데 메서드명을 바꾸는 게 좋을까요? 위에서 했던 것처럼 예외를 발생시켜 처리하면 좋을 것 같지만 커스텀예외가 너무 많이 늘어나는 건 아닌가싶어 Result를 반환타입으로 가지게 했으나 뭔가 제가 잘못하고있는 것 같다는 생각이 자꾸 들었어요ㅠㅡㅠ
Result
에 대한 의견 역시 코멘트에 남겨두었습니다~
커스텀 예외가 많으면 문제가 되는 부분이 있을까요? 저는 적절한 예외클래스를 정의하는 것에는 문제가 없다고 생각해요.
고민했던 부분들은 계속 이게 맞나? 하는 생각이 들었고 다른 방법으로 시도해도 이 방법이 과연 더 나은 방법인가? 질문이 꼬리에 꼬리를 물었습니다. 강의 동영상을 보며 따라하며 대강 요구사항은 충족시켰지만 제가 생각하는 '어느정도의 기능을 갖춘' 게시판을 만들고자 하니 사소한 데에 꽂혀 욕심을 부려 몇 시간을 날린 일도 있고 이전에 작성했던 코드들을 버리고싶지 않음(제 새끼 같아요..)과 새로운 걸 시도해보고싶은 마음이 만나 갈등도 있었습니다..
역시 좋은 고민을 계속해서 해주고 계신다고 생각됩니다. 그런 질문 사항들을 게속해서 정리해나가시다보면 분명 좋은 개발자가 되어계실거에요.
'어느정도의 기능을 갖춘' 게시판을 만들고자 하는 목표 아주 훌륭합니다. 👍👍👍 응원할게요!
사실 그 몇 시간을 날린게 흔히 말하는 '삽질'인데요, 저는 '삽질'하는 경험이 제일 중요하다고 생각합니다.
이전에 작성했던 코드는 대부분 지금 작성하는 코드보다 품질이 나쁘기 마련이라, 저는 과감히 버리는 과정도 필요하다고 생각합니다. (물론 실무에서는 둘 다 해보고, 차차 바꿔나가는 편이 좋겠죠.)
웹 애플리케이션 리뷰
- 중복 가입은 되지 않는데, 404응답을 주는 것은 조금 이상하다고 생각돼요.
- 중복 가입은 되지않았는데, id는 증가했네요.
- 빈 정보를 입력받을 수 있는데, 이를 어떻게 처리할 수 있는지, 알아만 보세요! 팀 프로젝트에서 적용해봅시다!
- 특정 url이 RESTful하지 않은 것 같아요. (회원가입시
/user/form
과 같이...) 한 번 체크해주세요! - 게시글이 역순정렬되어 출력되는거 되게 좋네요 👍
- 댓글 작성/삭제시 댓글 수는 변하지 않네요.
- 4단계 요구사항 중 하나였던 글 삭제시, 다른 사람의 댓글이 있는 경우 삭제되지 않아야한다는 요구사항이 지켜지지 않았네요!
코드 관련 리뷰
- 전체적으로 깔끔하게 잘 작성해주셨어요.
- 다만, 상속할 때, equals, hashCode의 적용은 더 생각해 볼 필요가 있을 것 같아요.
- early return을 사용하기 전에, boolean type 리턴이라면 if - else 문을 사용하기 전에, 결과를 바로 리턴하면 안되는지 생각해보세요!
대부분 깔끔했고, 요구사항이 충족되지 않은 부분들이 있어서 이 부분에 대한 개선만 해주시면 좋을 것 같습니다.
미션 수행하시느라 고생많으셨어요! 👍 👍
궁금하시거나, 도움이 필요한 부분, 이해가 되지 않는 부분, 토론하고 싶은 부분은 언제든 코멘트 남겨주세요!
// swagger 2 | ||
compile group: 'io.springfox', name: 'springfox-swagger2', version: '2.9.2' | ||
compile group: 'io.springfox', name: 'springfox-swagger-ui', version: '2.9.2' |
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.
주석으로 어떤 의존성인지 파악할 수 있도록 해주신거 좋습니다.
이런 질문을 드릴 수 있을 것 같아요.
compile
과implementation
의 차이는 무엇인지 한 번 공부해보시겠어요?- Swagger는 REST API를 문서화하는 도구인데 이런 문서화도구는 왜 사용할까요?
@@ -23,4 +25,9 @@ public String handleUserNotFoundException() { | |||
public String handleQuestionNotFoundException() { | |||
return "redirect:/"; | |||
} | |||
|
|||
@ExceptionHandler(NotLoggedInException.class) | |||
public Result handleNotLoggedInException() { |
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.
저는 가급적이면 예외를 인자로 받아서 이를 통해서 예외 정보를 포함해 핸들링 해주는 것을 선호합니다.
|
||
@Bean | ||
public Docket newsApi () { |
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 Docket newsApi () { | |
public Docket newsApi() { |
newsApi
라는 이름은 어떤 이유에서 지어주셨나요?
@@ -23,25 +22,23 @@ public AnswerController(AnswerService answerService) { | |||
} | |||
|
|||
@PostMapping | |||
public String create(@PathVariable Long questionId, String contents, HttpSession session) { | |||
public Answer create(@PathVariable Long questionId, String contents, HttpSession session) { |
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.
추가로 학습하실 거리를 제공해드릴게요.
REST API에서는 생성 성공시에 201 Created 응답을 반환해주곤 하는데요.
이를 어떻게 하면 Spring Applicaiton 상에서 제공해줄 수 있을까요?
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.
자주 사용될 법한 응답코드에 대해서도 알아두시는 것을 추천드립니다!
} | ||
|
||
@DeleteMapping("/{id}") | ||
public String delete(@PathVariable Long questionId, @PathVariable Long id, HttpSession session) { | ||
public Result delete(@PathVariable Long questionId, @PathVariable Long id, HttpSession session) { |
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.
REST API에서는 삭제등을 성공했을 때, 별 다른 응답을 줄 필요가 없는경우
204 No Content 응답을 줍니다! 이는 성공했으나 Body에 아무런 내용도 포함하지 않는다는 의미에요!
그렇게 되면 Result
는 굳이 필요없어지고, 예외가 발생했을 경우엔 예외에 해당하는 상태코드를 반환하므로써, 클라이언트는 상태코드를 보고 성공/실패 여부를 파악하고, 별도의 분기처리를 해주지 않아도 됩니다.
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가 여러 개인 경우 answerId라고 특정지어주는것을 좋아합니다.
var queryString = $(".answer-write").serialize(); | ||
var url = $(".answer-write").attr("action"); |
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.
ES6 문법을 학습해보시고, var
를 let
또는 const
로 고쳐보세요~
function onError(request, error) { | ||
alert("Login First"); | ||
} |
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.
로그인이 실패할 때만, 에러가 발생할까요?
function onSuccess(data) { | ||
console.log(data); | ||
var answerTemplate = $("#answerTemplate").html(); | ||
var template = answerTemplate.format(data.writer.name, data.formattedCreatedDate, data.contents, data.question.id, data.id); | ||
$(".qna-comment-slipp-articles").prepend(template); | ||
$("textarea[name=contents]").val(""); | ||
} |
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.
함수 추출 좋네요!
function deleteAnswer(e) { | ||
e.preventDefault(); | ||
|
||
var deleteBtn = $(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.
저는 변수명에는 축약어 사용은 추천하지 않는 입장이에요!
if (data.valid) { | ||
deleteBtn.closest("article").remove(); | ||
} else { | ||
alert(data.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.
Result라는 구조가 등장하면서 if - else가 등장할 수밖에 없게 되었죠? 이건 어떻게 해결할 수 있을까요?
안녕하세요, 새리입니다.
배포링크: https://sallys-qna.herokuapp.com/
요구사항에 맞게 에이젝스를 활용해 답변 추가와 삭제기능을 구현하고 도메인 클래스의 중복을 줄이기 위한 AbstractEntity 클래스를 만들어 각 도메인이 상속받게 했습니다. 또 Swagger 라이브러리를 통해 웹 API를 문서화하고 관리할 수 있게 했습니다.
답변 수 보여주기 기능같은 경우에는 이전 미션을 할 때 강의 동영상과는 다른 방식으로 구현해놓았던 부분이라 그대로 뒀습니다.
자바 외의 문법을 봐야해서 헷갈리기도 했고 이전에 썼던 코드들을 뜯어고쳐야하나 남겨둬야하나 고민이 유독 많았습니다. 그래서 개인적으로 다른 미션들보다 좀 더 애를 먹었던 것 같습니다.
if(data == null) { alert("Login First"); }
코드를 시도해보았으나 ajax내에서 데이터타입을 json으로 설정해두어 parsererror가 발생했고 찾아보니 '데이터타입 설정부분을 삭제하면 된다'는 결과가 나왔으나 이 방법은 맞지 않는 것 같다고 생각했습니다. 결국 커스텀예외를 새로 하나 만들어 핸들러에서 Result값을 반환하게 했지만 원래 Answer를 반환타입으로 가지는 메서드여서 그런지 제대로 작동하지 않았습니다..deleteById()
의 반환타입이 Result인데 메서드명을 바꾸는 게 좋을까요? 위에서 했던 것처럼 예외를 발생시켜 처리하면 좋을 것 같지만 커스텀예외가 너무 많이 늘어나는 건 아닌가싶어 Result를 반환타입으로 가지게 했으나 뭔가 제가 잘못하고있는 것 같다는 생각이 자꾸 들었어요ㅠㅡㅠ고민했던 부분들은 계속 이게 맞나? 하는 생각이 들었고 다른 방법으로 시도해도 이 방법이 과연 더 나은 방법인가? 질문이 꼬리에 꼬리를 물었습니다. 강의 동영상을 보며 따라하며 대강 요구사항은 충족시켰지만 제가 생각하는 '어느정도의 기능을 갖춘' 게시판을 만들고자 하니 사소한 데에 꽂혀 욕심을 부려 몇 시간을 날린 일도 있고 이전에 작성했던 코드들을 버리고싶지 않음(제 새끼 같아요..)과 새로운 걸 시도해보고싶은 마음이 만나 갈등도 있었습니다..
아무쪼록 시간내서 리뷰해주셔서 감사드립니다!