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

[게시판 회원 기능] 베디 미션 제출 합니다. #63

Merged
merged 67 commits into from
Jul 24, 2019

Conversation

dpudpu
Copy link

@dpudpu dpudpu commented Jul 20, 2019

현구님 안녕하세요.
리뷰를 위해서 귀한 시간 내주셔서 감사합니다.

몇 가지 질문이 있습니다.

  1. 제가 TDD를 하려고 노력했는데, Repository, Entity 같은 경우에는 비즈니스 로직이 없으면 어떻게 해야 할까요? test 코드에서 builder나 javabean 테스트하면서 생성해주면 될까요? 아니면 프로덕션 코드 작성 후에 나중에 필요할 때 테스트를 작성하면 될까요?
  2. 각 컨트롤러의 URI를 상수처리 했는데, 이렇게 사용하는 것에 대해서 어떻게 생각하시나요?
  3. 테스트 코드 중복제거가 아주 어렵더라고요. 제거한다고 오히려 가독성이 더 떨어지는 거 같고, 혹시 조언 좀 받을 수 있을까요?

리뷰해주셔서 감사합니다.

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

안녕하세요 베디님 리뷰어 강현구입니다.
스프링을 사용해보신 경험이 있나보네요
능숙하게 잘 구현하셨어요 👍
몇 가지 피드백 남겼으니 확인해보시고 반영해보아요

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
src/main/java/techcourse/myblog/MyblogApplication.java Outdated Show resolved Hide resolved
WebTestClient webTestClient;

@Autowired
ArticleService articleService;

Choose a reason for hiding this comment

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

article service bean을 사용하는 방법도 있지만 게시글 추가 api를 호출하는 방법도 있어요. 이 테스트는 인수테스트니 api를 활용해서 게시글을 추가해보아요!

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다! 감사합니다. dm으로 자세하게 알려주셔서 해결할 수 있었습니다!


public UserDto.Response login(UserDto.Register userDto) {
UserDto.Response userResponseDto = new UserDto.Response();
User user = userRepository.findByEmailAndPassword(userDto.getEmail(), userDto.getPassword())

Choose a reason for hiding this comment

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

보통 db에 데이터를 저장할 땐 개인정보 문제로 암호화해서 저장하게 됩니다. 그렇다면 userDto.getPassword() 메서드로 쿼리 조회가 불가능하겠죠.
로그인은 user의 비즈니스 로직으로 보이는데요. authenticate 같은 메서드를 user에 추가해보는 건 어떨까요

Copy link
Author

@dpudpu dpudpu Jul 21, 2019

Choose a reason for hiding this comment

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

확실히 그러네요! 미처 그 부분까지는 생각 못했습니다. 감사합니다!
로그인은 user의 비즈니스 로직으로 보이는데요 이 말씀은 패스워드를 복호화 해주는 부분을 말씀하시는 걸까요?

아래처럼 변경 했습니다. 어떻게 생각하시나요?

authenticate()는 아직은 따로 로직은 없습니다.

    public UserDto.Response login(UserDto.Register userDto) {
        String decodedPassword = User.authenticate(userDto.getPassword());
        
        User user = userRepository.findByEmailAndPassword(userDto.getEmail(), decodedPassword)
                .orElseThrow(() -> new ValidUserException("존재하지 않는 이메일 또는 비밀번호가 틀립니다.", "password"));

        return UserDto.Response.createByUser(user);
    }

아니면
User.authenticate(userDto.getEmail(), userDto.getPassword()); 이런식으로 변경해줘야 할까요?

Copy link

@kang-hyungu kang-hyungu Jul 24, 2019

Choose a reason for hiding this comment

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

로그인이라는 기능이 User라는 객체가 처리해야할 기능으로 보여서 남긴 코멘트였어요
로그인 처리 절차를 보면 아이디와 암호가 맞는지 확인하는 절차가 필요하겠죠. 입력한 값과 db에서 조회한 user 객체의 아이디, 암호가 일치하는지 확인하는 내용을 코드로 작성하면 로그인 기능인 것이겠죠.
사용자를 인증하는 것과 암호화된 비밀번호를 복호화하는 건 다른 기능이에요. 로그인 과정 중에 암호화된 비밀번호를 복호화할 순 있겠지만요..

사실 저 코멘트의 의도는 쿼리에 의존적인 코드라서 수정해보자고 남긴 것이었어요.
쿼리로 아이디, 암호로 필터 조건을 걸게 되면 db에서 아이디로 비밀번호로 사용자 인증하는 user의 로그인 기능을 처리하는게 되니깐요.
db에서 데이터 조회와 로그인이라는 두 가지 관심사를 userRepository.findByEmailAndPassword 메서드가 처리하게 되는 문제가 생겨서 분리해보라는 뜻이었어요

@dpudpu
Copy link
Author

dpudpu commented Jul 22, 2019

현구님 리뷰 해주셔서 감사합니다.
피드백 해주신 부분들 수정하면서 몇가지 질문들 코멘트에 달았는데 확인해주시면 감사하겠습니다.
그리고 도메인에도 유효성 검사 추가했습니다. 이 부분 어떤지 말씀해주시면 감사하겠습니다.

감사합니다.

@kang-hyungu
Copy link

user 도메인 클래스에 유효성 검사 추가한 내용 확인했어요!
조금만 더 수정하면 좋을 것 같아요
static final로 선언한 유효성 검사용 상수들이 User 객체 전체에서 관심가져야 할 내용일까요?
각 validateXXX 메서드에서 알고 있으면 충분한 내용 아닐까요?

실제 user 객체를 이루는 속성보다 유효성 검사용 상수가 많아서 객체 파악할 때 헷갈릴 것 같아요

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

피드백 반영하신 부분 확인했어요!
그리고 질문 주신 내용들에 대해서 제 의견도 남겼어요
꼭 반영할 필요는 없으니 개발하실 때 참고만 하세요
추가로 수정하고 싶으신 부분은 다음 단계 진행하시면서 적용하셔도 충분해보이니 머지 처리할게요
코드 구현하느라 수고하셨어요!

@kang-hyungu kang-hyungu merged commit d2d0a75 into woowacourse:dpudpu Jul 24, 2019
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