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

[쿠킴] 스프링 카페 1단계 - 회원 가입 및 목록 기능 #38

Merged
merged 7 commits into from Mar 8, 2022

Conversation

ku-kim
Copy link
Member

@ku-kim ku-kim commented Mar 7, 2022

안녕하세요~ 리뷰어님! 쿠킴 이라고 합니다.
스프링은 처음이라 어려웠지만 즐거운 미션이었습니다. Step1 요구사항 완료하여 리뷰 요청드립니다.
아직 부족한 점이 많지만 차근차근 발전시켜보겠습니다. 미리 감사드립니다.😆


URL convention
URL 기능 설명 Response Page Page Type
GET / List All posts 게시판 index.html index.html 정적
GET /users/form Get create form 회원가입 입력 포맷 (form.html) form.html 정적
POST /users create User 회원가입 redirect: /users 동적
GET /users List All Users 회원 목록 조회 /user/list.html 동적
GET /user/{userId} Get a User Profile 회원 profile 조회 /users/profile.html 동적
View 결과

form

/

/users/{userId}

단위 테스트 결과

UnitTest


부족한 점

  • 컨트롤러 테스트 X
    • 서비스, 리포지토리는 DI 주입이 아닌 직접 객체 생성을 하여 단위테스트를 했습니다. 직접 객체 생성을 통한 컨트롤러 단위 테스트로 구현하는데 어려움이 있었습니다. Mock 테스트는 현재 어려워 다음에 시도해보려 합니다.
  • 예외 페이지 관리 X
    • 서비스에서 중복 회원이면 예외를 던지지만 이 예외를 컨트롤러에서 처리하지 못했습니다.

- UserRepository 인터페이스 기준으로 메모리 기반 구현체 MemoryUserRepository 구현과 테스트 작성
- MemoryUserRepository 기반 데이터는 User 객체를 저장 Map 형태로 저장
- UserForm은 InputView의 입력 포맷을 맞춘 객체이다.
- UserService 구현
- 회원 가입 : UserService.join()
- 회원 전체 조회 : UserService.findUsers()
- 특정 회원 조회 : UserService.findUser()
- UserServiceTest에서 DCI 패턴으로 테스트 코드 작성
- 테스트 시 스프링 컨테이너 사용 X / 독립성을 위해 매 테스트 마다 리포지토리 초기화
- GET /users : 회원 목록 전체 조회
- GET /users/{userId} : 특정 회원 프로필 조회
- GET /users/form : 회원가입 폼
- POST /users : 회원가입
- rename : UserForm -> UserJoinRequest
- 사용자 요청 데이터이기 때문에 Form에서 JoinRequest로 이름 변경
- 기존 UserController에서 User 생성하여 UserService.join(User) 했자면 -> 컨트롤러는 UserJoinRequest 자체만 서비스에 넘겨주는 것으로 변경 (UserService.join(UserJoinRequest)
- 필요없는 세터 제거
- README.md 파일에 step1 내용 추가
@ku-kim ku-kim added the review-BE Improvements or additions to documentation label Mar 7, 2022
Comment on lines +46 to +50
@PostMapping
public String create(UserJoinRequest userJoinRequest) {
userService.join(userJoinRequest);
return "redirect:/users";
}
Copy link
Member Author

Choose a reason for hiding this comment

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

처음엔 User 객체 생성을 컨트롤러에서 생성하여 서비스로 넘겨주었지만, User는 도메인 객체라서 생성을 컨트롤러에서 하기 이상한 것 같아 UserJoinRequest만 서비스로 넘기고 User 객체 생성을 서비스로 옮겼습니다.
Q) 서비스에서 User 생성하는 것이 괜찮을까요?

변경 전

// UserController
    @PostMapping
    public String create(UserJoinRequest userJoinRequest) {
        User user = new User(userJoinRequest);
        userService.join(user);
        return "redirect:/users";
    }
    
// UserService
        public Long join(User user) {
        checkDuplicateUser(user);
        userRepository.save(user);
        return user.getId();
    }

변경 후

// UserController
    @PostMapping
    public String create(UserJoinRequest userJoinRequest) {
        userService.join(userJoinRequest);
        return "redirect:/users";
    }

// UserService
    public Long join(UserJoinRequest userJoinRequest) {
        User user = new User(userJoinRequest);
        checkDuplicateUser(user);
        userRepository.save(user);
        return user.getId();
    }

Copy link

Choose a reason for hiding this comment

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

넴 서비스에서 도메인을 생성하는게 더 좋아요~~! 좋습니다 💯
컨트롤러는 요청에 대한 모델만 처리하는 것이 더 좋아요.

@tmdgusya tmdgusya self-requested a review March 8, 2022 08:33
@tmdgusya tmdgusya self-assigned this Mar 8, 2022
Copy link

@tmdgusya tmdgusya left a comment

Choose a reason for hiding this comment

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

잘작성 하셨네요 ㅎㅎ LGTM~
몇가지 생각할거리 드립니다. 다음미션도 열심히 하시죠.

Comment on lines +46 to +50
@PostMapping
public String create(UserJoinRequest userJoinRequest) {
userService.join(userJoinRequest);
return "redirect:/users";
}
Copy link

Choose a reason for hiding this comment

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

넴 서비스에서 도메인을 생성하는게 더 좋아요~~! 좋습니다 💯
컨트롤러는 요청에 대한 모델만 처리하는 것이 더 좋아요.

Comment on lines +8 to +11
private final String userId;
private final String password;
private final String name;
private final String email;
Copy link

Choose a reason for hiding this comment

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

👍

@Repository
public class MemoryUserRepository implements UserRepository {

private static Map<Long, User> store = new HashMap<>();
Copy link

Choose a reason for hiding this comment

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

final 이여도 무방하지 않나요?

Copy link
Member Author

@ku-kim ku-kim Mar 8, 2022

Choose a reason for hiding this comment

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

final 이여도 무방합니다. 항상 불변 객체에 신경써야겠습니다. 감사해요

public class MemoryUserRepository implements UserRepository {

private static Map<Long, User> store = new HashMap<>();
private static long sequence = 0L;
Copy link

Choose a reason for hiding this comment

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

필요한 이유는 무엇일까요 ㅎㅎ?

Copy link
Member Author

@ku-kim ku-kim Mar 8, 2022

Choose a reason for hiding this comment

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

미래의 DB를 고려해서 테이블 인덱스의 autoincrement를 생각하고 만들었습니다만
하지만 요구사항을 벗어나고 기능적으로 꼭 필요하지 않다고 느껴집니다.
오버엔지니어링 하지 않아야겠습니다.!


private final UserRepository userRepository;

@Autowired
Copy link

Choose a reason for hiding this comment

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

이게 필요하지 않을 수도 있을거 같아요 ㅎㅎ
생성자가 한개인 경우 생략가능하거든요. 이부분에 대해서도 학습해보시죠 ㅎㅎ

Copy link
Member Author

@ku-kim ku-kim Mar 8, 2022

Choose a reason for hiding this comment

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

네 감사합니다 !
생성자 한개인 경우 생략 가능한점을 알고 있었으나 스프링이 처음이라 의도적인 연습과 익숙치 않은 제겐 @Autowired가 적혀있는게 가독성이 좋게 느껴졌었어요.

@tmdgusya (로치님) 께서는 생략하시는 편 이신가요?!



@DisplayName("MemoryUserRepository 클래스")
class MemoryUserRepositoryTest {
Copy link

Choose a reason for hiding this comment

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

테스트 점점 잘 작성하시네요 ㅎㅎ 💯

class Context_with_user {

@Test
@DisplayName("user 객체를 리포지토리에 저장하고 Id를 리턴한다.")
Copy link

Choose a reason for hiding this comment

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

저는 현업에서 요즘 @DisplayName 대신 함수에 한글이름을 적는데요.
사실 함수이름을 영어로 고민하는게 너무 시간아깝더라고요.
어차피 @DisplayName 이 본체인데 쿠킴님도 한번 고민해보시죠 ㅎㅎ 뭐가 더 좋은 방법같은지
꼭 제말이 정답은 아니죠, 답은 없습니다 ㅎㅎ.

Copy link
Member Author

@ku-kim ku-kim Mar 8, 2022

Choose a reason for hiding this comment

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

현업에서 @DisplayName 대신 테스트 함수 명을 한글로 적으시는군요!!
안그래도 저도 같은 고민을 했습니다.
말씀해주신것 처럼 @DisplayName과 테스트 함수 명이 겹치고, 작성에 시간이 상당해서요. :(
두둥! 현업 경험 알려주셔서 감사드려요.!!!

회사에서 테스트 작성 가이드라인 같은게 있는건가요?! ㅎㅎ

@tmdgusya tmdgusya merged commit 4079747 into codesquad-members-2022:ku-kim Mar 8, 2022
@ku-kim
Copy link
Member Author

ku-kim commented Mar 8, 2022

@tmdgusya (로치님) 감사합니다 :D
리뷰주신 사항 잊지않고 다음 단계에 적용하겠습니다. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants