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

[Feature] - 여행기 나라 관련 리팩토링 #586

Merged
merged 13 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 0 additions & 27 deletions backend/src/main/java/kr/touroot/tag/controller/TagController.java
Original file line number Diff line number Diff line change
@@ -1,23 +1,15 @@
package kr.touroot.tag.controller;

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import io.swagger.v3.oas.annotations.responses.ApiResponses;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.validation.Valid;
import java.net.URI;
import java.util.List;
import kr.touroot.global.exception.dto.ExceptionResponse;
import kr.touroot.tag.dto.TagCreateRequest;
import kr.touroot.tag.dto.TagResponse;
import kr.touroot.tag.service.TagService;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

Expand All @@ -29,25 +21,6 @@ public class TagController {

private final TagService tagService;

@Operation(summary = "태그 생성")

Choose a reason for hiding this comment

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

👍

@ApiResponses(value = {
@ApiResponse(
responseCode = "201",
description = "태그가 생성이 정상적으로 성공했을 때"
),
@ApiResponse(
responseCode = "400",
description = "Body에 유효하지 않은 값이 존재하거나 중복된 태그가 존재할 때",
content = @Content(schema = @Schema(implementation = ExceptionResponse.class))
)
})
@PostMapping
public ResponseEntity<TagResponse> createTag(@Valid @RequestBody TagCreateRequest request) {
TagResponse data = tagService.createTag(request);
return ResponseEntity.created(URI.create("/api/v1/tags/" + data.id()))
.body(data);
}

@Operation(summary = "모든 태그 조회")
@ApiResponses(value = {
@ApiResponse(
Expand Down
15 changes: 0 additions & 15 deletions backend/src/main/java/kr/touroot/tag/dto/TagCreateRequest.java

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,4 @@
import org.springframework.data.jpa.repository.JpaRepository;

public interface TagRepository extends JpaRepository<Tag, Long> {

boolean existsByTag(String tag);
}
17 changes: 0 additions & 17 deletions backend/src/main/java/kr/touroot/tag/service/TagService.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
package kr.touroot.tag.service;

import java.util.List;
import kr.touroot.global.exception.BadRequestException;
import kr.touroot.tag.domain.Tag;
import kr.touroot.tag.dto.TagCreateRequest;
import kr.touroot.tag.dto.TagResponse;
import kr.touroot.tag.repository.TagRepository;
import lombok.RequiredArgsConstructor;
Expand All @@ -19,20 +16,6 @@ public class TagService {

private final TagRepository tagRepository;

@Transactional
public TagResponse createTag(TagCreateRequest tagCreateRequest) {
validateDuplicated(tagCreateRequest);
Tag savedTag = tagRepository.save(tagCreateRequest.toTag());

return TagResponse.from(savedTag);
}

private void validateDuplicated(TagCreateRequest tagCreateRequest) {
if (tagRepository.existsByTag(tagCreateRequest.tag())) {
throw new BadRequestException("이미 존재하는 태그입니다.");
}
}

@Cacheable(cacheNames = "tag")
@Transactional(readOnly = true)
public List<TagResponse> readTags() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import kr.touroot.global.exception.BadRequestException;
import kr.touroot.travelogue.domain.search.CountryCode;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
Expand All @@ -24,6 +25,8 @@
@Entity
public class TravelogueCountry {

private static final int MIN_COUNT = 1;

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
Expand All @@ -40,8 +43,34 @@ public class TravelogueCountry {
private Integer count;

public TravelogueCountry(Travelogue travelogue, CountryCode countryCode, Integer count) {
validate(travelogue, countryCode, count);
this.travelogue = travelogue;
this.countryCode = countryCode;
this.count = count;
}


private void validate(Travelogue travelogue, CountryCode countryCode, Integer count) {
validateNotNull(travelogue, countryCode, count);
validateCountryCode(countryCode);
validateCount(count);
}

private void validateNotNull(Travelogue travelogue, CountryCode countryCode, Integer count) {
if (travelogue == null || countryCode == null || count == null) {
throw new BadRequestException("여행기와 국가 코드, 국가 코드의 count 는 null 일 수 없습니다.");
}
}

private void validateCountryCode(CountryCode countryCode) {
if (countryCode == CountryCode.NONE) {
throw new BadRequestException("여행기의 국가 코드는 NONE 일 수 없습니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

오잉? enum에 존재하는 값 중 하나만 예외적으로 처리되네요.
이 부분은 많이 어색한 것 같습니다.
CountryCode에 NONE이라는 값이 없던지 예외 처리를 안하는 방식이 자연스럽다고 생각해요.

해당 검증은 왜 필요한가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

먼저 NONE은, 사용자가 검색 시 존재하지 않는 나라에 대해 검색을 하더라도 예외를 발생시키는 것이 아니라 빈 결과를 반환하기 위해 도입되었습니다. null을 반환해서 처리하는 방법도 있겠지만, 일종의 null object pattern으로 NONE을 도입한거구요!

하지만 TravelogueCountry 는 NONE 코드에 대해 저장될 필요가 없습니다.
NONE 을 null 로 감싼 것이라고 생각하면 말이죠.

하지만 해당 부분이 되려 도메인을 조금 복잡하게 만드는 경향이 있다는 생각이 드는 것 같습니다.
차라리 null로 처리하는 것이 나을수도 있을 것 같은데, 이에 대해 어떻게 생각하실까요??


private void validateCount(Integer count) {
if (count < MIN_COUNT) {
throw new BadRequestException(String.format("국가 코드의 개수는 %d 보다 커야합니다.", MIN_COUNT));
}
}
Comment on lines +64 to +68
Copy link
Member Author

Choose a reason for hiding this comment

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

해당 부분은 개발자가 직접 계산해서 넣어주는 값이기 때문에, 사용자의 잘못된 요청이 아닌 서버의 에러로 봐야 한다고 생각하는데요.
이뿐만 아니라 null 체킹도 일부는 서버의 에러로 봐야할 것 같다고 생각합니다.

해당 부분은 이후에 같이 이야기해보면 좋을 것 같아요.

}
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public TraveloguePlace(
this.name = name;
this.position = position;
this.travelogueDay = travelogueDay;
this.countryCode = CountryCode.valueOfIgnoreCase(countryCode);
this.countryCode = CountryCode.from(countryCode);
}

public TraveloguePlace(
Expand Down Expand Up @@ -147,11 +147,7 @@ private void validatePlaceNameLength(String placeName) {
}

private void validateCountryCode(String countryCode) {
try {
CountryCode.valueOfIgnoreCase(countryCode);
} catch (IllegalArgumentException e) {
throw new BadRequestException("존재하지 않는 국가 코드입니다");
}
CountryCode.from(countryCode);
}

public void addPhoto(TraveloguePhoto photo) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.Arrays;
import java.util.Set;
import kr.touroot.global.exception.BadRequestException;

public enum CountryCode {

Expand Down Expand Up @@ -259,7 +260,11 @@ public static CountryCode findByName(String name) {
.orElse(NONE);
}

public static CountryCode valueOfIgnoreCase(String name) {
return CountryCode.valueOf(name.toUpperCase());
public static CountryCode from(String code) {
try {
return CountryCode.valueOf(code.toUpperCase());
} catch (IllegalArgumentException exception) {
throw new BadRequestException("존재하지 않는 국가 코드입니다.");
}
}
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
package kr.touroot.travelogue.domain.search;

import java.util.Arrays;
import kr.touroot.global.exception.BadRequestException;

public enum SearchType {
TITLE, AUTHOR, COUNTRY;

public static SearchType from(String searchType) {
return Arrays.stream(SearchType.values())
.filter(type -> searchType.equalsIgnoreCase(type.name()))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 검색 키워드 종류입니다."));
try {
return SearchType.valueOf(searchType.toUpperCase());
} catch (IllegalArgumentException exception) {
throw new BadRequestException("존재하지 않는 검색 키워드 종류입니다.");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,33 @@ public class TravelogueCountryService {

private final TravelogueCountryRepository travelogueCountryRepository;

@Transactional(readOnly = true)
public List<TravelogueCountry> readCountryByTravelogue(Travelogue travelogue) {
return travelogueCountryRepository.findAllByTravelogue(travelogue);
}

@Transactional
public void createTravelogueCountries(Travelogue travelogue, TravelogueRequest request) {
public List<TravelogueCountry> createTravelogueCountries(Travelogue travelogue, TravelogueRequest request) {
Map<CountryCode, Long> countryCounts = countCountries(request);

countryCounts.forEach((countryCode, count) -> travelogueCountryRepository.save(
new TravelogueCountry(travelogue, countryCode, count.intValue())));
return countryCounts.entrySet().stream()
.map(entry -> travelogueCountryRepository.save(
new TravelogueCountry(travelogue, entry.getKey(), entry.getValue().intValue())))
nak-honest marked this conversation as resolved.
Show resolved Hide resolved
.toList();
}

private Map<CountryCode, Long> countCountries(TravelogueRequest request) {
return request.days().stream()
.flatMap(day -> day.places().stream())
.map(place -> CountryCode.valueOf(place.countryCode()))
.filter(countryCode -> countryCode != CountryCode.NONE)
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
}
Comment on lines 33 to 39
Copy link
Member Author

Choose a reason for hiding this comment

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

해당 부분은 일급 컬렉션으로 묶을까 하다, 두었는데 이에 대해 어떻게 생각하실까욥?


@Transactional(readOnly = true)
public List<TravelogueCountry> getTravelogueCountryByTravelogue(Travelogue travelogue) {
return travelogueCountryRepository.findAllByTravelogue(travelogue);
}

@Transactional
public void updateTravelogueCountries(Travelogue travelogue, TravelogueRequest request) {
public List<TravelogueCountry> updateTravelogueCountries(Travelogue travelogue, TravelogueRequest request) {
deleteAllByTravelogue(travelogue);
createTravelogueCountries(travelogue, request);
return createTravelogueCountries(travelogue, request);
}

@Transactional
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public TravelPlanPlace(Long id, Integer order, TravelPlanDay day, String name, P
this.day = day;
this.name = name;
this.position = position;
this.countryCode = CountryCode.valueOfIgnoreCase(countryCode);
this.countryCode = CountryCode.from(countryCode);
}

public TravelPlanPlace(Integer order, TravelPlanDay day, String name, String latitude, String longitude,
Expand Down Expand Up @@ -115,7 +115,7 @@ private void validatePlaceNameLength(String placeName) {

private void validateCountryCode(String countryCode) {
try {
CountryCode.valueOfIgnoreCase(countryCode);
CountryCode.from(countryCode);
} catch (IllegalArgumentException e) {
throw new BadRequestException("존재하지 않는 국가 코드입니다");
}
Expand Down
56 changes: 0 additions & 56 deletions backend/src/test/java/kr/touroot/tag/TagControllerTest.java

This file was deleted.

5 changes: 0 additions & 5 deletions backend/src/test/java/kr/touroot/tag/fixture/TagFixture.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package kr.touroot.tag.fixture;

import kr.touroot.tag.domain.Tag;
import kr.touroot.tag.dto.TagCreateRequest;
import kr.touroot.tag.dto.TagResponse;

public enum TagFixture {
Expand All @@ -21,10 +20,6 @@ public Tag get() {
return new Tag(tag);
}

public TagCreateRequest getCreateRequest() {
return new TagCreateRequest(tag);
}

public TagResponse getResponse(Long id) {
return new TagResponse(id, tag);
}
Expand Down
27 changes: 0 additions & 27 deletions backend/src/test/java/kr/touroot/tag/helper/TagTestHelper.java

This file was deleted.

Loading
Loading