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

[Spring MVC] 한상우 미션 제출합니다. #29

Open
wants to merge 2 commits into
base: sangu1026
Choose a base branch
from

Conversation

sangu1026
Copy link

리뷰어님 시험 치시느라 미션하시느라 너무 고생많으셨습니다! 👏👏👏

이번 미션은 양이 많지는 않았지만 ArgumentReslover, Intercepter등 새로운 개념들이 많이 나왔어서, 생각보다 하는데
시간이 꽤 걸렸던 것 같습니다. 그리고 아주 기본적인 토큰에 대한 내용이였어서, 저희가 나중에 실제로 서비스를 구현할때는
어떤 점에 주의를 기울여야하는지 예를들어 쿠키나 세션등의 만료 시간 설정 같은거는 어떻게 할건지에 대한 추가적인
공부가 필요한 것 같다는 것을 느꼈습니다.

미션에서 요구했던 사항들은 모두 구현했지만 조금 빈약해 보일 수 있다는 점 양해 부탁드립니다. 🙇
해커톤까지 열심히 해봅시당! 👍

Copy link

@gogo1414 gogo1414 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 상우님!
한 학기동안 고생 많으셨어요👍

저도 새로운 개념들을 배우면서 진행하느라 시간이 꽤 걸렸답니다!
이번 방학을 통해 쿠키, 세션에 대해서 더 알아갔으면 하는 좋은 시간이 됐으면 좋겠네요!

리뷰는 전반적으로 작성할게 없이 너무 잘 구현하신 것 같아요
그래서 같이 얘기해보고 싶은 주제에 대한 토론 거리를 한번 적어봤어요!
천천히 읽어보시고 답변 적어주시면 좋겠네요

다시 한번 고생 많으셨어요!

Comment on lines +30 to +33
if (member == null || !member.getRole().equals("ADMIN")) {
response.setStatus(401);
return false;
}

Choose a reason for hiding this comment

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

member가 존재하지 않는 것에 대해서도 같은 에러처리를 하셨네요! 혹시 이유가 따로 있을까요??

}

@Override
public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) throws Exception {

Choose a reason for hiding this comment

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

Suggested change
public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) throws Exception {
public Object resolveArgument(MethodParameter parameter,
ModelAndViewContainer mavContainer,
NativeWebRequest webRequest,
WebDataBinderFactory binderFactory) throws Exception {

받는 파라미터의 길이가 길고 개수가 4개나 되니 이렇게 수정하면 가독성이 더 좋지 않을까요??

Comment on lines +30 to +31
registry.addInterceptor(adminHandlerInterceptor)
.addPathPatterns("/admin");

Choose a reason for hiding this comment

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

저는 resolvers.add(loginMemberArgumentResolver);로만 코드를 작성했는데
.addPathPatterns("/admin"); 하게 될 경우 어떤 차이가 발생하는지 설명해주실 수 있을까요?

Choose a reason for hiding this comment

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

JwtTokenDecoder, JwtTokenEnoder, TokenExtractor 를 분리해서 각각 하는 역할을 다르게 객체를 생성하셨네요!

혹시 Util 이라는 단어에 대해서 아실까요??
https://kong-dev.tistory.com/229
위의 사이트를 참고해서 간단하게 얘기 드리자면
"utils 패키지 내에 있는 구현들은 최대한 단순하고, 변경가능성이 낮고, 또한 도메인 로직과는 관계가 없어야 한다." 가 핵심이 되겠네요!

하나로 구현하는건 어떨지 한번 같이 이야기 해보고 싶어서 리뷰 남깁니다!

Comment on lines 11 to 17
private MemberDao memberDao;
private JwtTokenProvider jwtTokenProvider;

public MemberService(MemberDao memberDao) {
public MemberService(MemberDao memberDao,JwtTokenProvider jwtTokenProvider) {
this.memberDao = memberDao;
this.jwtTokenProvider = jwtTokenProvider;
}

Choose a reason for hiding this comment

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

@Autowired 어노테이션을 사용해보면 어떨까요?

저도 해당 어노테이션을 사용해보면 좋을것 같다는 리뷰를 받아서 말씀드려요😊

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.

2 participants