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

[BE] 액션 이력 조회 리펙토링 #141

Merged
merged 11 commits into from
Aug 4, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import server.haengdong.presentation.request.EventSaveRequest;
import server.haengdong.presentation.response.EventDetailResponse;
import server.haengdong.presentation.response.EventResponse;
import server.haengdong.presentation.response.StepResponse;
import server.haengdong.presentation.response.StepsResponse;

@RequiredArgsConstructor
@RestController
Expand All @@ -35,9 +35,9 @@ public ResponseEntity<EventDetailResponse> findEvent(@PathVariable("eventId") St
}

@GetMapping("/api/events/{eventId}/actions")
public ResponseEntity<StepResponse> findActions(@PathVariable("eventId") String token) {
StepResponse stepResponse = StepResponse.of(eventService.findActions(token));
public ResponseEntity<StepsResponse> findActions(@PathVariable("eventId") String token) {
StepsResponse stepsResponse = StepsResponse.of(eventService.findActions(token));

return ResponseEntity.ok(stepResponse);
return ResponseEntity.ok(stepsResponse);
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,57 +1,27 @@
package server.haengdong.presentation.response;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import server.haengdong.application.response.ActionAppResponse;

public record StepResponse(
List<ActionsResponse> steps
String type,
String stepName,
List<String> members,
List<ActionResponse> actions
) {

public static StepResponse of(List<ActionAppResponse> actions) {
if (actions.isEmpty()) {
return new StepResponse(List.of());
}
List<ActionsResponse> actionsResponse = new ArrayList<>();
Set<String> members = new HashSet<>();
ActionAppResponse firstAction = getFirstAction(actions);
List<ActionAppResponse> group = new ArrayList<>();
group.add(firstAction);
String currentActionType = firstAction.actionTypeName();
members.add(firstAction.name());

for (int i = 1; i < actions.size(); i++) {
ActionAppResponse action = actions.get(i);
String typeName = action.actionTypeName();
if (currentActionType.equals(typeName)) {
if (typeName.equals("IN")) {
members.add(action.name());
}
if (typeName.equals("OUT")) {
members.remove(action.name());
}
group.add(action);
continue;
}
actionsResponse.add(ActionsResponse.of(group, members));
currentActionType = typeName;
group.clear();
if (typeName.equals("IN")) {
members.add(action.name());
}
if (typeName.equals("OUT")) {
members.remove(action.name());
}
group.add(action);
}
actionsResponse.add(ActionsResponse.of(group, members));

return new StepResponse(actionsResponse);
public static StepResponse of(List<ActionAppResponse> actions, List<String> members, String stepName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

r: 필드 순서와 of 메서드의 매개변수 순서를 맞춰야 할 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했습니다!

return new StepResponse(
actions.get(0).actionTypeName(),
stepName,
new ArrayList<>(members),
toActionsResponse(actions)
);
}

private static ActionAppResponse getFirstAction(List<ActionAppResponse> actions) {
return actions.get(0);
private static List<ActionResponse> toActionsResponse(List<ActionAppResponse> actions) {
return actions.stream()
.map(ActionResponse::of)
.toList();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package server.haengdong.presentation.response;

import java.util.ArrayList;
import java.util.List;
import server.haengdong.application.response.ActionAppResponse;
import server.haengdong.application.response.ActionAppResponse.ActionType;

public record StepsResponse(List<StepResponse> steps) {

public static StepsResponse of(List<ActionAppResponse> actions) {
List<StepResponse> steps = new ArrayList<>();
List<String> currentMembers = new ArrayList<>();
List<List<ActionAppResponse>> groups = createGroups(actions);
Copy link
Contributor

Choose a reason for hiding this comment

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

c: List<List<ActionAppResponse>> 타입이 너무 복잡한데 객체로 분리하는게 더 좋을까요? 비즈니스 로직 자체가 복잡한데, 로직을 수행하기에 객체 분리가 더 필요해서 복잡한건지, 리팩터링 방향성을 다 같이 더 얘기해보면 좋을 것 같아요.

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 복잡하다고 생각해요. 좀더 리팩터링하면 좋겠단 생각이 들지만 현재까지 진행된 사항도 훌륭하다고 생각해요. 이후에 dto 변경 가능성도 높다고 생각해서 이만해도 충분하다 생각해요!


int billGroupCount = 0;
for (List<ActionAppResponse> group : groups) {
changeCurrentMembers(group, currentMembers);
if (group.get(0).actionType() == ActionType.BILL) {
billGroupCount++;
}
StepResponse stepResponse = StepResponse.of(group, currentMembers, billGroupCount + "차");
Copy link
Contributor

Choose a reason for hiding this comment

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

c: ActionType.BILL이 아닌 IN, OUT 경우에도 stepName이 n차로 들어가지 않나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stepName은 제거하도록 하겠습니다.

steps.add(stepResponse);
}
return new StepsResponse(steps);
}

private static List<List<ActionAppResponse>> createGroups(List<ActionAppResponse> actions) {
List<List<ActionAppResponse>> groups = new ArrayList<>();

for (ActionAppResponse action : actions) {
if (groups.isEmpty() || isActionTypeChange(action, groups)) {
groups.add(new ArrayList<>());
}
groups.get(groups.size() - 1).add(action);
}
return groups;
}

private static boolean isActionTypeChange(ActionAppResponse action, List<List<ActionAppResponse>> groups) {
List<ActionAppResponse> currentGroup = groups.get(groups.size() - 1);
return currentGroup.get(0).actionType() != action.actionType();
}

private static void changeCurrentMembers(List<ActionAppResponse> group, List<String> currentMembers) {
for (ActionAppResponse action : group) {
if (action.actionType() == ActionType.IN) {
currentMembers.add(action.name());
continue;
}
if (action.actionType() == ActionType.OUT) {
currentMembers.remove(action.name());
}
Comment on lines +43 to +49
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이부분에 MemberAction Type으로 인원을 추가하고 제거하는 비지니스 로직이 녹아있다고 판단되는데 어떻게 생각하시나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

백호가 말씀하신 대로 CurrentMembers 객체를 사용해서 현재 인원을 계산하는 것도 좋을 것 같아요. CurrentMembers 내부에 참여자의 이름을 Set으로 관리하고있어서 원본 순서를 보장할 수는 없겠지만, 현재 그룹 내 순서를 보장하지 않아도 문제가 없어보입니다. 👍

}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package server.haengdong.presentation.response;


import static org.assertj.core.api.Assertions.assertThat;

import java.util.List;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import server.haengdong.application.response.ActionAppResponse;
import server.haengdong.application.response.ActionAppResponse.ActionType;

class StepsResponseTest {

@DisplayName("이웃한 같은 타입의 액션들을 그룹화 하여 응답객체를 생성한다.")
@Test
void of() {
List<ActionAppResponse> actions = List.of(
new ActionAppResponse(1L, "망쵸", null, 1L, ActionType.IN),
new ActionAppResponse(2L, "백호", null, 2L, ActionType.IN),
new ActionAppResponse(3L, "감자탕", 10_000L, 3L, ActionType.BILL),
new ActionAppResponse(4L, "인생네컷", 10_000L, 4L, ActionType.BILL),
new ActionAppResponse(5L, "소하", null, 5L, ActionType.IN),
new ActionAppResponse(6L, "웨디", null, 6L, ActionType.IN),
new ActionAppResponse(7L, "망쵸", null, 7L, ActionType.OUT),
new ActionAppResponse(8L, "백호", null, 8L, ActionType.OUT),
new ActionAppResponse(9L, "노래방", 20_000L, 9L, ActionType.BILL)
);

StepsResponse stepsResponse = StepsResponse.of(actions);

StepsResponse expected = new StepsResponse(
List.of(
new StepResponse("IN", "0차", List.of("망쵸", "백호"), List.of(
new ActionResponse(1L, "망쵸", null, 1L),
new ActionResponse(2L, "백호", null, 2L)
)),
new StepResponse("BILL", "1차", List.of("망쵸", "백호"), List.of(
new ActionResponse(3L, "감자탕", 10_000L, 3L),
new ActionResponse(4L, "인생네컷", 10_000L, 4L)
)),
new StepResponse("IN", "1차", List.of("망쵸", "백호", "소하", "웨디"), List.of(
new ActionResponse(5L, "소하", null, 5L),
new ActionResponse(6L, "웨디", null, 6L)
)),
new StepResponse("OUT", "1차", List.of("소하", "웨디"), List.of(
new ActionResponse(7L, "망쵸", null, 7L),
new ActionResponse(8L, "백호", null, 8L)
)),
new StepResponse("BILL", "2차", List.of("소하", "웨디"), List.of(
new ActionResponse(9L, "노래방", 20_000L, 9L)
))
Copy link
Contributor

Choose a reason for hiding this comment

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

IN, OUT 경우에는 차수 이름을 null, BILL인 경우에는 1차부터 올리는 방식으로 하는게 깔끔할 것 같아요. 하지만 오늘 팀 전체 회의에서 stepName을 관리하지 않는 방향으로 얘기가 나왔으니, 확정되는 방향으로 리팩터링 하면 될 것 같습니다! 👍

)
);

assertThat(stepsResponse).isEqualTo(expected);
}

@DisplayName("액션이 없으면 빈 스탭들이 만들어진다.")
@Test
void ofEmpty() {
StepsResponse stepsResponse = StepsResponse.of(List.of());

assertThat(stepsResponse.steps()).isEmpty();
}
}
Loading