-
Notifications
You must be signed in to change notification settings - Fork 6
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
[BE] Topic 패키지 관련 코드 리팩토링 #238
Changes from 13 commits
8646b1a
5b153cc
c54a142
1397964
4efef6e
29997b1
82233e0
70f351f
b98ce02
bd32df0
69268ee
f6faad0
960b9b3
68838dc
5bda12f
83f79ff
27a0738
41ee4ef
b0850af
9ffcefd
471a826
5075d4b
30a97f7
470fcb5
9b58830
80d7e9f
939964c
12e83b4
8c869a1
7bdea8a
5cf4318
cc4c0c4
b45f3b1
0dec766
c451958
2433935
4383f25
209a4e2
d6e4e7a
712a289
846490a
cd57810
426ad35
f8370c3
f43c20a
1e36545
f4b7dd6
40d230b
20d9d75
e7bb218
a838a12
feb58e9
b65374e
cfe84c0
cd9169c
422d067
0458224
e04e896
1ad610f
8f3be11
8581453
853bf26
4bd3877
eb2bcd3
0af5c36
29806b9
521e8e2
fa309a5
5699a7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ | |
import jakarta.persistence.JoinColumn; | ||
import jakarta.persistence.ManyToOne; | ||
import jakarta.persistence.OneToMany; | ||
import java.math.BigDecimal; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import lombok.Getter; | ||
|
@@ -81,13 +80,19 @@ public void updatePinInfo(String name, String description) { | |
pinInfo.update(name, description); | ||
} | ||
|
||
public Pin copy(Topic topic) { | ||
return Pin.createPinAssociatedWithLocationAndTopic( | ||
pinInfo.getName(), | ||
pinInfo.getDescription(), | ||
location, | ||
topic | ||
); | ||
/** | ||
* createPinAssociatedWithLocationAndTopic을 재사용하지 않은 이유 | ||
* 내부적으로 copy를 처리하면, 반환값이 필요 없음 -> 외부에서 반환값을 무시함 | ||
* 반환값이 있을 경우, topic과의 연관관계를 맺어주지 않은 상태로 반환하고, | ||
* 외부에서 해당 연관관계를 맺어주어도 됨 -> 이 방법은 객체가 불안정한 상태라고 판단 됨. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. copyToTopic 메서드에서 반환값이 필요 없다는 생각은 동의합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 간략하게 쓰려다보니 생략된 부분이 다소 존재해서, 혼란의 여지가 있었던 것 같아요.
의 의미는 다음과 같습니다. (1) (2) (억지로라도 ?) 외부에서 사용할 수 있도록 하는 방법은 외부에서 연관관계를 맺어주는 방법이 있다. (3) (2)의 방법을 위해서는 기존의 (4) 이 방법은 객체를 불안정한 상태로 반환하는 것과 같다. (5) 따라서, 별도의 메서드로 분리했다. 글로 표현하려다보니 전달이 잘 안될 수도 있을 것 같네요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분을 놓쳐서 추가 코멘트 남깁니다! 또, Pin 리팩터링 PR에 반영되어있는 Pin.copy 메서드를 보시면, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
제가 아는 선에서는 "프로그래머가 예상치 못한 동작을 할 수 있다"라는 이유에서의 권장사항으로 알고 있습니다.
저는 변경 된 행의 개수를 검증하는 것도 필요하다고 생각해요. 위 상황에서 이를 무시하게 되면 프로그래머가 예상치 못한 동작을 할 수도 있다고 생각해요. 물론, List의 add 메서드에서도 성공/실패 여부를 반환해주지만, 이는 대부분 무시하잖아요 ?
사실, 권장사항일 뿐 항상 지켜야 하는 것은 아니라고 생각해요. // Pin 클래스의 일부
public void copyToTopic(Topic otherTopic) {
createPinAssociatedWithLocationAndTopic(
pinInfo.getName(),
pinInfo.getDescription(),
location,
otherTopic
);
} 저는 위 코드에서 메서드 이름과는 다르게, 핀을 복사한다는 로직이 안 보이는 것 같아요. 또한, createXXX 메서드를 재사용 하게 됐을 경우, 나중에 정책이 변경되면 추가적인 질문이나, 의견 환영입니다 !! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 자세히 설명해주셔서 이해가 갔습니다 !! |
||
*/ | ||
|
||
public void copyToTopic(Topic otherTopic) { | ||
PinInfo copiedPinInfo = PinInfo.of(pinInfo.getName(), pinInfo.getDescription()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 PinInfo를 새로 생성해야할 필요가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. VO라고 하기에는 내부에 값을 변경할 수 있는 update가 존재하고, 불변을 보장할 수 없지 않나요 ? 기존의 pinInfo를 그대로 사용한다고 가정했을 때, 하나의 트랜잭션에서 현재 존재하는 기능은 아니지만, 특정 핀을 복사해서 나의 토픽으로 넣을 때 핀의 정보를 수정해서 넣을 수 있게끔 하는 기능이 추가될 수 있지 않을까요 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 제가 (2) 사례를 고려하지 못했군요 ㅋㅋ |
||
Pin copiedPin = new Pin(copiedPinInfo, location, otherTopic); | ||
location.addPin(copiedPin); | ||
|
||
otherTopic.addPin(copiedPin); | ||
} | ||
|
||
public void addPinImage(PinImage pinImage) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
import java.util.Collection; | ||
import java.util.List; | ||
import java.util.NoSuchElementException; | ||
import java.util.Objects; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
|
@@ -34,112 +35,166 @@ public TopicCommandService( | |
this.memberRepository = memberRepository; | ||
} | ||
|
||
public long createNew(AuthMember member, TopicCreateRequest request) { | ||
Topic topic = createNewTopic(member, request); | ||
public Long saveEmptyTopic(AuthMember member, TopicCreateRequest request) { | ||
Topic topic = convertToTopic(member, request); | ||
|
||
List<Long> pinIds = request.pins(); | ||
List<Pin> original = pinRepository.findAllById(pinIds); | ||
Topic savedTopic = topicRepository.save(topic); | ||
|
||
validateExist(pinIds.size(), original.size()); | ||
pinRepository.saveAll(copyPins(original, topic)); | ||
|
||
return topic.getId(); | ||
return savedTopic.getId(); | ||
} | ||
|
||
public long createMerge(AuthMember member, TopicMergeRequest request) { | ||
List<Long> topicIds = request.topics(); | ||
List<Topic> topics = findTopicsByIds(member, topicIds); | ||
private Topic convertToTopic(AuthMember member, TopicCreateRequest request) { | ||
Member creator = findCreatorByAuthMember(member); | ||
|
||
validateExist(topicIds.size(), topics.size()); | ||
Topic topic = createMergeTopic(member, request); | ||
return Topic.createTopicAssociatedWithCreator( | ||
request.name(), | ||
request.description(), | ||
request.image(), | ||
request.publicity(), | ||
request.permission(), | ||
creator | ||
); | ||
} | ||
|
||
List<Pin> original = getPinFromTopics(topics); | ||
pinRepository.saveAll(copyPins(original, topic)); | ||
private Member findCreatorByAuthMember(AuthMember member) { | ||
if (Objects.isNull(member.getMemberId())) { | ||
throw new IllegalArgumentException("Guest는 토픽을 생성할 수 없습니다."); | ||
} | ||
Comment on lines
+65
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validation 좋네요 👍 |
||
|
||
return topic.getId(); | ||
return memberRepository.findById(member.getMemberId()) | ||
.orElseThrow(NoSuchElementException::new); | ||
} | ||
|
||
public void updateTopicInfo( | ||
AuthMember member, | ||
Long id, | ||
TopicUpdateRequest request | ||
) { | ||
Topic topic = topicRepository.findById(id) | ||
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 Topic입니다.")); | ||
member.canTopicUpdate(topic); | ||
public Long saveTopicWithPins(AuthMember member, TopicCreateRequest request) { | ||
Topic topic = convertToTopic(member, request); | ||
|
||
topic.updateTopicInfo( | ||
request.name(), | ||
request.description(), | ||
request.image() | ||
); | ||
List<Pin> originalPins = findAllPins(request.pins()); | ||
validateCopyablePins(member, originalPins); | ||
copyPinsToTopic(originalPins, topic); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
해당 부분만
이렇게 처리하면 한 메서드로 처리 가능할 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (메서드 명만 보고 해당 로직이 무엇인지 유추할 수 있게 하기 위해) 조건문에 따라 다른 로직을 수행한다면, 이는 메서드로 분리해야 한다고 생각해요 ! 이럴 경우, 해당 코멘트에도 남겨놓았지만 해당 구조가 조금 어색하게 느껴지네요.. 메서드 분리를 하지 말아야 할까요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ㅎㅎㅎㅎㅎㅎㅎㅎ 생각을 잘못 했네요 수정했습니다 |
||
|
||
Topic savedTopic = topicRepository.save(topic); | ||
|
||
return savedTopic.getId(); | ||
} | ||
|
||
public void delete(AuthMember member, Long id) { | ||
Topic topic = topicRepository.findById(id) | ||
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 Topic입니다.")); | ||
member.canDelete(topic); | ||
private List<Pin> findAllPins(List<Long> pinIds) { | ||
List<Pin> findPins = pinRepository.findAllById(pinIds); | ||
|
||
if (pinIds.size() != findPins.size()) { | ||
throw new IllegalArgumentException("존재하지 않는 핀 Id가 존재합니다."); | ||
} | ||
|
||
pinRepository.deleteAllByTopicId(id); | ||
topicRepository.deleteById(id); | ||
return findPins; | ||
} | ||
|
||
private List<Topic> findTopicsByIds(AuthMember member, List<Long> topicIds) { | ||
return topicRepository.findAllById(topicIds) | ||
.stream() | ||
.filter(member::canRead) | ||
.toList(); | ||
private void validateCopyablePins(AuthMember member, List<Pin> originalPins) { | ||
int copyablePinCount = (int) originalPins.stream() | ||
.filter(pin -> member.canRead(pin.getTopic())) | ||
.count(); | ||
|
||
if (copyablePinCount != originalPins.size()) { | ||
throw new IllegalArgumentException("복사할 수 없는 pin이 존재합니다."); | ||
} | ||
Comment on lines
+101
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validation 훌륭한데요? |
||
} | ||
|
||
private List<Pin> getPinFromTopics(List<Topic> topics) { | ||
return topics.stream() | ||
.map(Topic::getPins) | ||
.flatMap(Collection::stream) | ||
.toList(); | ||
private void copyPinsToTopic(List<Pin> pins, Topic topic) { | ||
pins.forEach(pin -> pin.copyToTopic(topic)); | ||
} | ||
|
||
private Topic createMergeTopic(AuthMember member, TopicMergeRequest request) { | ||
Member creator = findCreatorByAuthMember(member); | ||
Topic topic = Topic.of( | ||
request.name(), | ||
request.description(), | ||
request.image(), | ||
request.publicity(), | ||
request.permission(), | ||
creator | ||
); | ||
return topicRepository.save(topic); | ||
public Long merge(AuthMember member, TopicMergeRequest request) { | ||
Topic topic = convertToTopic(member, request); | ||
|
||
List<Topic> originalTopics = findAllTopics(request.topics()); | ||
validateCopyableTopics(member, originalTopics); | ||
List<Pin> originalPins = getAllPinsFromTopics(originalTopics); | ||
|
||
copyPinsToTopic(originalPins, topic); | ||
|
||
Topic savedTopic = topicRepository.save(topic); | ||
|
||
return savedTopic.getId(); | ||
} | ||
|
||
private Topic createNewTopic(AuthMember member, TopicCreateRequest request) { | ||
private Topic convertToTopic(AuthMember member, TopicMergeRequest request) { | ||
Member creator = findCreatorByAuthMember(member); | ||
Topic topic = Topic.of( | ||
|
||
return Topic.createTopicAssociatedWithCreator( | ||
request.name(), | ||
request.description(), | ||
request.image(), | ||
request.publicity(), | ||
request.permission(), | ||
creator | ||
); | ||
return topicRepository.save(topic); | ||
} | ||
|
||
private Member findCreatorByAuthMember(AuthMember member) { | ||
return memberRepository.findById(member.getMemberId()) | ||
.orElseThrow(NoSuchElementException::new); | ||
private List<Topic> findAllTopics(List<Long> topicIds) { | ||
List<Topic> findTopics = topicRepository.findAllById(topicIds); | ||
|
||
if (topicIds.size() != findTopics.size()) { | ||
throw new IllegalArgumentException("존재하지 않는 토픽 Id가 존재합니다."); | ||
} | ||
|
||
return findTopics; | ||
} | ||
|
||
private void validateExist(int idCount, int existCount) { | ||
if (idCount != existCount) { | ||
throw new IllegalArgumentException("찾을 수 없는 ID가 포함되어 있습니다."); | ||
private void validateCopyableTopics(AuthMember member, List<Topic> originalTopics) { | ||
int copyablePinCount = (int) originalTopics.stream() | ||
.filter(member::canRead) | ||
.count(); | ||
|
||
if (copyablePinCount != originalTopics.size()) { | ||
throw new IllegalArgumentException("복사할 수 없는 토픽이 존재합니다."); | ||
} | ||
} | ||
|
||
private List<Pin> copyPins(List<Pin> pins, Topic topic) { | ||
return pins.stream() | ||
.map(original -> original.copy(topic)) | ||
private List<Pin> getAllPinsFromTopics(List<Topic> topics) { | ||
return topics.stream() | ||
.map(Topic::getPins) | ||
.flatMap(Collection::stream) | ||
.toList(); | ||
} | ||
|
||
public void updateTopicInfo( | ||
AuthMember member, | ||
Long topicId, | ||
TopicUpdateRequest request | ||
) { | ||
Topic topic = findTopic(topicId); | ||
|
||
validateUpdateAuth(member, topic); | ||
|
||
topic.updateTopicInfo(request.name(), request.description(), request.image()); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updateTopicStatus도 수정하는 메서드가 필요할 것 같습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 사항 반영했습니다 ! |
||
private Topic findTopic(Long topicId) { | ||
return topicRepository.findById(topicId) | ||
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 Topic입니다.")); | ||
} | ||
|
||
private void validateUpdateAuth(AuthMember member, Topic topic) { | ||
if (member.canTopicUpdate(topic)) { | ||
return; | ||
} | ||
|
||
throw new IllegalArgumentException("업데이트 권한이 없습니다."); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validate을 이렇게 빼니까 훨씬 깔끔하네요!! |
||
|
||
public void delete(AuthMember member, Long topicId) { | ||
Topic topic = findTopic(topicId); | ||
|
||
validateDeleteAuth(member, topic); | ||
|
||
pinRepository.deleteAllByTopicId(topicId); | ||
topicRepository.deleteById(topicId); | ||
} | ||
|
||
private void validateDeleteAuth(AuthMember member, Topic topic) { | ||
if (member.canDelete(topic)) { | ||
return; | ||
} | ||
|
||
throw new IllegalArgumentException("삭제 권한이 없습니다."); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,11 @@ | ||
package com.mapbefine.mapbefine.topic.application; | ||
|
||
import com.mapbefine.mapbefine.auth.domain.AuthMember; | ||
import com.mapbefine.mapbefine.location.domain.LocationRepository; | ||
import com.mapbefine.mapbefine.topic.domain.Topic; | ||
import com.mapbefine.mapbefine.topic.domain.TopicRepository; | ||
import com.mapbefine.mapbefine.topic.dto.response.TopicDetailResponse; | ||
import com.mapbefine.mapbefine.topic.dto.response.TopicResponse; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
|
@@ -17,28 +15,47 @@ public class TopicQueryService { | |
|
||
private final TopicRepository topicRepository; | ||
|
||
public TopicQueryService(final TopicRepository topicRepository, LocationRepository locationRepository) { | ||
public TopicQueryService(final TopicRepository topicRepository) { | ||
this.topicRepository = topicRepository; | ||
} | ||
|
||
public List<TopicResponse> findAll(AuthMember member) { | ||
public List<TopicResponse> findAllReadable(AuthMember member) { | ||
return topicRepository.findAll().stream() | ||
.filter(topic -> member.canRead(topic)) | ||
.filter(member::canRead) | ||
.map(TopicResponse::from) | ||
.toList(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. findAllReadable이라는 네이밍이 아주 이해하기 좋네요!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 근데 저희 모든 조회 쿼리에서 isDeleted = true 인 건 걸러줘야겠네요....? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 맞아요.. 걸러줘야해요.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
public List<TopicDetailResponse> findAllByIds(List<Long> ids) { | ||
return topicRepository.findByIdIn(ids).stream() | ||
public TopicDetailResponse findDetailById(AuthMember member, Long id) { | ||
Topic topic = topicRepository.findById(id) | ||
.orElseThrow(() -> new IllegalArgumentException("해당하는 Topic이 존재하지 않습니다.")); | ||
|
||
if (member.canRead(topic)) { | ||
return TopicDetailResponse.from(topic); | ||
} | ||
|
||
throw new IllegalArgumentException("조회할 수 없는 Topic 입니다."); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3. TopicCommandService와 예외처리 메서드 분리 방식을 통일해도 괜찮을 것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 어떤 메서드에서는 조건문 안에서 예외를 던지고, 어떤 메서드에서는 조건문 안에서 early return을 해서 이를 통일해달라는 말씀이신거죠 ? 사실 이 부분을 적용하고자 했었는데, 부정 조건문 사용을 지양하려다보니까 통일이 잘 안 되더라구요 ㅠ_ㅠ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이미 TopicCommandService에서 쓴 방식이 좋아서, 같은 방식으로 하면 좋겠다 싶었어요!! public TopicDetailResponse findDetailById(AuthMember member, Long topicId) {
Topic topic = findTopic(topicId);
validateReadAuth(member, topic);
return TopicDetailResponse.from(topic);
}
private void validateReadAuth(AuthMember member, Topic topic) {
if (member.canRead(topic)) {
return;
}
throw new IllegalArgumentException("조회 권한이 없습니다.");
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 반영했어요 !! |
||
|
||
// TODO: 2023/08/07 토픽의 id가 존재하지 않는 경우도 검증해야 하는가 ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 제가 생각했을 때는 검증하는 것이 좋을 것 같다는 생각이 들어요! 사용자가 웹을 통해 접근한 경우를 고려해봤을 때, 해당 메서드를 통해 전달된 토픽의 id 가 존재하지 않는 경우라면, 사용자가 해당 토픽을 선택할 때에는 존재했지만, 같이 보기를 누르기 이전에 사라진 경우일 것 같아요. 저희가 이전에 delete 할 때 id가 존재하지 않는 경우를 검증해야 하는가에 대해서 토의할 때, 쥬니 말을 빌리면, 사용자가 삭제하려던 토픽이었으니 삭제하기 이전에 이미 삭제되었어도 사용자의 의도대로 된 것이니까 상관이 없지만, 이번에는 사용자가 해당 토픽의 내용을 보려고하는데 사라진 경우이니 사용자의 의도와 다른 방향으로 흘러갔다고 생각이 들기 때문에 예외를 터트리는 것이 맞을 것 같다는 생각이 드네요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 사항 검증 로직 추가 및 테스트 추가 구현 완료하였습니다 ! |
||
public List<TopicDetailResponse> findDetailsByIds(AuthMember member, List<Long> ids) { | ||
List<Topic> topics = topicRepository.findByIdIn(ids); | ||
|
||
validateReadableTopics(member, topics); | ||
|
||
return topics.stream() | ||
.map(TopicDetailResponse::from) | ||
.collect(Collectors.toList()); | ||
.toList(); | ||
} | ||
|
||
public TopicDetailResponse findById(AuthMember member, Long id) { | ||
Topic topic = topicRepository.findById(id) | ||
private void validateReadableTopics(AuthMember member, List<Topic> topics) { | ||
int readableCount = (int) topics.stream() | ||
.filter(member::canRead) | ||
.orElseThrow(() -> new IllegalArgumentException("해당하는 Topic이 존재하지 않습니다.")); | ||
.count(); | ||
|
||
return TopicDetailResponse.from(topic); | ||
if (topics.size() != readableCount) { | ||
throw new IllegalArgumentException("읽을 수 없는 토픽이 존재합니다."); | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저랑 취향이 비슷하시군요 ㅋㅋ 좋습니다