이런 반복되는 validation 로직을 User 로 모두 몰아넣을 방법은 없을까요? 파라미터가 올바른지를 사전에 검사하는건 User 의 책임일까요, UserLoginServiceImpl 의 책임일까요?

Java 8 스럽게 구현하려면 어떻게 해야 할까요?

결국 User에 값이 담기게 되므로 User에서 validation을 책임을 갖는 것이 맞을 것 같습니다.
User 생성자 혹은 setter 메소드에서 validation을 해야 될 것 같습니다.
이렇게 된다면 Exception 처리는 어떻게 해야 될지가 궁금합니다.
java 8 스럽게 구현한다면 Optional을 이용해서 orElseThrow로 Exception 처리를 할 수 있을 것 같습니다.

네 좋은 포인트입니다. 잘 짚어주셨어요. 그리고 말씀하신것 처럼 set 시점에 validation 을 하는게 좀 더 자연스럽겠네요.
사실 제가 Java 8 을 언급한 것은 저는 처음에는 이런 코드를 생각했었거든요.

interface User {
  // ...

  default void validate() throws ValidationException {
    if (this.getEmail().isEmpty()) { ... } 
   // ...
  }
}

다만 이렇게 구현할 경우 필드가 늘어날 때 마다 validate() 도 함께 수정해 줘야 하니 set 시점에서 validate 하도록 정책을 정하면 추후 변경 발생시에도 코드 변경이 훨씬 적을테니 말씀하신 접근법이 좀 더 낫겠네요.

그리고 예외는 ValidationException 처럼 공통 예외를 하나 두고, 그걸 상속하는 UserValidationException... 등으로 분화시킬 수 있을 것 같습니다. 그렇게 하면 최상위 예외 처리로직은 ValidationException 만 처리하게 할지, 그런 예외 중 UserValidationException 는 좀더 특별한 처리를 할 지 등의 전략을 결정할 수 있을 것 같아요.

User에서 validation 할 경우에는 Exception Test는 UserLoginServiceTest에서 하는게 아니라
따로 UserEntityTest 클래스를 만들어서 진행을 해야 되는 건가요?
login에서는 이미 완성된 User 객체가 넘어와서 상관없고, given 단계에서부터 테스트가 실패하네요..

UserLoginService 의 역할에 객체를 '만드는' 역할도 포함되어 있는지,
객체 만들라는 메시지를 받으면 객체를 만들어 주는 컴포넌트를 호출하는 역할인건지 한번 고민해 보시는게 좋겠어요

• 객체 생성 시점이 어디냐에 따라서는, validation 시점을 크게 앞당기거나 하는 전략도 가능합니다

final 으로 선언하지 않으시는 이유가 있나요? 그리고 이 정규식대로라면 [email protected] 같은 주소도 통과할 것 같은데 이런 경우는 어떻게 걸러낼 수 있을지 궁금합니다. 과연 rfc-822 를 모두 만족해야 할까요?

정규식 부분은 큰 생각 없이 구글링해왔습니다. 말씀하신 부분을 걸러내려면, 제한을 좀 더 걸어야 될 것 같습니다.
final은 일단은 무조건 선언해놓고, 가변적인 변수가 필요할 때에만 풀어주는 방식으로 코딩해야겠군요..

그렇다면 이런 경우 외부 출처를 documentation 으로 간략하게 남겨주시는게 좋겠어요.

파라미터는 가급적 final 로 선언해 주세요.

https://stackoverflow.com/questions/137868/using-the-final-modifier-whenever-applicable-in-java

spotbugs 돌려보시면 비슷한 경고 뜹니다.

이 클래스는 상속 가능한가요? 디자인의 이유를 알 수 있을까요? 기본 constructor 가 private 인 걸로 봐선 상속 못하게 하려고 이렇게 하신 것 같은데, 그렇다면 final class 가 맞지 않을까요?

인스턴스 생성을 막으려고 생성자를 private으로 정의했는데, 생각해보니 상속도 막아야 되겠군요...
final class도 해야 되는게 맞습니다. 이건 effective java에서도 본 내용으로 기억합니다.

Pattern.compile 이 내부적으로 어떤 일을 하나요? 메소드가 호출될 때 마다 매번 compile 을 호출해도 되나요?

파라미터(정규식)을 통해 Pattern 객체를 생성합니다.
매번 compile을 호출하면 pattern이 계속 생성되겠네요..
저건 싱글턴으로 처리되면 좋을 것 같습니다.

return matcher.matches() 로 구현해도 되지 않을까요?

네 return 값이 이미 boolean이니 if문은 쓸데 없네요..

여기도 final 이 아닌 이유가 궁금합니다.

final을 붙이는 습관을 들여야 겠습니다.

find 의 결과가 boolean 인것은 뭔가 이상하지 않나요?

isUserExists 로 메서드명을 바꾸거나, find의 리턴타입을 User로 바꿔야 할 것 같습니다.

findXXX 는 항상 boolean 을 반환하도록 코딩하실건가요? 그렇다면 진짜 도메인 객체를 찾는 로직은 어떤 이름을 붙이실 건가요?

사실 처음엔 User로 했다가, 구현 편의상 boolean으로 바꿨었는데
지금 생각해보니 너무 좋지 않은 생각이었네요. is000 형식으로 바꿔야 의도가 더 명확할 것 같습니다.

public RuntimeException(final String message)
public RuntimeException(final String message, final Throwable cause)
public RuntimeException(final Throwable cause)

세 개 constructor 는 각각 어떤 의미가 있나요?

LoginException 은 왜 Unchecked exception 으로 구현하신건가요? 의도가 궁금합니다.

여긴 스프링 의존성이 없지만, 개발할 때 @transactional이 기본적으로 RuntimeException일 경우에만 rollback 처리를 하기 때문에, RuntimeException으로 예외처리를 구현을 주로 하게 되었습니다.
또 클라이언트 코드에서 try catch를 선택해서 구현할 수 있다는 점도 한 몫 했습니다.
안그래도 예외처리 부분에서는 어떤 상황에서 CheckedException, UncheckedException을 써야 되는 건지 사실 감이 잘 안옵니다.

지금 단계에서는 스프링 프레임워크는 최대한 뒤로 생각하셔도 될 것 같아요. 그리고 Checked exception 으로도 @Transactional 로 롤백시킬 방법은 있습니다.

같은 이유로 @neropsys 님의 작업 #11 에도 제가 테이블 같은 요소는 최대한 나중에 생각하라고 가이드 드렸습니다.

그리고 Checked exception 은 예외가 발생하는 것이 확실한 상황일 때, 그리고 recover 가 필요한 경우 발생시키는게 좋고,

Unchecked exception 은 예외가 생길수도 안 생길수도 있는 상황일 때, 그리고 recover 가 때로는 불가능한 경우 발생기키는게 좋습니다.

가령 '파일시스템에 파일을 쓸 때' 같은 상황에서는 파일시스템에 접근할 수 없는 경우 Checked exception 인 IOException 을 발생시켜서, 저장 실패 이후의 상황을 클라이언트 코드가 제어할 수 있게 하는게 맞는 반면,

HTTP Request 에 사전에 정하지 않은 API Payload 가 들어옴으로 인해 더 이상의 진행이 불가능한 경우 IllegalArgumentException 을 발생시키는 식으로 씁니다.

https://www.baeldung.com/java-checked-unchecked-exceptions

그리고 이건 여담이지만 Java 의 Exception system 은 안전성에만 지나치게 집중하다 편의성을 크게 떨어뜨린 사례로,

Kotlin 에서는 기본적으로 모든 예외가 Unchecked exception 처럼 동작합니다.

UserEntity 를 immutable 로 구현한다면, 나중에 이메일이나 암호 변경같은 사양은 어떻게 구현하실 건가요?

이메일이나 암호가 변경된 UserEntity를 새로 생성해서 구현하면 될 것 같습니다.
이러면 생성비용이나 Heap Memory가 추가로 들 것 같긴 합니다.
하지만, mutable하게 구현하는 것도 변경가능성이 늘어나기 때문에 좋아 보이진 않습니다.
여긴 특히나 변경되면 안된다고 생각해서 final을 썼었네요...

하지만, mutable하게 구현하는 것도 변경가능성이 늘어나기 때문에 좋아 보이진 않습니다.

음, 좋은 포인트이긴 합니다. 하지만 그렇다고 해서 무조건 immutable 로 시스템 전체를 구성한다 같은 하드코어한 접근은 좋지 않습니다.

제가 여러분들께 처음 보여드린 방식이 그렇게 하드코어한 방식이었는데요, 적당한 시점에서 유연성을 생각하셔도 괜찮을 것 같아요. 오히려 new 키워드를 최대한 줄이는 게 차라리 더 낫습니다.

제가 저번에 설명드렸던, 'User + Documentation + LoginHistory' 처럼 세개 이상의 도메인 오브젝트가 필요한 비즈니스 요구사항 같은게 발생할 경우, 보통 이런 상황을 처리하기 위해 DDD 에서는 Root Aggregate 라는 개념을 도입하는데요,

만약 도메인 모델이 죄다 immutable 인 경우 Root Aggregate 를 변경해야 하는 요구사항이 발생하면 그 때는 거대한 객체인 Root Aggregate 를 매번 새로 생성해야 하는 상황이 생깁니다.

가령 위의 사례에서 저런 사용자 활동 보여주는 화면에서 사용자가 자기 정보를 빠르게 수정하고 싶다던가, '사용자 활동을 몇번 봤는지를 추가로 기록해 뱃지 시스템에 활용한다' 같은 요구사항이 생겨서 User 를 업데이트 해야 할 경우 비즈니스 로직에서 간단하게 수정하고 업데이트 하는 일이 갑자기 User 전체를 새로 만들어서 User mutator (제가 최초에 TDD 데모에서는 Service 가 이 역할을 했었죠) 에게 넘겨주고 그걸 다시 받아서 재조립 하고 ... 처럼 일이 쓸데없이 커지게 될 거에요.

어느 선에서 타협해야 할지 한번 잘 생각해 보시는게 좋겠습니다.

cc: @neropsys 님도 한번 이 이슈에 대해 찬찬히 생각해 보시는게 좋겠어요

Aggregate root 란 무엇인가?

setAttribute 의 실행 결과는 key 인가요, username 인가요? Documentation 을 명시해 주시면 좋겠네요.

아... 의도는 username이었는데, 구현 클래스에서 의도를 알 수가 없겠네요.

저장공간을 외부에서 주입 받을 수 있도록 한다면, 그게 반드시 HashMap 의 무언가일 필요는 없을 것 같네요.

그리고 database 변수의 visibility 가 default 인 의도가 궁금합니다.

네 주입해서 진행하는게 맞겠네요.
저긴 private static final 이어야 맞을 것 같습니다.

private 만으로도 충분할거에요.

저긴 private static final 이어야 맞을 것 같습니다.

오히려 요건 위험한 생각입니다. 왜 그럴지는 한번 곰곰히 생각해 보시죠. 자바의 신 책에서 static 키워드에 대해 뭐라고 설명하고 있던가요?

힌트: GC, GC Root, Memory leak

static은 static 영역에 메모리가 올라가기 때문에 GC 대상이 되지 않겠네요.
왜냐면 static 영역의 클래스, 변수들은 GC Root일 뿐더러 GC는 Heap 영역만 진행 되기 때문입니다.
결국 database 객체에 데이터가 추가되고 database 내의 사용하지 않는 데이터도
제거되지 않아 memory를 계속 잡아먹을 것이고, 이는 OutOfMemoryError를 발생시키는 원인이 될 것 같습니다.

그럼 데이터가 계속 증가할 수 있는 자료구조나 컬렉션에는 되도록이면 static 사용을 지양해야겠군요..

if - else 는 참 조건을 먼저 쓰고, 거짓 조건을 쓰는 편이 일반적으론 좀 더 읽기 좋은 코드입니다.

네, 이 경우에는 그런 것 같습니다.
만약 다음과 같이 거짓인 조건만 로직이 작성된다고 해도 참 조건을 제시하고, 뒤에 거짓 조건을 제시하는게 맞을까요?
이런 코드를 회사에서 가끔 보긴 했어서... 뭔가 보기 좋진 않았는데, 맞는 방식인지 궁금합니다.
if(password == null){
// do nothing
} else if (password != null){
// do something
}

아뇨, 보여주신 코드는 쓸데없는 비교를 두번 하는 나쁜 코드입니다. 말씀하신 시나리오처럼 null 인가? 아닌가? 같이 조건이 두개 뿐인 경우는 if - else 만으로도 충분하죠.

그리고 만약 kotlin 이었다면 이런 코드는 컴파일 에러가 날 겁니다.

val isUserVerified = if (password == null) {
  // ...
  /* return if */ true
} else if (password != null) {
  // ...
  /* return if */ false
}
  ^^^ isUserVerified 의 값을 결정하지 못한 decision path 가 존재

오히려 말씀하시는 상황은 early return pattern 을 쓰면 좀 더 좋긴 합니다.

Swift 에서는 early return 을 오히려 권장합니다.
Java 에서는 이런 식으로 구현하고요.

다만 함수형 패러다임에서는 early return 은 나쁜거라고 말하고 실제로 detekt 는 이 때문에 한 메소드 이내에 return 구문이 3개 이상이면 code smell 로 간주하고 있습니다.

이건 상황따라 잘 선택하면 될 문제라 생각해요. 저는 개인적으론 early return 은 논리학에서 말하는 소거법이랑 같다고 생각해서, '문제의 본질을 빨리 파악하고 싶다' 같은 상황에서는 아래처럼 Detekt 경고를 무시해버리고 early return 도 종종 씁니다.

@Suppress("ReturnCount") // 관련없는 정보 빠르게 소거해서 논리를 파악하기 위해 Early return 을 쓴다

https://detekt.github.io/detekt/style.html#returncount

Suppress 를 하실 때는 그 이유를 반드시 1줄 코멘트로 적어주시기 바랍니다.

네 알겠습니다.

이 test suite 이 이루고자 하는 목적을 좀 더 상세하게 적어주세요.

네 이것도 다음 작성 때 추가해놓겠습니다.

메소드 이름이 이미 좋기 때문에 이런 구문은 static import 로 바꾸면 UserTestUtils 라는 namespace 를 굳이 쓰지 않으셔도 괜찮을 것 같습니다.

네 static import를 적극 활용해야 겠네요.

테스트에는 given - when - then - expect 를 명시해 주시기 바랍니다.

참고로 우아한형제들 개발팀은 이런 식으로 일합니다.

https://woowabros.github.io/study/2018/03/01/spock-test.html

Nested class랑 method에 접미사로 추가하긴 했는데, method 내부에 주석으로 추가를 해야된다는 말씀이시군요.
테스트 코드 작성할 때 참고한 내용인데,

https://johngrib.github.io/wiki/junit5-nested/

이건 유스케이스 혹은 given when then 방식보다 메소드의 기능에 더 집중한 것처럼 보이는데요,
이런식으로 Describe-Context-it 방식의 진행방식은 어떻게 생각하시는지 궁금합니다.

그것도 나이스하죠. 사실 저는 TDD 처음 배울때 setup - replay - verify 로 배웠는데 해보니깐 given-when-then-expect 가 좀 더 자연스러워서 이쪽으로 바꾼 케이스입니다.

다만 두분이 어느 접근을 취할지 서로 이 문제에 관해 한번 토론해 보시고 한쪽 방향으로 일관성만 지켜주시면 좋겠어요.

오히려 포트폴리오 중복 같은 warning sign 을 줄일 수 있어서 전 그 방식도 나쁘지 않다고 봅니다.

DisplayName 으로 의도를 잘 설명하고 있는 것 같은데 이 주석 굳이 필요한지 궁금합니다.

좀 더 설명해야 된다고 생각해서 습관적으로 추가했네요.
이런 부분도 생각하면서 작업해야겠습니다.

이거 굳이 method 로 뺄 이유가 있을까요?

뺄 필요 없습니다. 바로 하면 되는 걸 작성할 때는 메소드로 분리해야된다고 생각했습니다.
코드를 작성할 때 이런 착오가 꽤나 발생하는 것 같네요...

관련 피드백
isMatched가 boolean이 아닌 nullable 값이었으면 불필요했을 것 같습니다

네 맞습니다. 이건 Optional로 처리하면 될 것 같네요..

이런 식으로 두 분이 의견 좀 더 활발하게 교환해 주시는게 좋겠습니다. 남한테 제대로 설명하려면 내가 우선 그걸 제대로 알아야 합니다.

그리고 그 과정에서 실력이 부쩍 늘고요.

isUserExists 로 메서드명을 바꾸거나, find의 리턴타입을 User로 바꿔야 할 것 같습니다.

결국 User에 값이 담기게 되므로 User에서 validation을 책임을 갖는 것이 맞을 것 같습니다.
User 생성자 혹은 setter 메소드에서 validation을 해야 될 것 같습니다.
이렇게 된다면 Exception 처리는 어떻게 해야 될지가 궁금합니다.
java 8 스럽게 구현한다면 Optional을 이용해서 orElseThrow로 Exception 처리를 할 수 있을 것 같습니다.

네 return 값이 이미 boolean이니 if문은 쓸데 없네요..

final을 붙이는 습관을 들여야 겠습니다.

파라미터(정규식)을 통해 Pattern 객체를 생성합니다.
매번 compile을 호출하면 pattern이 계속 생성되겠네요..
저건 싱글턴으로 처리되면 좋을 것 같습니다.

네 알겠습니다.

네 이것도 다음 작성 때 추가해놓겠습니다.

네 static import를 적극 활용해야 겠네요.