이 글은 http://www.slipp.net/questions/153 에 있는 내용을 보고 리팩토링을 과정을 한 번 정리해본 것이다.
1. 테스트 코드 일부 리팩토링 및 버그 수정
테스트 대상은 ApiUsersController 클래스이다. 이 클래스를 테스트하는 코드는 아래와 같다.
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
@Mock
private SocialUserService userService;
@InjectMocks
private ApiUsersController dut = new ApiUsersController();
@Test
public void duplicateUserId_login_isSameUser() {
String userId = "userId";
SocialUser loginUser = new SocialUser(1L);
when(userService.findByUserId(userId)).thenReturn(loginUser);
String actual = dut.duplicateUserId(loginUser, userId);
assertThat(actual, is("false"));
}
@Test
public void duplicateUserId_login_isNotSameUser() {
String userId = "userId";
SocialUser loginUser = new SocialUser(1L);
when(userService.findByUserId(userId)).thenReturn(loginUser);
String actual = dut.duplicateUserId(new SocialUser(2L), userId);
assertThat(actual, is("true"));
}
@Test
public void duplicateEmail_doesnot_existed() {
String actual = dut.duplicateEmail(SocialUser.GUEST_USER, "userId", ProviderType.slipp);
assertThat(actual, is("false"));
}
@Test
public void duplicateEmail_login_isSameUser() {
String email = "email@slipp.net";
ProviderType providerType = ProviderType.slipp;
SocialUser loginUser = new SocialUser(1L);
when(userService.findByEmailAndProviderId(email, providerType)).thenReturn(loginUser);
String actual = dut.duplicateEmail(loginUser, email, providerType);
assertThat(actual, is("false"));
}
@Test
public void duplicateEmail_login_isNotSameUser() {
String email = "email@slipp.net";
ProviderType providerType = ProviderType.slipp;
SocialUser loginUser = new SocialUser(1L);
when(userService.findByEmailAndProviderId(email, providerType)).thenReturn(loginUser);
String actual = dut.duplicateEmail(loginUser, email, providerType);
assertThat(actual, is("false"));
}
}
다음의 두 가지가 걸린다.
- 상수처럼 쓰이는 것: "userId", "email@slipp.net", new SocialUser(1L), "false", "true"
- loginUser 변수의 의미가 두 메서드에서 다름
이 두 가지를 정리하기 위해 먼저 상수처럼 쓰이는 것들을 상수로 정리한다.
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
public static final String USER_ID = "userId";
public static final String EMAIL = "email@slipp.net";
public static final SocialUser USER1 = new SocialUser(1L);
public static final String FALSE = "false";
public static final String TRUE = "true";
@Mock
private SocialUserService userService;
@InjectMocks
private ApiUsersController dut = new ApiUsersController();
@Test
public void duplicateUserId_login_isSameUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
String actual = dut.duplicateUserId(USER1, USER_ID);
assertThat(actual, is(FALSE));
}
@Test
public void duplicateUserId_login_isNotSameUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
String actual = dut.duplicateUserId(new SocialUser(2L), USER_ID);
assertThat(actual, is(TRUE));
}
@Test
public void duplicateEmail_doesnot_existed() {
String actual = dut.duplicateEmail(SocialUser.GUEST_USER, USER_ID, ProviderType.slipp);
assertThat(actual, is(FALSE));
}
@Test
public void duplicateEmail_login_isSameUser() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
String actual = dut.duplicateEmail(USER1, EMAIL, ProviderType.slipp);
assertThat(actual, is(FALSE));
}
@Test
public void duplicateEmail_login_isNotSameUser() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
String actual = dut.duplicateEmail(USER1, EMAIL, ProviderType.slipp);
assertThat(actual, is(FALSE));
}
}
리팩토링 후, 테스트를 돌려보니 정상 동작한다.
음,, 근데, duplicateEmail_login_isSameUser() 메서드와 duplicateEmail_login_isNotSameUser() 메서드가 구현이 동일하다. 이를 올바르게 변경하기 위해 duplicateEmail_login_isNotSameUser()를 다음과 같이 변경한다.
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
...
public static final SocialUser USER2 = new SocialUser(2L);
@Test
public void duplicateUserId_login_isNotSameUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
String actual = dut.duplicateUserId(USER2, USER_ID);
assertThat(actual, is(TRUE));
}
...
@Test
public void duplicateEmail_login_isNotSameUser() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
String actual = dut.duplicateEmail(USER2, EMAIL, ProviderType.slipp);
assertThat(actual, is(TRUE));
}
}
2. 테스트 대상 코드
테스트 대상 클래스인 ApiUsersController 클래스는 아래와 같다.
@Controller
public class ApiUsersController {
private SocialUserService userService;
@RequestMapping("/duplicate_userid")
@ResponseBody
public String duplicateUserId(SocialUser loginUser, String userId) {
SocialUser socialUser = userService.findByUserId(userId);
if (socialUser == null) {
return "false";
}
if (socialUser.isSameUser(loginUser)) {
return "false";
}
return "true";
}
@RequestMapping("/duplicate_email")
@ResponseBody
public String duplicateEmail(SocialUser loginUser, String email, ProviderType providerType) {
SocialUser socialUser = userService.findByEmailAndProviderId(email, providerType);
if (socialUser == null) {
return "false";
}
if (socialUser.isSameUser(loginUser)) {
return "false";
}
return "true";
}
}
http://www.slipp.net/questions/153 링크의 글을 보면 위의 중복된 코드를 제거하기 위한 작업에서 시작된 것을 알 수 있다. 음.... 중복 코드 제거 작업 이전에 한 가지 먼저 처리하고 싶은 것이 있다. 그것은 ApiUsersController 로부터 중복 검사 로직을 분리하는 것이다. ApiUsersController 클래스는 두 가지 책임을 가지고 있다.
- 웹 요청 수신 및 처리 결과 응답
- 중복 검사 구현
중복 검사 구현이 변경되어도 바뀌고, 응답 방식이 바뀌어도 바뀐다. 즉, 단일 책임 원칙 위반(SRP; Single Responsibility Principle)이다. 분리하자. 이거 분리하려면 리팩토링을 몇 단계를 거쳐야 한다.
- 중복 검사 기능 제공하는 CheckDupService 클래스에 두 메서드를 복사하고,
- CheckDupService 클래스의 객체를 생성하고
- 그 객체에 위임하고
- ApiUsersControllerTest 통과하나 확인하고,
- CheckDupService 클래스를 CheckDupServiceImpl로 복사하고, CheckDupService를 인터페이스로 바꾸고,
- ApiUsersControllerTest 클래스로부터 CheckDupServceImpl에 대한 단위 테스트를 작성해서 동작확인하고,
- ApiUsersControllerTest 클래스는 CheckDupService 인터페이스를 Mock으로 하도록 바꾸고,
- 등등등
음, 글로 일일이 정리하면 글 쓰다가 토가 나올지 모르니, 일단 중간 과정의 코드를 보자.
3. ApiUsersController에서 중복 검사 기능 별도 클래스로 분리
중복 검사 기능을 별도 클래스로 분리하고, 그것이 정상적으로 동작하는지 확인한 과정까지의 진행 결과이다. 먼저 CheckDupService 클래스의 코드는 아래와 같다. 두 메서드의 리턴 타입을 String에서 boolean으로 변경했다.
// ApiUsersController의 코드 상당 부분 이동
public class CheckDupService {
private SocialUserService userService;
public CheckDupService(SocialUserService userService) {
this.userService = userService;
}
public boolean duplicateUserId(SocialUser loginUser, String userId) {
SocialUser socialUser = userService.findByUserId(userId);
if (socialUser == null)
return false;
return !socialUser.isSameUser(loginUser);
}
public boolean duplicateEmail(SocialUser loginUser, String email, ProviderType providerType) {
SocialUser socialUser = userService.findByEmailAndProviderId(email, providerType);
if (socialUser == null)
return false;
return !socialUser.isSameUser(loginUser);
}
}
ApiUsersController 클래스는 이제 CheckDupService를 사용하도록 바뀐다.
@Controller
public class ApiUsersController {
private CheckDupService checkDupService;
public ApiUsersController(CheckDupService checkDupService) {
this.checkDupService = checkDupService;
}
@RequestMapping("/duplicate_userid")
@ResponseBody
public String duplicateUserId(SocialUser loginUser, String userId) {
return Boolean.toString(checkDupService.duplicateUserId(loginUser, userId));
}
@RequestMapping("/duplicate_email")
@ResponseBody
public String duplicateEmail(SocialUser loginUser, String email, ProviderType providerType) {
return Boolean.toString(checkDupService.duplicateEmail(loginUser, email, providerType));
}
}
ApiUsersController 클래스는 이제 아래와 같이 ApiUsersController에 CheckDupService 객체를 전달하도록 바뀐다.
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
@Mock
private SocialUserService userService;
private ApiUsersController dut;
@Before
public void init() {
CheckDupService checkDupService = new CheckDupService(userService);
dut = new ApiUsersController(checkDupService);
}
// 테스트 메서드는 바뀌지 않음
@Test
public void duplicateUserId_login_isSameUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
String actual = dut.duplicateUserId(USER1, USER_ID);
assertThat(actual, is(FALSE));
}
테스트를 돌려보면 통과다. 다음 단계로 넘어가자.
4. CheckDupService를 인터페이스로 추출하기
CheckDupService 클래스에서 인터페이스를 추출하자.
public interface CheckDupService {
boolean duplicateUserId(SocialUser loginUser, String userId);
boolean duplicateEmail(SocialUser loginUser, String email, ProviderType providerType);
}
// 기존 CheckDupService 클래스를 CheckDupServiceImpl 클래스로 변경
public class CheckDupServiceImpl implements CheckDupService {
private SocialUserService userService;
public CheckDupServiceImpl(SocialUserService userService) {
this.userService = userService;
}
테스트 클래스는 이제 CheckDupServiceImpl 클래스를 이용해서 객체를 생성하도록 바뀐다.
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
...
@Before
public void init() {
CheckDupService checkDupService = new CheckDupServiceImpl(userService);
dut = new ApiUsersController(checkDupService);
}
5. CheckDupServiceImpl 클래스에 대한 단위 테스트 만들기
ApiUsersControllerTest 클래스는 사실상 CheckDupServiceImpl 클래스의 테스트에 가까워졌다. ApiUsersControllerTest 클래스를 복사해서 CheckDupServiceImpl 클래스에 대한 단위 테스트를 만들자.
@RunWith(value = MockitoJUnitRunner.class)
public class CheckDupServiceImplTest {
public static final String USER_ID = "userId";
public static final String EMAIL = "email@slipp.net";
public static final SocialUser USER1 = new SocialUser(1L);
public static final SocialUser USER2 = new SocialUser(2L);
@Mock
private SocialUserService userService;
private CheckDupService checkDupService;
@Before
public void init() {
checkDupService = new CheckDupServiceImpl(userService);
}
@Test
public void duplicateUserId_login_isSameUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
boolean actual = checkDupService.duplicateUserId(USER1, USER_ID);
assertThat(actual, is(false));
}
@Test
public void duplicateUserId_login_isNotSameUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
boolean actual = checkDupService.duplicateUserId(new SocialUser(2L), USER_ID);
assertThat(actual, is(true));
}
@Test
public void duplicateEmail_doesnot_existed() {
boolean actual = checkDupService.duplicateEmail(
SocialUser.GUEST_USER, USER_ID, ProviderType.slipp);
assertThat(actual, is(false));
}
@Test
public void duplicateEmail_login_isSameUser() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
boolean actual = checkDupService.duplicateEmail(USER1, EMAIL, ProviderType.slipp);
assertThat(actual, is(false));
}
@Test
public void duplicateEmail_login_isNotSameUser() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
boolean actual = checkDupService.duplicateEmail(USER2, EMAIL, ProviderType.slipp);
assertThat(actual, is(true));
}
}
테스트 실행, 통과다.
6. ApiUsersControllerTest 클래스에서 UserService 관련 코드 제거
이제 ApiUsersController 클래스는 더 이상 UserService를 사용하지 않으므로, UserService Mock 객체를 만들 필요가 없다. ApiUsersController 협업 대상이 CheckDupService로 변경되었으므로, CheckDupService에 대한 Mock을 만들어서 테스트하도록 ApiUsersControllerTest를 바꾸자.
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
public static final String USER_ID = "userId";
public static final String EMAIL = "email@slipp.net";
public static final SocialUser USER1 = new SocialUser(1L);
public static final SocialUser USER2 = new SocialUser(2L);
public static final String TRUE = "true";
public static final String FALSE = "false";
@Mock
private CheckDupService checkDupService;
private ApiUsersController dut;
@Before
public void init() {
dut = new ApiUsersController(checkDupService);
}
@Test
public void duplicateUserId_login_isSameUser() {
when(
checkDupService.duplicateUserId(any(SocialUser.class), anyString())
).thenReturn(Boolean.FALSE);
String actual = dut.duplicateUserId(USER1, USER_ID);
assertThat(actual, is(FALSE));
}
@Test
public void duplicateUserId_login_isNotSameUser() {
when(
checkDupService.duplicateUserId(any(SocialUser.class), anyString())
).thenReturn(Boolean.TRUE);
String actual = dut.duplicateUserId(USER2, USER_ID);
assertThat(actual, is(TRUE));
}
/* 더 이상 필요 없으므로 삭제!
@Test
public void duplicateEmail_doesnot_existed() {
String actual = dut.duplicateEmail(SocialUser.GUEST_USER, USER_ID, ProviderType.slipp);
assertThat(actual, is(FALSE));
}
*/
@Test
public void duplicateEmail_login_isSameUser() {
when(
checkDupService.duplicateEmail(
any(SocialUser.class), anyString(), any(ProviderType.class))
).thenReturn(Boolean.FALSE);
String actual = dut.duplicateEmail(USER1, EMAIL, ProviderType.slipp);
assertThat(actual, is(FALSE));
}
@Test
public void duplicateEmail_login_isNotSameUser() {
when(
checkDupService.duplicateEmail(
any(SocialUser.class), anyString(), any(ProviderType.class))
).thenReturn(Boolean.TRUE);
String actual = dut.duplicateEmail(USER2, EMAIL, ProviderType.slipp);
assertThat(actual, is(TRUE));
}
}
7. 다시 최초의 문제로 돌아가
최초의 문제는 ApiUsersController 클래스의 아이디/이메일이 이미 존재하는지 확인하는 코드에서 중복이 발생해서 그걸 빼고 싶은 것이었다. 이 부분이 지금은 CheckDupServiceImpl 클래스로 옮겨졌으니, 그 부분의 코드를 보자.
public class CheckDupServiceImpl implements CheckDupService {
...
@Override
public boolean duplicateUserId(SocialUser loginUser, String userId) {
SocialUser socialUser = userService.findByUserId(userId);
if (socialUser == null)
return false;
return !socialUser.isSameUser(loginUser);
}
@Override
public boolean duplicateEmail(SocialUser loginUser, String email, ProviderType providerType) {
SocialUser socialUser = userService.findByEmailAndProviderId(email, providerType);
if (socialUser == null)
return false;
return !socialUser.isSameUser(loginUser);
}
}
굵게 표시한 부분이 중복된 코드인데, 이를 어떻게 없애면 좋을까? 의미를 먼저 파악해보자.
- userId와 일치하는 유저가 존재하는지 검사한다. 존재하지 않으면 false를 리턴한다.
- 단, 현재 로그인한 유저의 userId를 입력한 경우에는 false를 리턴한다.
음,,, 두 번째의 의미를 잘 모르겠다. 이 의미를 알면 조금 더 정리해 볼 수 있을 것 같은데, 지금으로서는 단순히 메서드로 빼는 것 정도에서 끝내야 겠다.
public class CheckDupServiceImpl implements CheckDupService {
private SocialUserService userService;
public CheckDupServiceImpl(SocialUserService userService) {
this.userService = userService;
}
@Override
public boolean duplicateUserId(SocialUser loginUser, String userId) {
SocialUser socialUser = userService.findByUserId(userId);
return doSomeMeaningCheck(loginUser, socialUser);
}
@Override
public boolean duplicateEmail(SocialUser loginUser, String email, ProviderType providerType) {
SocialUser socialUser = userService.findByEmailAndProviderId(email, providerType);
return doSomeMeaningCheck(loginUser, socialUser);
}
private boolean doSomeMeaningCheck(SocialUser loginUser, SocialUser socialUser) {
if (socialUser == null) return false;
return ! socialUser.isSameUser(loginUser);
}
}
CheckDupServiceImplTest를 실행해보자. 녹색이다. 테스트 통과다. doSomeMeaningCheck() 메서드는 아직 의미를 정확하게 몰라서 메서드 이름을 이렇게 지었다.
8. 의미에 맞는 기능 분리
약간의 대화를 거쳐서 doSomeMeaningCheck() 메서드의 의미를 알았다. 음, 정확하게는 다음의 두 기능이 필요한 것이다.
- 회원 가입시 사용할 사용자아이디/이메일 중복 검사 기능
- 회원 정보 수정에서 사용자아이디/이메일 중복 검사 기능
- 현재와 동일한 사용자아이디/이메일 입력시 중복으로 처리하지 않음
이 두 가지는 의미가 다소 다르기 때문에 분리된 메서드를 제공하는 것이 좋지 않을까 하는 생각이 든다. 예를 들면 아래와 같은 분리가 필요하지 않을까? 이건 나중에 다시 생각해보고, 쉬운 것 부터 해보자.
9. 코드 정리: 메서드 이름 변경
CheckDupService 인터페이스는 다음과 같이 정의되어 있다.
public interface CheckDupService {
boolean duplicateUserId(SocialUser loginUser, String userId);
boolean duplicateEmail(SocialUser loginUser, String email, ProviderType providerType);
}
duplicateUserId 란 이름은 "중복된 사용자 ID" 이므로, 검사한다는 의미를 부여할 수 있도록 아래와 같이 check라는 단어를 붙여보자.
public interface CheckDupService {
boolean checkDuplicateUserId(SocialUser loginUser, String userId);
boolean checkDuplicateEmail(SocialUser loginUser, String email, ProviderType providerType);
}
이름을 변경했으면 단위 테스트를 실행해서 통과되는지 확인한다.
ApiUsersController 클래스도 동일하게 check 단어를 붙여주자.
@Controller
public class ApiUsersController {
...
@RequestMapping("/duplicate_userid")
@ResponseBody
public String checkDuplicateUserId(SocialUser loginUser, String userId) {
return Boolean.toString(checkDupService.checkDuplicateUserId(loginUser, userId));
}
@RequestMapping("/duplicate_email")
@ResponseBody
public String checkDuplicateEmail(SocialUser loginUser, String email, ProviderType providerType) {
return Boolean.toString(checkDupService.checkDuplicateEmail(loginUser, email, providerType));
}
}
테스트 메서드의 이름도 변경해주자.
@RunWith(value = MockitoJUnitRunner.class)
public class CheckDupServiceImplTest {
public static final String USER_ID = "userId";
public static final String EMAIL = "email@slipp.net";
public static final SocialUser USER1 = new SocialUser(1L);
public static final SocialUser USER2 = new SocialUser(2L);
@Mock
private SocialUserService userService;
private CheckDupService checkDupService;
@Before
public void init() {
checkDupService = new CheckDupServiceImpl(userService);
}
@Test
public void shouldReturnFalseWhenLoggedUserInputSelfCurrentUserId() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
boolean actual = checkDupService.checkDuplicateUserId(USER1, USER_ID);
assertThat(actual, is(false));
}
@Test
public void shouldReturnTrueWhenLoggedUserInputUserIdOfAnotherUser() {
when(userService.findByUserId(USER_ID)).thenReturn(USER1);
boolean actual = checkDupService.checkDuplicateUserId(USER2, USER_ID);
assertThat(actual, is(true));
}
@Test
public void shouldReturnFalseWhenGuestInputEmailWhichDoesNotExist() {
when(userService.findByEmailAndProviderId(USER_ID, ProviderType.slipp)).thenReturn(null);
boolean actual = checkDupService.checkDuplicateEmail(SocialUser.GUEST_USER, USER_ID, ProviderType.slipp);
assertThat(actual, is(false));
}
@Test
public void shouldReturnFalseWhenLoggedUserInputSelfCurrentEmail() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
boolean actual = checkDupService.checkDuplicateEmail(USER1, EMAIL, ProviderType.slipp);
assertThat(actual, is(false));
}
@Test
public void shouldReturnTrueWhenLoggedUserInputEmailOfOtherUser() {
when(userService.findByEmailAndProviderId(EMAIL, ProviderType.slipp)).thenReturn(USER1);
boolean actual = checkDupService.checkDuplicateEmail(USER2, EMAIL, ProviderType.slipp);
assertThat(actual, is(true));
}
}
@RunWith(value = MockitoJUnitRunner.class)
public class ApiUsersControllerTest {
public static final String USER_ID = "userId";
public static final String EMAIL = "email@slipp.net";
public static final SocialUser USER1 = new SocialUser(1L);
public static final SocialUser USER2 = new SocialUser(2L);
public static final String TRUE = "true";
public static final String FALSE = "false";
@Mock
private CheckDupService checkDupService;
private ApiUsersController dut;
@Before
public void init() {
dut = new ApiUsersController(checkDupService);
}
@Test
public void shouldReturnFalseWhenCheckDuplicateUserIdReturnFalse() {
when(checkDupService.checkDuplicateUserId(any(SocialUser.class), anyString())).thenReturn(Boolean.FALSE);
String actual = dut.checkDuplicateUserId(USER1, USER_ID);
assertThat(actual, is(FALSE));
}
@Test
public void shouldReturnTrueWhenCheckDuplicateUserIdReturnTrue() {
when(checkDupService.checkDuplicateUserId(any(SocialUser.class), anyString())).thenReturn(Boolean.TRUE);
String actual = dut.checkDuplicateUserId(new SocialUser(2L), USER_ID);
assertThat(actual, is(TRUE));
}
@Test
public void shouldReturnFalseWhenCheckDuplicateEmailReturnFalse() {
when(checkDupService.checkDuplicateEmail(any(SocialUser.class), anyString(), any(ProviderType.class))).thenReturn(Boolean.FALSE);
String actual = dut.checkDuplicateEmail(USER1, EMAIL, ProviderType.slipp);
assertThat(actual, is(FALSE));
}
@Test
public void shouldReturnFalseWhenCheckDuplicateEmailReturnTrue() {
when(checkDupService.checkDuplicateEmail(any(SocialUser.class), anyString(), any(ProviderType.class))).thenReturn(Boolean.TRUE);
String actual = dut.checkDuplicateEmail(USER2, EMAIL, ProviderType.slipp);
assertThat(actual, is(TRUE));
}
}
의미에 맞는 기능분리까지 하고 싶으나, UI 코드의 상황을 잘 모르는 관계로 여기에서 마무리 지어본다.
