-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/#23 문항, 새끼문항 도메인 생성, 수정, 삭제 API 구현 #26
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
Conversation
seokbeom00
left a comment
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.
고생하셨습니다!
| private void validateByType(String answer, ProblemType problemType) { | ||
| if (answer.isBlank()) { | ||
| throw new InvalidValueException(ErrorCode.BLANK_INPUT_VALUE); | ||
| } | ||
| if (problemType == ProblemType.MULTIPLE_CHOICE) { | ||
| if (!answer.matches("^[1-5]*$")) { | ||
| throw new InvalidValueException(ErrorCode.INVALID_MULTIPLE_CHOICE_ANSWER); | ||
| } | ||
| } | ||
| if (problemType == ProblemType.SHORT_NUMBER_ANSWER) { | ||
| try { | ||
| int numericAnswer = Integer.parseInt(answer); | ||
| if (numericAnswer < 0 || numericAnswer > 999) { | ||
| throw new InvalidValueException(ErrorCode.INVALID_SHORT_NUMBER_ANSWER); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| throw new InvalidValueException(ErrorCode.INVALID_SHORT_NUMBER_ANSWER); | ||
| } | ||
| } |
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.
Answer 클래스로 분리하여 유효성 검증까지 도메인 레벨에서 수행하는거 너무 좋은 것 같습니다!
| int subject = practiceTestTag.getSubject().getIdCode(); // AA (과목) | ||
| int year = practiceTestTag.getYear() % 100; // YY (두 자리 연도) | ||
| int month = practiceTestTag.getMonth(); // MM (두 자리 월) | ||
| int DEFAULT_MODIFIED = 0; // 변형 여부 (0: 기본, 1: 변형) |
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.
변형 여부는 기존 모의고사인지, 저희가 변형한 문제인지를 구분하는 식별자일까요?
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.
맞습니다!
|
|
||
| // 중복되지 않는 ID 찾을 때까지 반복 | ||
| do { | ||
| sequence = SEQUENCE.getAndIncrement() % 1000; // 000~999 순환 |
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.
서버가 재시작되면 항상 1로 재시작하기 떄문에, UUID를 통한 난수로 고유한 값 보장 + 중복 가능성을 줄이는 것은 어떨까요?
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.
UUID를 사용하면 기존 13자리에서 UUID(36자리)를 포함하여 46자리가 됩니다.
중복 가능성을 줄이는 것보다 아래 2가지 이유로 조회 속도면에서 효율이 안좋을 것 같습니다.
- ID 인덱스 B-Tree의 크기가 커짐
- 문자열 비교 비용 증가함
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.
아, 저는 단순히 1부터 999까지 조회하는 부분만 UUID를 적용하자는 뜻이였습니다! substring(0, 3)을 이용하면 1부터 999까지 순환하는 방식보다 중복되지 않는 3자리를 찾는 것이 더 빠를 것이라고 생각했습니다!
| private final PracticeTestTagRepository practiceTestTagRepository; | ||
|
|
||
| @GetMapping("") | ||
| @Operation(summary = "모의고사 목록 조회") | ||
| public ResponseEntity<List<PracticeTestTagResponse>> getPracticeTestTags() { | ||
| List<PracticeTestTagResponse> responses = practiceTestTagRepository.findAll().stream() | ||
| .map(PracticeTestTagResponse::of) | ||
| .toList(); | ||
| return ResponseEntity.ok(responses); |
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.
단순 조회기 떄문에 로직이 무겁지 않아서, 서비스 레이어를 사용하지 않은 걸까요? 컨트롤러 레이어에서 바로 DB를 조회한 이유가 궁금합니다!
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.
조회 이외에 다른 로직이 없고 조회로직이라 트랜잭션이 필요하지 않아 굳이 서비스 레이어를 만들면 복잡도만 올릴 것이라 판단했습니다!
| public class AlreadyExistException extends BusinessException { | ||
| public AlreadyExistException(ErrorCode errorCode) { | ||
| super(errorCode); | ||
| } | ||
| } |
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.
자주 사용되는 계열의 에러 클래스를 추가적으로 생성해서 사용하는 방식의 장점이 있을까요? (진짜 몰라서)
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.
AlreadyExistException이 자주 생기는 예외 상황이어서 추가했습니다!
이미 존재하는 데이터일 때 명시적으로 예외를 보여줄 수 있는 장점이 있다고 생각했어요
| @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "problem_id") | ||
| @OrderBy("sequence ASC") | ||
| private List<ChildProblem> childProblems = new ArrayList<>(); |
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.
동일한 새끼문제 추가 방지를 위해 Set 자료구조를 사용하는 것은 어떨까요?
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.
출제자가 중복 등록할 일이 없다고 생각했어요! 화면에 바로 보이니까요. 그래서 중복 검사를 한다고 하면
만약에 같은 새끼문제 등록이 안된다고하면 equals와 hashcode 구현은 어떤 필드값으로 해야할까요..?
Id는 중복될 일이 현재 생성 로직상 없어서 내용으로 구분해야할 것 같아서요!
🌱 관련 이슈
📌 작업 내용 및 특이사항
Problem 애그리거트
ChildProblem 애그리거트
MapStruct 라이브러리를 통해 dto -> 객체 매퍼 구현
📝 테스트 사항
이후 작업 내용