-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from 6 commits
32f8439
16e835e
af1c2d6
c4181da
2bb9bc0
a794707
fc2a5a9
be89d9f
b639197
429c192
0b5eb09
d41fcd9
3de2275
92230fb
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 |
---|---|---|
|
@@ -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( | ||
|
@@ -83,7 +82,25 @@ public record ClassInfo( | |
public static List<ClassInfo> of(String classTime, String classPlace) { | ||
// 정규 강의인 경우 강의 장소가 없기 때문에 바로 반환 | ||
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. R이번 PR내용은 아니긴한데.. 정규강의도 강의 장소 추가가 가능합니다! 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. 이 부분은 주석 설명이 잘못된 것 같습니다. |
||
if (classPlace == null) { | ||
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. C혹시 classPlace를 굳이 null로 조건문을 달아서 따로 로직 처리하는 이유가 있나요? 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. 강의 시간은 무조건 현재 존재하지만, 장소는 존재하지 않는 경우가 존재합니다. |
||
return List.of(new ClassInfo(parseClassTimes(classTime), null)); | ||
String[] split = classTime.substring(1, classTime.length() - 1).trim().split(",\\s*"); | ||
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. RclassTime이 null값이면 NPE발생할것같아요! 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. 정규 강의인 경우 getClassTime 메소드를 사용해서 null 체크를 하고 있습니다 ! 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. 커스텀인 경우에는 발생하지않나요?? 이번에 stage에서 생긴 Cannot invoke "java.util.Collection.toArray()" because "c" is null 오류가 이 부분같다고 생각이 드는데.. 지금 로컬에서 테스트해봤는데, 커스텀 생성에 class_time값을 null을 주면 똑같은 오류 발생하고있습니다! 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. 커스텀인 경우 class_time이 null이 되면 안되는 것을 dto에서 어노테이션을 통해 검증을 해야하는데, 정규 강의의 경우 class_time를 null로 추기 때문에 발생하는 문제라고 생각합니다. 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. 음.. 그러면 pr에 수정된 코드는 해결된건가요?? 일단 승인하겠습니다! |
||
|
||
List<ClassInfo> classInfos = new ArrayList<>(); | ||
List<Integer> currentTimes = new ArrayList<>(); | ||
|
||
for (String str : split) { | ||
int num = Integer.parseInt(str); | ||
if (!currentTimes.isEmpty() && currentTimes.get(currentTimes.size() - 1) + 1 != num) { | ||
classInfos.add(new ClassInfo(new ArrayList<>(currentTimes), null)); | ||
currentTimes.clear(); | ||
} | ||
currentTimes.add(num); | ||
} | ||
|
||
if (!currentTimes.isEmpty()) { | ||
classInfos.add(new ClassInfo(new ArrayList<>(currentTimes), null)); | ||
} | ||
|
||
return classInfos; | ||
} | ||
|
||
// 구분자를 바탕으로 강의 시간과 강의 장소 분리 | ||
|
@@ -98,11 +115,15 @@ 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); | ||
} | ||
} | ||
|
@@ -113,16 +134,6 @@ public static List<ClassInfo> of(String classTime, String classPlace) { | |
|
||
return classInfos; | ||
} | ||
|
||
private static List<Integer> parseClassTimes(String classTime) { | ||
if (classTime == null) return null; | ||
|
||
String classTimeWithoutBrackets = classTime.substring(INITIAL_BRACE_INDEX, classTime.length() - 1); | ||
return Arrays.stream(classTimeWithoutBrackets.split(SEPARATOR)) | ||
.map(String::strip) | ||
.map(Integer::parseInt) | ||
.toList(); | ||
} | ||
} | ||
|
||
public static List<InnerTimetableLectureResponse> from(List<TimetableLecture> timetableLectures) { | ||
|
@@ -200,8 +211,6 @@ private static String getClassTime(TimetableLecture timetableLecture, Lecture le | |
} | ||
|
||
private static final String GRADE_ZERO = "0"; | ||
private static final int INITIAL_BRACE_INDEX = 1; | ||
private static final String SEPARATOR = ","; | ||
|
||
public static TimetableLectureResponse of(TimetableFrame timetableFrame, Integer grades, Integer totalGrades) { | ||
return new TimetableLectureResponse( | ||
|
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.
A
전체적인 of()메서드의 로직부분에 대한 제안해봅니다.
classTime 반복문 내에서 classPlace관련 로직을 다루는것보다, 각각 시간과 장소에 대한 로직을 분리해서 for문으로 돌리는게 좋지않을까요??
제가 생각한방식은 처음에 class_time관련된 부분은 -1구분자로 나눠서 2차원 배열로 만든 후에, class_time과 class_place중에 max_size를 알아낸 후에, 그 max_size크기 만큼 각각 class_time과 class_place에서 for문 돌리면서 쌍을 맞추는 방식은 어떨까요? (이 부분 무시하셔도 될것같습니다..)
일단 지금은 에러 수정사항이니 이 부분은 리팩토링으로 두면 어떨까 생각합니다!
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.
넵 리펙토링으로 남기겠습니다.