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

fix: timetableLecture 오류 수정 #1096

Merged
merged 14 commits into from
Nov 28, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ ResponseEntity<Void> deleteAllTimetablesFrame(
)
@Operation(summary = "시간표에 강의 정보 추가",
description = """
lecture_id가 있는 경우 class_title, class_time, professor은 null, grades는 '0'으로 입력해야합니다.\n
lecture_id가 없는 경우 class_title, class_time, professor, grades을 선택적으로 입력합니다.
lecture_id가 있는 경우 class_infos, professor은 null, grades는 '0'으로 입력해야합니다.\n
lecture_id가 없는 경우 class_infos, professor, grades을 선택적으로 입력합니다.
Comment on lines +126 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

A

설명이 자세해져서 좋은 거 같아요!

""")
@SecurityRequirement(name = "Jwt Authentication")
@PostMapping("/v2/timetables/lecture")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import in.koreatech.koin.domain.timetableV2.model.TimetableFrame;
import in.koreatech.koin.domain.timetableV2.model.TimetableLecture;
import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.Size;

@JsonNaming(value = SnakeCaseStrategy.class)
public record TimetableLectureResponse(
Expand Down Expand Up @@ -81,12 +80,13 @@ public record ClassInfo(
String classPlace
) {
public static List<ClassInfo> of(String classTime, String classPlace) {
Copy link
Contributor

@kwoo28 kwoo28 Nov 27, 2024

Choose a reason for hiding this comment

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

A

전체적인 of()메서드의 로직부분에 대한 제안해봅니다.

classTime 반복문 내에서 classPlace관련 로직을 다루는것보다, 각각 시간과 장소에 대한 로직을 분리해서 for문으로 돌리는게 좋지않을까요??

제가 생각한방식은 처음에 class_time관련된 부분은 -1구분자로 나눠서 2차원 배열로 만든 후에, class_time과 class_place중에 max_size를 알아낸 후에, 그 max_size크기 만큼 각각 class_time과 class_place에서 for문 돌리면서 쌍을 맞추는 방식은 어떨까요? (이 부분 무시하셔도 될것같습니다..)

일단 지금은 에러 수정사항이니 이 부분은 리팩토링으로 두면 어떨까 생각합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 리펙토링으로 남기겠습니다.

// 정규 강의인 경우 강의 장소가 없기 때문에 바로 반환
// 강의 장소가 없는 경우 강의 시간과 매핑을 못하기 때문에 바로 반환
if (classPlace == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

C

혹시 classPlace를 굳이 null로 조건문을 달아서 따로 로직 처리하는 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

강의 시간은 무조건 현재 존재하지만, 장소는 존재하지 않는 경우가 존재합니다.
초기 정규 강의는 classPlace가 null이기 때문에 체크를 하지 않으면 에러가 반환이 되서 조건문을 달았습니다.

return List.of(new ClassInfo(parseClassTimes(classTime), null));
}

// 구분자를 바탕으로 강의 시간과 강의 장소 분리
// TODO. StringBuilder으로 리펙토링
String[] classPlaceSegment = classPlace.split(",\\s*");
String[] classTimeSegment = classTime.substring(1, classTime.length() - 1).trim().split(",\\s*");

Expand All @@ -98,24 +98,34 @@ public static List<ClassInfo> of(String classTime, String classPlace) {
int parseInt = Integer.parseInt(segment);
if (parseInt == -1) {
if (!currentTimes.isEmpty()) {
classInfos.add(new ClassInfo(new ArrayList<>(currentTimes), classPlaceSegment[index++]));
if (classPlaceSegment.length < index + 1) {
classInfos.add(new ClassInfo(new ArrayList<>(currentTimes), ""));
} else {
classInfos.add(
new ClassInfo(new ArrayList<>(currentTimes), classPlaceSegment[index++]));
}
currentTimes.clear();
}
}
else {
} else {
currentTimes.add(parseInt);
}
}

if (!currentTimes.isEmpty()) {
classInfos.add(new ClassInfo(new ArrayList<>(currentTimes), classPlaceSegment[index]));
if (classPlaceSegment.length < index + 1) {
classInfos.add(new ClassInfo(new ArrayList<>(currentTimes), ""));
} else {
classInfos.add(
new ClassInfo(new ArrayList<>(currentTimes), classPlaceSegment[index++]));
}
}

return classInfos;
}

private static List<Integer> parseClassTimes(String classTime) {
if (classTime == null) return null;
if (classTime == null)
return null;

String classTimeWithoutBrackets = classTime.substring(INITIAL_BRACE_INDEX, classTime.length() - 1);
return Arrays.stream(classTimeWithoutBrackets.split(SEPARATOR))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static in.koreatech.koin.domain.timetableV2.validation.TimetableFrameValidate.validateUserAuthorization;

import java.util.List;
import java.util.Objects;

import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -18,6 +19,7 @@
import in.koreatech.koin.domain.timetableV2.repository.TimetableLectureRepositoryV2;
import in.koreatech.koin.domain.timetableV2.factory.TimetableLectureCreator;
import in.koreatech.koin.domain.timetableV2.factory.TimetableLectureUpdater;
import in.koreatech.koin.global.auth.exception.AuthorizationException;
import lombok.RequiredArgsConstructor;

@Service
Expand Down Expand Up @@ -70,7 +72,7 @@ private TimetableLectureResponse getTimetableLectureResponse(Integer userId, Tim
public void deleteTimetableLectures(List<Integer> request, Integer userId) {
request.stream()
.map(timetableLectureRepositoryV2::getById)
.peek(lecture -> validateUserAuthorization(lecture.getTimetableFrame().getId(), userId))
.peek(lecture -> validateUserAuthorization(lecture.getTimetableFrame().getUser().getId(), userId))
Copy link
Contributor

Choose a reason for hiding this comment

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

A

이 부분을 제대로 안 읽은 리뷰어의 잘못..👀

.forEach(lecture -> timetableLectureRepositoryV2.deleteById(lecture.getId()));
}

Expand Down
Loading