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

[Starve + Kyu] Mission5 - XHR과 AJAX #138

Open
wants to merge 20 commits into
base: Jiwon-JJW
Choose a base branch
from

Conversation

Jiwon-JJW
Copy link

안녕하세요.

코드스쿼드 백엔드 과정 Starve입니다.

Kyu와 함께 짝 프로그래밍을 진행하며 미션 5를 완료하였습니다!
이번 코드도 잘 부탁드립니다!

배포 사이트

감사합니다!

@ksundong ksundong self-requested a review April 2, 2021 07:22
@ksundong ksundong self-assigned this Apr 2, 2021
Copy link
Member

@ksundong ksundong left a comment

Choose a reason for hiding this comment

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

안녕하세요! Starve & Kyu!! 리뷰어 Dion입니다.
5단계까지 정말 고생많으셨습니다. 재밌게 즐기셨나요?

웹 애플리케이션 리뷰

  • 회원가입이 되지 않네요. 수정이 되면 다시 리뷰해드리겠습니다.

코드 관련 리뷰

  • 대부분 깔끔하게 잘 작성해주셨습니다.
  • 다만, 추상클래스의 사용에 있어서 좀 더 주의를 요하는 부분이 있는 것 같네요.
  • NPE를 방어할 수 있는 형태로 코드를 작성해주는 습관을 들여주세요.

자세한 리뷰내용은 코멘트를 참고해주세요.
궁금한 점이나 도움이 필요한 부분, 이해가 되지 않는 부분 토론하고 싶은 부분은 코멘트 남겨주세요!
고생하셨습니다! 수정 요청 드리겠습니다~

Comment on lines +30 to +32
testImplementation('org.springframework.boot:spring-boot-starter-test') {
exclude group: 'org.junit.vintage', module: 'junit-vintage-engine'
}
Copy link
Member

Choose a reason for hiding this comment

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

Spring Boot 2.4 버전부터는 vintage engine이 기본적으로 포함되어있지 않아서
exclude 사용해주지 않으셔도 됩니다!

Copy link
Member

Choose a reason for hiding this comment

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

그리고 위에 이미 존재합니다.

Comment on lines +27 to +28
implementation group: 'io.springfox', name: 'springfox-swagger2', version: '2.9.2'
implementation group: 'io.springfox', name: 'springfox-swagger-ui', version: '2.9.2'
Copy link
Member

Choose a reason for hiding this comment

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

swagger를 사용하는 이유를 한 번 생각해보세요!

public class SwaggerConfig {

@Bean
public Docket apiV2() {
Copy link
Member

Choose a reason for hiding this comment

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

왜 v2인가요?

.version("2.0")
.build();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

통행금지 표시가 있네요!
codesquad-members-2021/java-chess#9 (comment)
여기 참고해서 수정해주세요!

Comment on lines +35 to +37
.termsOfServiceUrl("http://www-03.ibm.com/software/sla/sladb.nsf/sla/bm?Open")
.license("Apache License Version 2.0")
.licenseUrl("https://github.com/IBM-Bluemix/news-aggregator/blob/master/LICENSE")
Copy link
Member

Choose a reason for hiding this comment

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

라이선스까지 있군요?

Comment on lines +47 to 50
if (!result.isValid()) {
model.addAttribute("errorMessage", result.getErrorMessage());
return "user/login";
}
Copy link
Member

Choose a reason for hiding this comment

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

잘 처리해주신 것 같네요~


function addAnswer(e) {
console.log("Hello World!");
Copy link
Member

Choose a reason for hiding this comment

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

Hello~

$(".answer-write input[type=submit]").click(addAnswer);

function addAnswer(e) {
Copy link
Member

Choose a reason for hiding this comment

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

e는 뭐에요?

e.preventDefault();

var queryString = $(".answer-write").serialize();
Copy link
Member

Choose a reason for hiding this comment

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

ES6의 문법을 사용해봅시다~

Comment on lines +23 to +24
}
Copy link
Member

Choose a reason for hiding this comment

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

아무런 로직 처리도 없는건 좋지 않은 것 같네요!

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.

3 participants