Skip to content

refactor: 시간표 등록시 학점 정보 추가 완료 #19

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.

Sign up for GitHub

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

Open
SongJaeHoonn wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

refactor: 시간표 등록시 학점 정보 추가 완료 #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89bf0d1
refactor(LectureSelectResponseDto): credit 정보 추가
SongJaeHoonn Sep 4, 2024
3d3c696
refactor(Lecture): 파일 끝 개행 추가
SongJaeHoonn Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/page/time/api/domain/lecture/application/LectureRetrieveService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,4 @@ public List<LectureSelectResponseDto> retrieveSelectedLecturesByIds(List<Long> l
.map(LectureSelectResponseDto::toDto)
.toList();
}
}
}
2 changes: 1 addition & 1 deletion src/main/java/page/time/api/domain/lecture/dao/LectureRepositoryCustom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ public interface LectureRepositoryCustom {
List<Lecture> findByFilter(List<String> campuses, List<Type> types, List<Integer> grades, List<String> days, List<String> times, List<String> majors, Boolean isExceeded, String lectureName, Long cursor, int limit);

List<String> findByMajor(String major);
}
}
2 changes: 1 addition & 1 deletion src/main/java/page/time/api/domain/lecture/dao/LectureRepositoryCustomImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,4 +139,4 @@ private BooleanExpression gtCursor(Long cursor) {
}
return lecture.id.gt(cursor);
}
}
}
4 changes: 3 additions & 1 deletion src/main/java/page/time/api/domain/lecture/dto/response/LectureSelectResponseDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public class LectureSelectResponseDto {
private String professor;
private String room;
private String time;
private Integer credit;
Copy link
Contributor

@limehee limehee Sep 4, 2024

Choose a reason for hiding this comment

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

credit이라는 단어는 맥락에 따라 다양한 의미를 가질 수 있기 때문에, 코드에서 사용될 때 직관적으로 이해하기 어려울 것 같아요. 학교 시스템 같은 환경에서는 이러한 단어를 한글 발음 그대로 영어로 옮기는 방식(hakjeom 등)이 종종 사용되는 것을 볼 수 있어요. 외에 토스 라이브러리에서도 이러한 방식이 사용된 것을 보았고, 해당 코드에서도 사용자의 이해를 돕기 위해 변수명을 hakjeom처럼 발음 그대로 사용하는건 어떨까 싶어요.

토스 라이브러리 한글 발음 변수명 사용 사례

https://github.com/toss/es-hangul/blob/main/src/_internal/hangul.ts

  if (
    hasBatchim(source, {
      only: 'single',
    }) &&
    canBeJongseong(`${lastConsonant}${nextCharacter}`)
  ) {
    return combineJongseong(`${lastConsonant}${nextCharacter}`);
  }

이거는 제 의견일 뿐 이렇게 해보는건 어떤지에 대한 제안입니다.
해당 내용에 대해 @mingmingmon 님의 의견도 궁금해요.

Copy link
Author

@SongJaeHoonn SongJaeHoonn Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

DB의 credit을 hakjeom으로 바꾸게 된다면 다른 헷갈리는 필드(type, category, groupName 등)도 변경하는 것이 좋을 것 같아요.

하지만 프론트엔드에서도 코드를 수정해야 한다는 불편함이 있고, 과목 정보를 스크래핑 할때도 코드를 수정해야 해서, 저도 @mingmingmon 님과, @SWARVY 님의 의견을 들어보는 것이 좋을 것 같습니다.

Copy link

@SWARVY SWARVY Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

해당 문제의 경우 프론트엔드에서도 동일하게 발생합니다. 백엔드에서 제공하는 명세서를 바탕으로 타입설계를 하기 때문입니다

하여, 표현이 모호한 변수명들을 바꾸는 것은 프론트엔드의 코스트가 발생하지만 유지보수적 관점으로 보았을 때 명확한 표현으로 교체하는것이 장기적으로 더 좋아보입니다.

바꾸게되면 변경사항 알려주세요 👍🏻

Copy link
Collaborator

@mingmingmon mingmingmon Sep 4, 2024

Choose a reason for hiding this comment

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

한글 발음 나는대로 변수명을 바꾸는 것에 동의합니다.
다만 코드 수정이 제법 일어날 것 같아서 수정 후에 변경사항 알려드릴게요!

Copy link
Author

@SongJaeHoonn SongJaeHoonn Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

작업의 명확한 분리를 위해 변수명 변경건은 현재 PR 머지 후 새로운 이슈에서 작업 진행하도록 하겠습니다.


public static LectureSelectResponseDto toDto(Lecture lecture) {
return LectureSelectResponseDto.builder()
Expand All @@ -28,6 +29,7 @@ public static LectureSelectResponseDto toDto(Lecture lecture) {
.professor(lecture.getProfessor())
.room(lecture.getRoom())
.time(lecture.getTime())
.credit(lecture.getCredit())
.build();
}
}
}