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

feat: lecture, timetableLecture API v3 #1156

Merged
merged 64 commits into from
Jan 5, 2025

Conversation

Soundbar91
Copy link
Contributor

@Soundbar91 Soundbar91 commented Jan 3, 2025

🔥 연관 이슈

🚀 작업 내용

  • lecture와 timetableLecture의 숫자 배열 강의 시간을 (week, startTime, endTime)으로 분리했습니다.
  • timetableLecture의 커스텀 강의와 정규 강의 API (생성, 수정)를 분리했습니다.
  • V2에서 사용하는 롤백 API도 응답 DTO 수정을 했기 때문에 가져왔습니다.

💬 리뷰 중점사항

timetableLecture 버저닝을 하면서 lecture와 겹치는 부분이 존재해서, 같이 작업을 진행했습니다.

테이블 정규화 실패

초기 작업을 할 때 역정규화된 강의 시간 데이터를 다 정규화를 했지만, 중복 시간 체크 구현에서 코드가 너무 꼬이고 엔티티 조회도 너무 많이 발생해서 코드로 정규화를 진행했습니다.
중복 시간 체크는 코드로 정규화를 진행해도 구현이 안 되서 클라이언트에서 구현한 로직을 따라가야 할 것 같습니다.

역정규화 데이터 정규화 로직

해당 로직은 응답 dto (LectureInfoResponse, LectureResponseV3)에 존재합니다.

정규 강의(lecture)의 강의 시간은 -1 구분자가 없고 연속되는 자연수인 경우를 판별하면 되기 때문에 이를 확인해서 정규화를 진행했습니다.

커스텀 강의의 강의 시간은 -1 구분자가 있는 경우에는 다음 강의 장소로 넘어가고, -1 구분자가 나오기 이전 연속되는 자연수가 아닌 경우 다음 강의 장소로 넘어가면 안 되기 때문에 이를 확인해서 정규화를 진행했습니다.

class_time = [1, 2, 100, 101, -1, 200, 201], class_place = "장소 A, 장소 B"의 경우 다음과 같이 응답값을 보냅니다.

[
  {
    "week": 0,
    "start_time": 1,
    "end_time": 2,
    "place": "장소 A"
  },
  {
    "week": 1,
    "start_time": 100,
    "end_time": 101,
    "place": "장소 A"
  },
  {
    "week": 2,
    "start_time": 200,
    "end_time": 201,
    "place": "장소 B"
  }
]

정규 강의는 숫자 배열의 연속이 끊길 때 마다, 다음 강의 장소로 넘어갑니다.
class_time = [1, 2, 100, 101], class_place = "장소 A, 장소 B

[
  {
    "week": 0,
    "start_time": 1,
    "end_time": 2,
    "place": "장소 A"
  },
  {
    "week": 1,
    "start_time": 100,
    "end_time": 101,
    "place": "장소 B"
  }
]

정규화 데이터 역정규화

요청 값으로 들어오는 정규화된 데이터를 역정규화하는 로직은 요청 dto 내부에 존재합니다.
강의 시간 (start_time, end_time) 역정규화는 커스텀 강의에만 진행을 합니다. 정규 강의는 이미 lecture 테이블에 역정규화된 상태이기 때문이고, lecture 테이블에서 강의 시간을 참조해야 하기 때문입니다.
start_time를 1씩 증가시켜서 end_time 값과 동일해질 때 까지 리스트에 넣습니다. 이 작업은 기존 데이터들과 형식을 맞추기 위해서 진행했습니다. start_time = 13, end_time = 16 -> 13, 14, 15, 16
다음 강의 시간으로 넘어가기 전에 -1 구분자를 넣어줍니다. 해당 값은 timetable_lecture 테이블의 class_time에 저장합니다.

강의 장소 (place) 역정규화는 정규 강의와 커스텀 강의 둘 다 진행을 합니다. 강의 장소는 입력으로 들어온 place를 , 구분자로 연결합니다. 해당 값은 timetable_lecture 테이블의 class_place에 저장합니다.

마무리

정규화 <-> 역정규화 이 부분을 제외하면 v2와 유사한 로직입니다..! 리뷰 남겨주시면 감사하겠습니다.

@Soundbar91 Soundbar91 added 기능 새로운 기능을 개발합니다. Team User 유저 팀에서 작업할 이슈입니다 labels Jan 3, 2025
@Soundbar91 Soundbar91 self-assigned this Jan 3, 2025
Copy link

github-actions bot commented Jan 3, 2025

Unit Test Results

349 tests   348 ✔️  1m 29s ⏱️
  44 suites      1 💤
  44 files        0

Results for commit b43521a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@kwoo28 kwoo28 left a comment

Choose a reason for hiding this comment

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

정규화<->역정규화하는 과정 정말 까다로웠는데 수고 많으셨습니다.. 패키지나 api 분리도 깔끔하게 잘 작성하신것같습니다!

Comment on lines 53 to 63
// 커스텀 강의 장소 역정규화
public String joinClassPlaces() {
StringBuilder classPlaces = new StringBuilder();
for (int index = 0; index < lectureInfos.size(); index++) {
if (index > 0) {
classPlaces.append(", ");
}
classPlaces.append(lectureInfos.get(index).place());
}
return classPlaces.toString();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

C

람다식으로 변경할 수 있을것같은데 혹시 이렇게하는거 어떨까요?
문법 참고 주소 : https://velog.io/@songyuheon/Java-Stream-collectCollectors.joining

Suggested change
// 커스텀 강의 장소 역정규화
public String joinClassPlaces() {
StringBuilder classPlaces = new StringBuilder();
for (int index = 0; index < lectureInfos.size(); index++) {
if (index > 0) {
classPlaces.append(", ");
}
classPlaces.append(lectureInfos.get(index).place());
}
return classPlaces.toString();
}
public String joinClassPlaces() {
return lectureInfos.stream()
.map(LectureInfoRequest::place)
.collect(Collectors.joining(", "));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오오,, 세 줄로 줄었네욥..! 반영하겠습니다

Comment on lines 55 to 57
if (index + 1 > classPlaces.size()) {
addLectureInfo(response, startTime, endTime, EMPTY_PLACE);
}
Copy link
Contributor

@kwoo28 kwoo28 Jan 4, 2025

Choose a reason for hiding this comment

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

A

해당 조건문은 시간은 2개인데, 강의장소는 1개일때 해당하는 부분 맞나요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 맞습니다 ! 강의 장소 수정이 이뤄졌는데, 한 장소만 수정이 됐을 경우 NPE가 발생하기 때문에 걸었습니다.
클라이언트에서 강의 장소를 추가하지 않은 경우 ""으로 보내주겠지만, 만약을 위해 추가했습니다.

prevTime = time;
}

if (!Objects.isNull(startTime)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A

조건문이 있다는건 startTime이 null값이 될 수 있는 경우가 있다는건데, 여기서 startTime이 null값이 되는 경우는 어떤 상황인가요?? 위에 로직부터 계속 봤는데 어떤 경우에 startTime이 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.

이 코드가 mysql 프로시저를 코드로 옮긴 내용이라, 다시 코드를 작성해서 푸시했습니다 !
47d93b1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

커스텀 강의도 변경했습니다.
3ba3955

Copy link
Contributor

@duehee duehee left a comment

Choose a reason for hiding this comment

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

어우 작업의 신..
코멘트 몇 개 남겼으니 한 번 확인해주세요! 고생하셨습니다~

@@ -114,4 +114,18 @@ public void delete() {
public void undelete() {
this.isDeleted = false;
}

public void customLectureUpdate(String classTitle, String professor, String classTime, String classPlace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

C

메소드명을 updateCustiomLecture로 하는 건 어떨까요? "동사 + 명사" 형태를 더 많이 사용해서요!
-> 아래 updateRegularLecture도 동일해요!

Suggested change
public void customLectureUpdate(String classTitle, String professor, String classTime, String classPlace) {
public void updateCustomLecture(String classTitle, String professor, String classTime, String classPlace) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영하겠습니다 !! 감사합니다

@Schema(description = "종료 시간", example = "115", requiredMode = REQUIRED)
Integer endTime,

@Schema(description = "장소", example = "2공학관314", requiredMode = NOT_REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

C

기존에서는 TEXT로 받았는데, 문제 없을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

각 문자열을 최대 30으로 받고, 하나의 문자열로 역 정규화를 진행해서 TEXT 범위 이내로 들어옵니다 !

@JsonNaming(value = SnakeCaseStrategy.class)
public record LectureInfoResponse(
@Schema(description = "요일 id", example = "0", requiredMode = REQUIRED)
Integer week,
Copy link
Contributor

Choose a reason for hiding this comment

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

A

저는 이거 처음 볼 때 주(한 주)로 해석하는 여지가 좀 생겼는데, 영어 표현은 day of the week라 너무 길고, weekday나 day같은 표현은 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

day가 좋아 보입니다 ! 피드백 감사합니다.

List<Integer> classTimes = parseToIntegerList(classTime);
List<LectureInfoResponse> response = new ArrayList<>();

// 온라인 강의인 경우
Copy link
Contributor

Choose a reason for hiding this comment

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

A

👍

.orElseThrow(() -> LectureNotFoundException.withDetail("lecture_id: " + id));
}

List<Lecture> findBySemester(String semesterDate);
Copy link
Contributor

Choose a reason for hiding this comment

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

A

find끼리 묶어서 두면 좋을 거 같아요!!!

Comment on lines +14 to +15
@Service
@RequiredArgsConstructor
Copy link
Contributor

Choose a reason for hiding this comment

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

C

👀

Suggested change
@Service
@RequiredArgsConstructor
@Service
@RequiredArgsConstructor
@Transactional(readOnly = true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉,,,,,,, 감사합니다

Comment on lines +23 to +24
@Service
@RequiredArgsConstructor
Copy link
Contributor

Choose a reason for hiding this comment

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

C

Suggested change
@Service
@RequiredArgsConstructor
@Service
@RequiredArgsConstructor
@Transactional(readOnly = true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ㅇ.ㅇ

"regular_number": "22",
"department": "기계공학부",
"target": "기공1",
"professor": "박한수,최준호",
Copy link
Contributor

Choose a reason for hiding this comment

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

A

어느새 교수자리까지

Copy link
Contributor Author

Choose a reason for hiding this comment

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

대단한 분들이십니다..!

@Soundbar91 Soundbar91 requested a review from duehee January 5, 2025 08:51
Copy link
Contributor

@duehee duehee left a comment

Choose a reason for hiding this comment

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

@Soundbar91 Soundbar91 merged commit 8c9cba9 into develop Jan 5, 2025
4 checks passed
@Soundbar91 Soundbar91 deleted the feature/1148-timetable-lecture-v3 branch January 5, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team User 유저 팀에서 작업할 이슈입니다 기능 새로운 기능을 개발합니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lecture, timetablelecture api v3
3 participants