[REFACTOR] Service Layer 리팩토링

[jminkkk]

📌 어떤 기능을 리팩터링 하나요?

해당 Service Layer 테스트 중 발견된 개선이 필요한 사항들을 발견했습니다. 이를 개선합니다.

추가로 다른 도메인에서도 Service Layer 리팩토링이 필요한 부분이 있다면 코멘트로 이를 남겨주시고 이슈 본문의 TODO에 추가해주세요.

TODO

• TemplateTagService, TagService 분리
• SourceCodeService에 레거시 코드 제거 및 개선
• SourceCodeService 중 문제가 있는 로직 수정
• CategoryTemplateApplicationService를 포함한 파사드 서비스, 하위 서비스의 역할과 책임 돌아보기
• Fixture 정리 및 메서드명 통일

[jminkkk]

TemplateTagService, TagService 분리

📌 어떤 기능을 리팩터링 하나요?

codezap.tag.service 아래의 TemplateTagService가 존재합니다.
TagService를 분리하여 Tag 도메인에만 해당하는 로직들을 TemplateTagService가 아닌 새 서비스로 이동합니다.
기존 TemplateTagService는 TemplateTag에 해당하는 로직들을 둡니다.

두 도메인에 대한 로직들은 TemplateTagApplicationService 에 두는 것이 더 적절하다고 생각됩니다.
예시로 생성에 대한 부분 예시를 두겠습니다.

AS-IS

// TemplateTagService.java

    @Transactional
    public void createTags(Template template, List<String> tagNames) {
        List<String> existingTags = tagRepository.findNameByNamesIn(tagNames);
        templateTagRepository.saveAll(
                existingTags.stream()
                        .map(tagRepository::fetchByName)
                        .map(tag -> new TemplateTag(template, tag))
                        .toList()
        );

        List<Tag> newTags = tagRepository.saveAll(
                tagNames.stream()
                        .filter(tagName -> !existingTags.contains(tagName))
                        .map(Tag::new)
                        .toList()
        );
        templateTagRepository.saveAll(
                newTags.stream()
                        .map(tag -> new TemplateTag(template, tag))
                        .toList()
        );
    }

TO-BE

// // TemplateTagApplicationService.java

    @Transactional
    public void createTemplateTags(Template template, List<String> tagNames) {
        List<Tag> tags = tagService.findOrCreateTags(tagNames);
        templateTagService.createTemplateTag(template, newTags);
    }

// TemplateTagService.java

    @Transactional
    public void createTemplateTags(Template template, List<Tag> tags) {
        List<TemplateTag> templateTags = tags.stream()
            .map(tag -> new TemplateTag(template, tag))
            .collect(Collectors.toList());

        templateTagRepository.saveAll(templateTags);
    }

// TagService.java

    @Transactional
    public List<Tag> findOrCreateTags(Template template, List<String> tagNames) {
        List<Tag> existingTags = tagRepository.findAllByNamesIn(tagNames);
        List<Tag> newTags = this.createTags(existingTags, newTagNames);
        return existingTags.saveAll(newTags);
    }

---

AS-IS

// TemplateTagService.java

    public List<Tag> getByTemplate(Template template) {
        return templateTagRepository.findAllByTemplate(template).stream()
                .map(TemplateTag::getTag)
                .toList();
    }

TO-BE

// TagService로 이동
    public List<Tag> getAllByTemplate(List<TemplateTag> templateTags) {
        return templateTags.stream()
                .map(TemplateTag::getTag)
                .toList();
    }

[jminkkk]

SourceCodeService에 레거시 코드 제거 및 개선

AS-IS

    @Transactional
    public void deleteByIds(List<Long> templateIds) {
        templateIds.forEach(sourceCodeRepository::deleteByTemplateId);
    }

deleteByTemplateId 메서드 n번 호출 & 쿼리 n번

3. sourceCodeService.deleteByIds()

소스 코드 아이디로 삭제하는 것 같지만 템플릿 아이디로 삭제하는 것임
위와 같은 혼동 가능성이 있는 메서드명 변경하기

TO-BE

    @Transactional
    public void deleteByIds(List<Long> templateIds) {
        sourseCodeRepository.deleteAllByTemplateId(templateIds);

        // 또는 sourseCodeRepository.deleteAllByTemplateIdInBatch(templateIds);
    }

각각 repository 메서드 1번 호출 & 쿼리 n번(deleteAllByTemplateId) 또는 1번(deleteAllByTemplateIdInBatch)

등등이 더 있을 수 있습니다.

3. sourceCodeService.deleteByTemplateIds()

🔍 참고할만한 자료(선택)

createSourceCodes() 메서드에 @transactional 관련

createSourceCodes에서는 saveAll만 호출하고 있습니다. 따라서 트랜잭션으로 묶이지 않아도 된다고 생각합니다~
또한 saveAll은 이미 내부에서 @transactional를 선언하고 있어 일부 save 실패 시 롤백됩니다.

[jminkkk]

SourceCodeService 중 문제가 있는 로직 수정

각 케이스에 대해 검증 또는 로직 변경이 필요

소스 코드 생성 실패 케이스

• 순서 중복된 코드 존재
• 순서가 1부터 시작하지 않는 코드

소스 코드 수정 성공 케이스

• 일부 소스 코드 삭제 및 새로운 소스 코드 추가 시, 삭제된 코드 순서는 앞당겨지고 새로 추가된 소스 코드의 순서는 가장 마지막 순서�
• 썸네일 코드 삭제 시, 새로 순서가 1인 코드가 썸네일으로 등록

소스 코드 수정 실패 케이스

• 소스 코드 전체 삭제
• 변경할 소스 코드의 순서가 중복된 소스 코드의 순서인 경우

[jminkkk]

CategoryTemplateApplicationService를 포함한 파사드 서비스, 하위 서비스의 역할과 책임

단일 책임 원칙 (SRP)

• 파사드의 주 역할: 복잡한 하위 시스템에 대한 간단한 인터페이스를 제공하는 것
  • 현재 구조에서는 카테고리 조회와 권한 검증이라는 별도의 책임을 가지고 있어, 파사드의 주 역할에서 적절하지 않음

추상화 수준 (DIP 관련)

현재 구조에서는 카테고리 관련 로직 변경 시 파사드 클래스도 수정해야 한다. == 유지보수에 좋지 않다.
검색 조건에서 카테고리 조회도 추가되는 것처럼 요구사항이 변경된다면? 만약 회원당 한개씩 있는 카테고리를 모든 회원이 같이 사용해야만 한다면?

category.validateAuthorization(member)를 제거해야 하므로 파사드도 함께 변경되어야 한다.
즉 파사드의 장점인 서브시스템 내부 설계의 변경이 다른 서브시스템에 독립적으로 자유롭게 될 수 있는 것과 멀어진다.

요약

• 고수준 모듈은 저수준 모듈에 의존하면 안된다 -> 고수준 모듈(파사드)은 저수준 모듈(하위 서비스)이 무엇을 하는지 몰라야 한다.
• 카테고리 조회와 권한 검증은 낮은 수준의 세부 구현에 해당하며, 이는 파사드 내부에 노출되면 안됨

즉, CategoryTemplateApplicationService 는 카테고리 서비스에게 카테고리를 가져오도록 요청만 해야 하며, 카테고리 서비스에서 줘도 되는지(권한 체크), 있는지 없는지(fetchBy) 등을 판단해야함

AS-IS

@Service
@RequiredArgsConstructor
public class CategoryTemplateApplicationService {
    private final CategoryService categoryService;
    private final TemplateApplicationService templateApplicationService;

    public Long createTemplate(Member member, CreateTemplateRequest createTemplateRequest) {
        Category category = categoryService.fetchById(createTemplateRequest.categoryId());
        category.validateAuthorization(member);
        return templateApplicationService.createTemplate(member, category, createTemplateRequest);
    }

    public void update(Member member, Long templateId, UpdateTemplateRequest updateTemplateRequest) {
        Category category = categoryService.fetchById(updateTemplateRequest.categoryId());
        category.validateAuthorization(member);
        templateApplicationService.update(member, templateId, updateTemplateRequest, category);
    }
}

TO-BE

@Service
@RequiredArgsConstructor
public class CategoryTemplateApplicationService {
    private final CategoryService categoryService;
    private final TemplateApplicationService templateApplicationService;

    public Long createTemplate(Member member, CreateTemplateRequest createTemplateRequest) {
        Category category = categoryService.findById(createTemplateRequest.categoryId(), member.getId());
        return templateApplicationService.createTemplate(member, category, createTemplateRequest);
    }

    public void update(Member member, Long templateId, UpdateTemplateRequest updateTemplateRequest) {
        Category category = categoryService.findById(updateTemplateRequest.categoryId(), member.getId());
        templateApplicationService.update(member, templateId, updateTemplateRequest, category);
    }
}

[zangsu]

Member.equals() 에서 클래스 비교 제거

해당 클래스가 Category 의 필드로 사용되고, lazy loading 을 사용하면서 프록시 객체와 Member 객체의 동등성 비교가 생긴다.
이 때문에 클래스 비교를 제거해야 한다.

if (o == null || getClass() != o.getClass()) {
    return false;
}

[zangsu]

DB 예외 핸들링이 필요할지 고민

CategoryService.findAllByMemer(Member member) 에 저장되지 않은 멤버가 들어가면 DB 예외 발생

이 부분을 예외 처리를 해 주어야 할지~

관련 테스트 코드

[zangsu]

CategoryTemplateService 을 CategoryService 로 통합

현재 CategoryTemplateService 의 public 메서드는 deleteById() 하나밖에 없음.
그리고, 이 메서드는 TemplateRepository 의 의존성이 필요하다는 이유 이외에는 클래스가 분리되어 있을 이유가 없음.

이에 CategoryService클래스로의 통합을 제안

[kyum-q]

Fixture 정리

MemberFixture 두개

MemberFixture가 두 개 존재한다 (fixture 패키지, member.fixture 패키지)
MemberFixture는 하나로 통일하면 좋을 것 같다

Fixture 메서드명 통일

Fixture 마다 정적 팩토리 메서드 명이 다르다
해당 메서드들을 통일하면 추후 재사용할 때 편리할 것 같다.

내가 제안하는 이름은 결국엔 모든 메서드가 정적 팩토리 메서드이니 정팩메 이름 규칙을 따르면 좋을 것 같다.

• from : 하나의 매개 변수를 받아서 객체를 생성
• of : 여러개의 매개 변수를 받아서 객체를 생성

추가적으로 Fixture 를 생성자로 사용할 일이 없을 것 같으니 private로 생성자를 막는 건 어떤가요?

[jminkkk]

CategoryTemplateApplicationService를 제거

테스트를 하기 위해 어떤 것을 테스트 해야하는지, 이 계층의 역할은 무엇인지 고민했습니다.
MemberTemplateApplicationService에서 단건 조회 같이 카테고리가 필요하지 않는 곳에 의존성이 제거되긴 하나, MemberTemplateApplicationService가 결과적으로 TemplateApplicationService를 두 번 의존하고 있는 등 구조가 매우 복잡하다.

제가 생각하기에 선택할 수 있는 방법

1. 의존성 제거를 우선으로 챙긴다.

• 현재도 의존성 제거가 완벽하게 되어있지 않다고 생각
• 클린 아키텍처(정확히는 헥사고날)를 참고하여 각 유스케이스(요구사항 ex. 템플릿 생성)별 하나의 객체를 만든다. 클린아키텍처, 사용 예시 깃허브 (클린 아키텍처를 도입하자는 것이 아님. usecase만 얘기하는 것임)
• 이렇게 되면 해당 요구사항에서 필요한 것들만 의존할 수 있기 때문에 명확해진다.

2. 의존성을 조금 양보하고 현재 서비스의 복잡도 낮춘다.

• CategoryTemplateApplicationService 등 중간 파사드(?)를 제거하고 각 도메인 중 여러 도메인의 의존이 필요한 경우, 상위 하나의 파사드만 놓는다.

[HoeSeong123]

TemplateService로 넘어오는 파라미터 변경

AS-IS

TemplateService의 createTemplate() 메서드를 확인해보면 다음과 같이 작성되어 있습니다.

public Template createTemplate(Member member, CreateTemplateRequest createTemplateRequest, Category category) {
    Template template = new Template(
            member,
            createTemplateRequest.title(),
            createTemplateRequest.description(),
            category);
    return templateRepository.save(template);
}

CreateTemplateRequest에는 title, description, sourceCodes, categoryId 등 다양한 필드들이 존재하지만 해당 메소드에서는 title과 description만을 사용하고 있습니다. 다른 필드들은 Templateservice의 상위 클래스들에서 사용되고 있습니다.
이에 다음과 같이 변경을 제안합니다.

TO-BE

public Template createTemplate(Member member, String title, String description, Category category) {
    Template template = new Template(
            member,
            title,
            description,
            category);
    return templateRepository.save(template);
}

[HoeSeong123]

리스트를 반환하는 JPA 쿼리 메서드명 변경

리스트를 반환하는데 getByMemberId, findByMemberId 등으로 작성되어 있습니다.
그렇다보니 테스트를 작성할 때 해당 메소드들이 하나의 값만을 반환해야 할 것 같은 느낌을 받았습니다.
이에 아래와 같은 메서드명 변경을 제안합니다.

AS-IS

public List<Template> getByMemberId(Long memberId) {
    return templateRepository.findByMemberId(memberId);
}

TO-BE

public List<Template> getAllByMemberId(Long memberId) {
    return templateRepository.findAllByMemberId(memberId);
}

---

(여기는 몰리가 추가함)

초롱의 이슈와 동일한 이유로 TemplateTagService도 변경이 필요합니다~

AS-IS

    public List<Tag> getByTemplate(Template template) {
        return templateTagRepository.findAllByTemplate(template).stream()
                .map(TemplateTag::getTag)
                .toList();
    }

TO-BE

    public List<Tag> getAllByTemplate(Template template) {
        return templateTagRepository.findAllByTemplate(template).stream()
                .map(TemplateTag::getTag)
                .toList();
    }

[HoeSeong123]

중복검사 로직

AS-IS

현재 여러 템플릿 삭제 시 아래와 같은 코드가 사용되는데, id가 중복되어 있는지 검사하는 로직이 TemplateService에만 위치하고 있습니다.

// TemplateApplicationService.java

@Transactional
public void deleteByMemberAndIds(Member member, List<Long> ids) {
    thumbnailService.deleteByTemplateIds(ids);
    sourceCodeService.deleteByIds(ids);
    templateTagService.deleteByIds(ids);
    templateService.deleteByMemberAndIds(member, ids);
}

// TemplateService.java

@Transactional
public void deleteByMemberAndIds(Member member, List<Long> ids) {
    if (ids.size() != new HashSet<>(ids).size()) {
        throw new CodeZapException(HttpStatus.BAD_REQUEST, "삭제하고자 하는 템플릿 ID가 중복되었습니다.");
    }
    ids.forEach(id -> deleteById(member, id));
}

모든 삭제 로직 전에 검증이 우선되어야 한다고 생각하여 아래와 같이 변경을 제안합니다.

TO-BE

// TemplateApplicationService.java

@Transactional
public void deleteByMemberAndIds(Member member, List<Long> ids) {
     if (ids.size() != new HashSet<>(ids).size()) {
        throw new CodeZapException(HttpStatus.BAD_REQUEST, "삭제하고자 하는 템플릿 ID가 중복되었습니다.");
    }
    thumbnailService.deleteByTemplateIds(ids);
    sourceCodeService.deleteByIds(ids);
    templateTagService.deleteByIds(ids);
    templateService.deleteByMemberAndIds(member, ids);
}

[HoeSeong123]

사용되지 않는 메소드

ThumbnailSerivce에서 전체 썸네일을 조회하는 메소드가 사용되지 않고 있습니다.
이에 삭제를 제안합니다.

public ExploreTemplatesResponse findAll() {
    return ExploreTemplatesResponse.from(thumbnailRepository.findAll());
}

외에도 사용되지 않는 메소드들이 다소 있는 것 같습니다.
전체적으로 한 번 확인하면 좋을 것 같습니다.

---

(여기는 몰리가 추가함)

초롱의 이슈와 동일한 이유로 TemplateTagService도 변경이 필요합니다~

TemplateTagService의 getTemplateIdContainTagIds이 사용되고 있지 않습니다.
또 getTemplateIdContainTagIds의 반환 값이 TemplateId인데, TemplateTagService에서 TemplateId를 반환하는 게 사용이 어색하다고 생각합니다.

    public List<Long> getTemplateIdContainTagIds(List<Long> tagIds) {
        if (tagIds.isEmpty()) {
            throw new CodeZapException(HttpStatus.BAD_REQUEST, "태그 ID가 0개입니다. 필터링 하지 않을 경우 null로 전달해주세요.");
        }
        tagIds.forEach(this::validateTagId);
        return templateTagRepository.findAllTemplateIdInTagIds(tagIds, tagIds.size());
    }

    private void validateTagId(Long tagId) {
        if (!tagRepository.existsById(tagId)) {
            throw new CodeZapException(HttpStatus.NOT_FOUND, "식별자 " + tagId + "에 해당하는 태그가 존재하지 않습니다.");
        }
    }

[jminkkk]

탬플릿 태그 삭제로 사용되지 않는 태그가 있는 경우

일단 TemplateTagService는 TagService 용도로도 사용되고 있습니다. (Tag 목록 조회 등등)
TemplateTagService에서 템플릿들에 대한 템플릿 태그 삭제 시 한개도 사용되지 않는 태그인 경우 태그도 함께 제거되어야 할 것 같습니다.

ex)
"Moly"라는 태그가 template1, template2에서 사용되고 있었는데, 두 템플릿에 해당하는 태그 목록 삭제 요청이 들어올 경우
"template1, Moly", "template2, Moly"라는 템플릿 태그는 삭제 되지만, "Moly"라는 태그는 계속해서 남아있습니다.

AS-IS

// TemplateTagService.java
    public void deleteByIds(List<Long> templateIds) {
        templateIds.forEach(templateTagRepository::deleteAllByTemplateId);
    }

TO-BE

// TemplateTagApplicationService.java
    public void deleteByIds(List<Long> templateIds) {
        List<Tag> notUsedTags = templateTagService.deleteAllByTemplate(templateIds);
        tagService.deleteIfNotUsed(notUsedTags);
    }

// TemplateTagService.java
    public void deleteAndCollectUnused(List<Long> templateIds) {
        // 1. 태그로 템플릿 테그 조회 
        // 2. 템플릿 태그 삭제 및 태그 목록 반환
        // 3. 태그들 중 다른 템플릿에 사용되는 태그들 제외
        // 4. 사용되지 않는 태그 목록 반환 
    }

// TagService.java
    public void deleteIfNotUsed(List<Tag> tags) {
        // 1. 태그 삭제
    }

[zeus6768]

Member.equals() 에서 클래스 비교 제거

해당 클래스가 Category 의 필드로 사용되고, lazy loading 을 사용하면서 프록시 객체와 Member 객체의 동등성 비교가 생긴다. 이 때문에 클래스 비교를 제거해야 한다.

if (o == null || getClass() != o.getClass()) {
    return false;
}

(해당 코멘트 관련)
#664

[zeus6768]

FindTemplateResponse` 클래스명 변경 제안

• 사용되는 컨트롤러 메서드명 TemplateController.getTemplateById()
• memberTemplateApplicationService.getTemplateById()
• templateApplicationService.getById()

해당 DTO가 사용되는 모든 메서드명의 prefix가 get인 반면, 클래스명dml prefix가 Find여서 가독성을 저해합니다.

---

(켬미가 이어씀)

find vs get vs fetch 메서드명 통일

조회 메서드의 prefix를 통일하면 좋을 것 같아요
켬미 개인 선호에 따른 변경

• get : 해당하는 값이 없을 시 빈 리스트나 optional 값을 반환하는 메서드
• fetch : 해당하는 값이 없을 시 예외를 발생 시키는 메서드

MemberCategoryApplicationService

• findAllByMember

CategoryService

• findAllByMember -> get
• findAll -> get

MemberService

• findMember -> fetch
• getByTemplateId ->fetch
• getById -> fetch

TemplateTagService

• findAllByTemplates -> get

TemplateApplicationService

• getById -> fetch
• findAllBy -> get

MemberTemplateApplicationService

• getTemplateById -> fetch

SourceCodeService

• getByTemplateAndOrdinal -> fetch
• findSourceCodesByTemplate -> get

TemplateService

• getById -> fetch
• findAll -> get

ThumbnailService

• getByTemplate -> fetch

[kyum-q]

메서드명 끝에 By 구문 통일성 필요

일부 메서드는 create만 사용한다

일부 메서드는 create 이후 인자를 작성하여 사용한다

통일하면 좋을 것 같다

[jminkkk]

SourceCode 삭제 or 조회

• 저희 도메인 개념상, 템플릿은 무조건 한개의 소스코드를 가지고 있어야하니 소스코드가 아무것도 없을 시 예외처리가 추가되어야 함
• 템플릿에 소스코드가 아무것도 존재하지 않는다. -> 검증할 수 없어 테스트를 일단 제거하였으니 추후 추가가 필요합니다.
• 소스 코드 생성 시 소스 코드의 순서가 연달아 나오는지 검증하는 코드가 없다(컨트롤러단에서 하는 것 같다)

[zangsu]

엔티티로 조회하는 기능 수정

CategoryService.findAllByMember() -> InvalidDataAccessApiUsageException 발생
Member member 대신 long memberId 로 조회하면 DB 예외가 아닌 java.util.NoSuchElementException 발생
이에 엔티티 객체가 아닌, id 값으로 조회하도록 변경 제안