refactor: CompletedCoursesService (책임 분리, 테스트 코드 개선)

[whxogus215]

🛠️작업 내용

• 파일(기이수 성적 파일)을 담당하는 서비스 객체 추가(FileService)
• CompletedCoursesService 로직 리팩토링 및 테스트 코드 추가

현재 코드에서 발견한 문제점

1. 테스트의 의도가 불분명하다

// CompletedCoursesServiceDataTest의 테스트 메서드

@Test  
@DisplayName("업로드 데이터 저장 테스트")  
public void testUploadFile() {  
    //기이수 과목 데이터1  
    CompletedCoursesDomain data1 =  
        CompletedCoursesDomain.builder().  
            userDomain(userRepository.findByStudentId(19111111L).get()).  
            coursesDomain(coursesDao.findByCourseId(12345L)).  
            year(23).semester("1학기").  
            build();  
  
    //기이수 과목 데이터2  
    CompletedCoursesDomain data2 =  
        CompletedCoursesDomain.builder().  
            userDomain(userRepository.findByStudentId(19111111L).get()).  
            coursesDomain(coursesDao.findByCourseId(54321L)).  
            year(23).semester("1학기").  
            build();  
  
    //기이수 과목 저장  
    completedCoursesDao.save(data1);  
    completedCoursesDao.save(data2);  
  
    List<CompletedCoursesDomain> dataList = new ArrayList<>();  
  
    dataList.add(data1);  
    dataList.add(data2);  
  
    UserDomain user = userRepository.findByStudentId(19111111L).get();  
    assertEquals(dataList, completedCoursesDao.findByUserDomain(user));  
}

기존에 있던 CompletedCoursesService, 이하 CCS에 대한 테스트 메서드는 다음과 같이 여러 Dao의 메서드를 호출하며, 기이수 과목이 효과적으로 저장되는지 테스트하고 있습니다.
하지만, 여기에는 CCS의 로직을 호출하지는 않고 있습니다. 따라서 이것이 CCS 로직에 대한 테스트 코드라고 판단되지는 않았습니다.

2. CompletedCoursesService가 많은 책임을 갖고 있다.

CompletedCoursesService, 이하 CCS는 기이수 과목(CompletedCourse) 이외에도 다양한 의존성을 갖고 있습니다.

private Workbook creatWorkbook(MultipartFile file, String extension) throws IOException {  
    Workbook workbook = null;  
    if (extension.equals("xlsx")) {  
        workbook = new XSSFWorkbook(file.getInputStream());  
    } else if (extension.equals("xls")) {  
        workbook = new HSSFWorkbook(file.getInputStream());  
    }  
    return workbook;  
}

@Transactional  
public void extractData(Sheet worksheet, DataFormatter dataFormatter, UserDomain userDomain) {  
    List<CompletedCoursesDomain> completedCoursesList = new ArrayList<>();
    for (int i = FIRST_ROW; i < worksheet.getPhysicalNumberOfRows(); i++) {
        Row row = worksheet.getRow(i);  
  
        String yearAsString = dataFormatter.formatCellValue(row.getCell(1));  
        int year = Integer.parseInt(yearAsString) % 100;  //년도  
  
        String semester = dataFormatter.formatCellValue(row.getCell(2)); //학기  
  
        String courseIdAsString = dataFormatter.formatCellValue(row.getCell(3));  
        Long courseId = courseIdToLong(courseIdAsString); //학수번호  
  
        ... 이하 생략 (CompletedCoursesDao의 save를 호출하는 로직)  
}

위 두 메서드만 보더라도, CCS는 엑셀 파일의 확장자, 형식에 대해서도 알고 있습니다.
CCS는 엑셀 파일에 저장된 년도, 학기, 학수번호 데이터만 필요한 것이며, 이들이 엑셀 파일에서 어떠한 순서로 저장되어 있는지 까지는 알지 않아도 된다고 생각했습니다.
CCS가 기이수 과목을 다루는 책임만 갖는다면, 엑셀 파일의 형식이 변경되더라도 그 영향을 받지 않을 것입니다. (SRP of SOLID)

해결한 방법

1. given/when/then 패턴 적용

given-when-then 구조를 적용하여 각각의 테스트 메서드가 테스트하고자 하는 것이 무엇인지를 한 눈에 알아볼 수 있도록 개선하였습니다. when절에는 테스트하고자 하는 메서드만 존재합니다.

2. 테스트를 문서화하기

테스트 코드만으로 해당 어플리케이션이 대략적으로 어떠한 기능을 수행하는지를 알 수 있도록 @DisplayName 의 내용을 문장으로 표현하였습니다.

@Test  
@DisplayName("성적파일을 업로드했던 유저가 다시 업로드하면, 초기화 후 새로 저장된다.")  
public void saveCompletedCoursesTest2() throws IOException {  
    ... 구현 코드 생략 
}

해당 테스트 코드를 보고, 성적파일을 업로드했던 유저가 다시 업로드하면, 기존 데이터가 초기화되고, 다시 저장되는구나라는 걸 바로 알 수 있습니다.

테스트 메서드를 한글로 표현하는 방법도 있지만, 이는 특정 테스트 메서드를 재사용할 경우, 효과적이라고 판단했습니다. 이를 제외한 경우에는 테스트 메서드를 한글로 작성하면 단어 사이에 언더바(_)를 붙여야 해서 개인적으로는 번거로운 것 같습니다.

테스트 메서드를 한글로 작성

public static MockMultipartFile 기이수성적조회_파일_생성(final String filePath) throws IOException {  
    String fileName = "기이수성적조회";  
    File file = new File(filePath);  
    return new MockMultipartFile(fileName, file.getName(), "xlsx", new FileInputStream(file));  
}

@Test  
@DisplayName("사용자가 올바른 기이수 성적파일을 업로드하면, 예외가 발생하지 않는다.")  
void validateWorkbookTest1() throws IOException {  
    //given  
    final MockMultipartFile testFile = 기이수성적조회_파일_생성("src/test/resources/file/기이수성적조회.xlsx");  
    final Workbook workbook = fileService.createWorkbook(testFile);  
  
    //when & then  
    assertThatCode(() -> fileService.validateWorkbook(workbook))  
            .doesNotThrowAnyException();  
}

3. File과 관련된 책임을 별도의 객체로 분리

AS-IS

TO-BE

테스트 커버리지 개선

리팩토링 전 : CompletedCoursesServiceDataTest의 라인 커버리지 : 14%

(왼쪽부터 클래스 / 메서드 / 라인에 대한 커버리지입니다.)

리팩토링 후

1. CompletedCoursesServiceTest의 라인 커버리지 : 96%
   
2. FileServiceTest의 라인 커버리지 : 84%
   

✔️PR 체크리스트

• 필요한 테스트를 작성했는가?
• 다른 코드를 깨뜨리지 않았는가?
• 연결된 이슈 외에 다른 이슈를 해결한 코드가 담겨있는가?

[gmltn9233]

분리하니까 로직이 더 한눈에 들어오네요. 좋습니다~~👍

[gmltn9233]

해당 메서드는 크롤링을 통해 얻은 csv 데이터를 로직에 맞게 변환하여 gonghakCourse로 변환하는 메서드입니다.

제일 처음 지섭이형이 만든 로직은 이 메서드를 이용하여 raw file을 바로 넣어서 해당 메서드를 통해 DB에 삽입하였습니다.
현재 이렇게 삽입된 DB에서 csv파일을 export 하여 운영 DB에 import 하는 식으로 진행하였는데, #62 에서 해당 csv 파일을 이용해 개발환경을 통합하는 과정에서 해당 메서드가 사용되지 않게 되었습니다.

하지만 이후 학과 추가라던지 사용될 일이 있을것 같아 남겨두었습니다.

[whxogus215]

확인했습니다 👍

[gmltn9233]

오 파일 업로드를 이렇게 테스트할 수 있군요..

[gmltn9233]

service로 userDetails를 넘겨주는 방식에서 studentId를 넘겨주는 방식으로 리팩토링하였는데, 로직에 studentId 만 사용되기때문에 studentId를 직접 넘겨주는 방식으로 리팩토링한건가요??
관련해서 userDetails 을 넘겨주는 방식이 맞을지 studentId를 넘겨주는 방식이 맞을지 고민해보았습니다.

혹시 모를 확장성이나 책임 측면에서 controller에서 userDetails에서 studentId를 추출하는 로직을 가지고 있는것이 적절하지 않다고 생각이 들었습니다. 허나 그렇게 생각하면 userDetails의 원본인 authentication을 넘겨줘야 하나?
근데 그건 또 너무 불필요하게 높은 추상화라는 생각이 들었습니다.

관련해서 의견을 듣고싶습니다. 사실상 studentId 말고는 service에서 이용할 일이 없긴 하니 현재 방식이 적절할까요?

[whxogus215]

제가 코드를 보고 의문이 들었던 점은 다음과 같았습니다.
CompletedCoursesService가 시큐리티 관련 의존성을 가져야 할까? 였습니다.
사실 이번에 CompletedCoursesService를 리팩토링하면서 중점적으로 가졌던 부분이기도 합니다.
CompletedCoursesService는 Controller로부터 기이수 과목을 조회하라는 메시지를 전달받는데, 이 책임을 수행하기 위해 필요한 정보만 알면 되지 않을까라는게 저의 생각이었습니다.

만약, studentId가 아니라 userDetails를 넘기도록 한다면, 테스트 코드를 작성하는 데에도 userDetails에 대한 추가 작업이 필요합니다.

@Test
@DisplayName("기이수 성적 파일을 업로드하면, 기이수 성적 과목이 저장된다.")
public void saveCompletedCoursesTest() throws IOException {
    //given
    MockMultipartFile testFile = 기이수성적조회_파일_생성("src/test/resources/file/기이수성적조회.xlsx");

    //when
    completedCoursesService.saveCompletedCourses(testFile, TEST_ID);

    //then
    List<CompletedCoursesDomain> findCourses = completedCoursesService.getCompletedCourses(TEST_ID);
    assertThat(findCourses.size()).isEqualTo(TEST_FILE_ROW_SIZE);
}

현재는 학번만 전달하면 기이수 과목을 조회하는 메서드를 호출할 수 있기 때문에 테스트 코드가 간단합니다.
만약, getCompletedCourses가 단순히 학번이 아니라 userDetails를 넘겨줘야 한다면, 기이수 서비스에 관한 테스트에 도 시큐리티 의존성을 추가해야 합니다.

[gmltn9233]

테스트 코드 측면에서 확실히 studentId를 넘기는 쪽이 이점이 더 많네요... 확인했습니다!

[gmltn9233]

새로운 service를 만들었음에도 dto를 사용해서 깔끔하게 처리하는 모습 좋습니다👍