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

[3주차 과제] 임강호 #53

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

limkangho
Copy link

이번 주에 어떤 작업을 했는지 설명해주세요.

회원가입, 로그인, DTO, 예외처리 구현


특히 어떤 부분을 리뷰받고 싶나요?

  1. 지난 강의에서 '좋은 코드를 작성하는 법'을 유의하여 각 메서드들이 최소한의 기능을 수행하고 어떤 기능을 수행하는지 쉽게 알아볼 수 있도록 메서드명을 작성해보았습니다. 이렇게까지 메서드를 나눠야하나? 너무 파일이 많아지기만 하는것 아닌가? 하는 생각이 들었는데 혹시 지금처럼 작성하면 괜찮게 만족하는 것인지, 아니면 메서드명을 더 명확하게 작성해야한다거나 기능을 더 분할해야한다거나 해야할 지 궁금합니다.

이번 주는 어떻게 학습했나요? 아래 질문에 짧게 답변주세요!

security config의 security filterchain 설정을 많이 건드려봤습니다. 메서드 중 Deprecated된 것들이 많아서 다른 분들은 해당 기능을 어떻게 구현하는지 궁금했는데, build.gradle 파일의 implementation에서 버전을 조절해주면 되는 것 같습니다.

이번 주에 학습에 투자한 시간

  • 15시간?

학습 하면서 좋았던 점과 아쉬웠던 점

  • 막히는 곳이 생기면 다른 분들이 PR하신 코드들을 참조하여 풀이하고있습니다. 어려운 부분은 다른 분들의 코드를 참조하여 어떻게 푸는지, 이렇게도 풀 수 있구나 찾아볼 수 있어서 도움이 많이 되고 있습니다.

어려움을 겪는 부분

  • 메서드 간 의존관계 파악하기: 코드량이 많아질 수록(제가 작성한 코드임에도 불구하고) 메소드 간의 의존 관계를 기억하는 것이 어려웠습니다. spring annotation이 어려워서 한참을 공부하다가 코드를 짜봤는데 의존 관계가 꼬여서 Spring Security 적용할 때 circular reference, dependency 상황이 발생하기도 했습니다.

스터디 개선되었으면 하는 점

  • 강의 내용을 저번처럼 녹화해서 올려주시면 한 번 더 복기하며 학습하기 좋을 것 같습니다. 가능하시다면 부탁을 드리고싶습니다!

@won983212
Copy link
Collaborator

전반적으로 잘 작성하신 것 같습니다. 사실 말씀해주신 부분은 취향이 어느 정도 반영되는 영역이라 맞다/틀리다를 논하기 어렵긴합니다...

아래 제 개인적인 생각을 적었는데, 참고만 해주세요.

String body = new String(cachingResponse.getContentAsByteArray());
Object data = objectMapper.readValue(body, Object.class);

if (body.contains("BAD_REQUEST") || !String.valueOf(response.getStatus()).startsWith("2")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

37~40과 42~45는 따로 private함수로 빼서 재사용이 가능한 로직으로 만들면 좋을 것 같습니다.

}

public String getUserPk(String token) {
return Jwts.parser().setSigningKey(secretKey).parseClaimsJws(token).getBody().getSubject();
Copy link
Collaborator

Choose a reason for hiding this comment

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

 Jwts.parser().setSigningKey(secretKey).parseClaimsJws(token)

이 부분도 다른 곳에서 공통적으로 사용되는 부분이고, 각 메서드별로 재정의의 여지가 거의 없는 부분이라 따로 메서드로 빼도 될 것 같습니다.

.compact();
}

public String createAccessToken(String userPK) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 AccessToken과 RefreshToken을 생성하는 메서드를 공통 추상화해서 묶을 수는 없을까요?

저였다면 AccessToken, RefreshToken별로 Validation이나 Generation로직을 포함한 클래스를 만들어서 추상화하여 사용했을 것 같습니다. 이렇게 하면...

  • 다른 종류의 Token이 생겨도 유연하게 대처
  • token생성, validation책임을 좀 더 작게 분활

물론 작은 규모의 프로젝트여서 굳이 이렇게까지 나눌 필요는 없을 것 같습니다. 취향 문제인 것 같아요.

chain.doFilter(wrappingRequest, wrappingResponse);
wrappingResponse.copyBodyToResponse();
} catch (CustomException e) {
HttpServletResponse errorResponse = (HttpServletResponse) response;
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분도 제 개인적인 취향인데 ㅎㅎㅎ...

저는 이 exception handling하는 부분을 따로 함수로 빼서 어떻게 handling하는지 설명하는 편입니다.

예를 들면 writeExceptionResponse와 같은 이름의 함수로 뺄 것 같습니다.

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