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

refactor: 해시태그에 카테고리 적용 완료 #647

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

Conversation

mingmingmon
Copy link
Collaborator

@mingmingmon mingmingmon commented Dec 26, 2024

Summary

#646

20가지 정도 되던 해시태그를 분류할 수 있도록 카테고리를 생성해서 관리하도록 했습니다.
@Jeong-Ag 님이 UX적인 요소를 고려해서 제안해 주신 내용입니다.

Tasks

  • 해시태그 카테고리 설정하기

SQL

DELETE FROM hashtag;
ALTER TABLE hashtag
ADD COLUMN hashtag_category VARCHAR(50) NOT NULL DEFAULT 'ETC';
INSERT INTO hashtag (name, category, is_deleted, board_usage, created_at, updated_at) VALUES

('C', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('C++', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Java', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Javascript', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Python', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Assembly', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Kotlin', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Typescript', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),


('Front-end', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Back-end', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('AI', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Game', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Android', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('IOS', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),


('DB', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Devops', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Infra', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('framework', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('algorithm', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),

('기타', 'ETC', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP);

Screenshot

image

@mingmingmon mingmingmon added the 🔨 Refactor 코드 수정 및 개선 label Dec 26, 2024
@mingmingmon mingmingmon self-assigned this Dec 26, 2024
@mingmingmon mingmingmon requested a review from limehee as a code owner December 26, 2024 12:00
@mingmingmon mingmingmon linked an issue Dec 26, 2024 that may be closed by this pull request
1 task
Copy link
Collaborator

@limehee limehee left a comment

Choose a reason for hiding this comment

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

현재 제시해주신 DDL에서 hashtag_category 컬럼을 VARCHAR(50)로 선언하고 있어요. 하지만, HashtagCategory는 enum으로 정의되어 있기 때문에, 데이터베이스 레벨에서 enum 값을 강제하는 것이 더 적절하다고 생각해요. 이렇게 하면 데이터 무결성을 유지하고, 허용되지 않은 값이 들어가는 것을 방지할 수 있어요.

개선된 DDL 제안

-- Enum 타입 생성
CREATE TYPE hashtag_category_type AS ENUM ('LANGUAGE', 'FIELD', 'SKILL', 'ETC');

-- 컬럼 추가
ALTER TABLE hashtag
ADD COLUMN hashtag_category hashtag_category_type NOT NULL DEFAULT 'ETC';


@Getter
@Setter
public class HashtagRequestDto {

List<String> hashtagNames = new ArrayList<>();
private String name;
private HashtagCategory hashtagCategory;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Table(name = "hashtag")
public class HashtagJpaEntity extends BaseEntity {

    @Column(name = "name", unique = true, nullable = false)
    private String name;

    @Column(name = "hashtag_category", nullable = false)
    @Enumerated(EnumType.STRING)
    private HashtagCategory hashtagCategory;
}

현재 hashtag 테이블의 namehashtag_category 컬럼은 nullable = false로 설정되어 있어, null 값을 허용하지 않아요. 이에 따라, 관련 DTO 클래스에서도 해당 필드에 대해 @NotNull 애너테이션을 추가하는 것이 적절해 보여요.

해시태그 생성 API에서 null 값을 포함한 요청이 들어올 경우 500 Server Error가 발생하고 있어요. @NotNull 애너테이션을 추가하면 이러한 예외 상황을 사전에 방지하고, 프론트 팀에서 보다 명확한 오류 메시지를 받을 수 있을 거예요.

Copy link
Collaborator

@limehee limehee left a comment

Choose a reason for hiding this comment

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

public class BoardRequestDto {
    @Schema(description = "해시태그 id 리스트", example = "[1, 2]")
    private List<Long> hashtagIdList;
}

추가로, 현재 게시글 등록 시 해시태그의 ID를 정수로 전달받도록 설계되어 있어요. 하지만, 이를 해시태그의 이름으로 전달받도록 변경하는 것을 권장드려요.

ID로 해시태그를 전달받을 경우, 테이블 데이터 변경이나 데이터 재등록과 같은 상황에서 ID 값이 변경될 가능성이 있고, 이는 서버 오류로 이어질 수 있어요. 또한, 프론트 팀과의 약속대로 해시태그의 ID 값이 고정되지 않는다면 예상치 못한 결과를 초래할 수 있어요.

해시태그 ID 대신 이름으로 전달받게 하면 데이터 무결성을 유지하면서, 변경 가능성을 최소화할 수 있어요. 해당 설계 변경에 대해 검토 부탁드려요.

@limehee limehee requested a review from Copilot December 26, 2024 13:02

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 19 changed files in this pull request and generated no comments.

Files not reviewed (10)
  • src/main/java/page/clab/api/domain/community/hashtag/application/service/HashtagRegisterService.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/domain/HashtagCategory.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/domain/Hashtag.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/adapter/in/web/HashtagRegisterController.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/application/dto/mapper/HashtagDtoMapper.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/application/dto/request/HashtagRequestDto.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/application/dto/response/HashtagResponseDto.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/hashtag/application/port/in/RegisterHashtagUseCase.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/board/application/dto/mapper/BoardDtoMapper.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/community/board/application/service/HotBoardRetrievalService.java: Evaluated as low risk
Copy link
Contributor

@SongJaeHoonn SongJaeHoonn left a comment

Choose a reason for hiding this comment

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

limehee 님께서 제안하신 DDL 개선점에 대해 현재 enum으로 카테고리가 관리되어 있기에 크게 동의하고, 수정된 코드에서 문제점은 없어보입니다! 고생하셨어요!!

@mingmingmon mingmingmon reopened this Dec 28, 2024
@mingmingmon
Copy link
Collaborator Author

mingmingmon commented Dec 29, 2024

@limehee 님이 제안주신 내용에 따라 아래와 같이 SQL문을 적용하도록 하겠습니다!

수정된 SQL문

CREATE TYPE hashtag_category_type AS ENUM ('LANGUAGE', 'FIELD', 'SKILL', 'ETC');
ALTER TABLE hashtag
ADD COLUMN hashtag_category hashtag_category_type NOT NULL DEFAULT 'ETC';```
INSERT INTO hashtag (name, category, is_deleted, board_usage, created_at, updated_at) VALUES

('C', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('C++', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Java', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Javascript', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Python', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Assembly', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Kotlin', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Typescript', 'LANGUAGE', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),


('Front-end', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Back-end', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('AI', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Game', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Android', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('IOS', 'FIELD', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),


('DB', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Devops', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('Infra', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('framework', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),
('algorithm', 'SKILL', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP),

('기타', 'ETC', false, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP);

@mingmingmon mingmingmon requested a review from limehee December 29, 2024 05:33
@@ -9,5 +10,7 @@
public class HashtagRequestDto {

private String name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

name 컬럼도 null을 허용하지 않고 있기 때문에, @NotNull 애너테이션 추가가 필요할 것 같아요.

return BoardHashtagRequestDto.builder()
.boardId(boardId)
.hashtagIds(hashtagIdList)
.hashtagNameList(hashtagNameList)
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 프로젝트에서는 List라는 자료형 명칭 대신 복수형(s)을 사용하도록 규정하고 있어요.

@limehee
Copy link
Collaborator

limehee commented Dec 29, 2024

@limehee 님이 제안주신 내용에 따라 아래와 같이 SQL문을 적용하도록 하겠습니다!

PR에 기재된 쿼리문도 수정 부탁드려요.

@@ -65,9 +65,9 @@ public String registerBoard(BoardRequestDto requestDto) throws PermissionDeniedE

Board savedBoard = registerBoardPort.save(board);

if (requestDto.getHashtagIdList() != null) {
if (requestDto.getHashtagNameList() != null) {
Copy link
Collaborator

@limehee limehee Dec 29, 2024

Choose a reason for hiding this comment

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

불필요한 연산 및 데이터 처리를 방지하기 위해, 리스트가 비어 있지 않은지 검사하는 과정을 추가해야 될 것 같아요.

제안

if (requestDto.getHashtagNameList() != null && !requestDto.getHashtagNameList().isEmpty())

또는

import org.springframework.util.CollectionUtils;

if (!CollectionUtils.isEmpty(requestDto.getHashtagNames()))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 Refactor 코드 수정 및 개선
Projects
None yet
Development

Successfully merging this pull request may close these issues.

해시태그에 카테고리 적용하기
3 participants