Skip to content

과제 1-1 976520 과제제출 #4

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

과제 1-1 976520 과제제출 #4

wants to merge 15 commits into from

Conversation

976520
Copy link
Member

@976520 976520 commented Feb 21, 2025

전공이 백엔드가 아니기도 하고, 태은짱 바쁜거같애서 안하려다가 리펙토링/최적화를 공부하거나 트러블 슈팅 과정이 재밌어서 무지성으로해보았는데, 더 정확한 조언을 듣고자 PR 올려봅니다..

Copy link
Member

@snowykte0426 snowykte0426 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다🎉

그래도 경력직(?) 이라고 코드 퀄리티는 가장 높네요...
백엔드 전공이 아닌 사람이 백엔드 전공들보다 높으면 안되는데

Service 전체에 비동기 처리를 활성화 해놓으셨던데
데이터가 수만개가 넘어가는 상황에서 findAll 요청이 아닌이상 필요없는 기술 도입 같긴 합니다
오히려 코드만 복잡해지고 차이는 거의 없을 것 같은데...한번 비교해보시고 결정해주세요

Comment on lines 17 to 19
public ArticleController(ArticleService articleService) {
this.articleService = articleService;
}
Copy link
Member

Choose a reason for hiding this comment

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

Lombok의 @RequiredArgsConstructor를 클래스에 적용하면 이런 생성자 같은 보일러플레이트 코드를 확 줄일 수 있으니 참고해주세요

Comment on lines 20 to 22
public ArticleService(ArticleRepository articleRepository) {
this.articleRepository = articleRepository;
}
Copy link
Member

Choose a reason for hiding this comment

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

여기도 보일러플레이트 코드를 줄여주세요

Comment on lines 33 to 43
@PostMapping
public CompletableFuture<ResponseEntity<Article>> createArticle(@RequestBody Article article) {
return articleService.createArticle(article).thenApply(ResponseEntity::ok);
}

@PatchMapping("/{articleId}")
public CompletableFuture<ResponseEntity<Article>> updateArticle(@PathVariable Long articleId, @RequestBody Article articleDetails) {
return articleService.updateArticle(articleId, articleDetails)
.thenApply(article -> article.map(ResponseEntity::ok)
.orElseGet(() -> ResponseEntity.notFound().build()));
}
Copy link
Member

Choose a reason for hiding this comment

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

RequestBody로 Entity가 사용되었는데 Entity 노출 문제는 둘째치고 이러면 값 검증이 어려워집니다@Vaild 어노테이션과 Jakarta 관련 어노테이션을 동원하여 값 검증을 수행할 수 있는 별도의 Request DTO를 만들고 그 DTO를 이용해서 값을 전달하게 만드는게 좋을 것 같습니다

Comment on lines +45 to +49
@DeleteMapping("/{articleId}")
public CompletableFuture<ResponseEntity<Void>> deleteArticle(@PathVariable Long articleId) {
return articleService.deleteArticle(articleId)
.thenApply(deleted -> deleted ? ResponseEntity.noContent().build() : ResponseEntity.notFound().build());
}
Copy link
Member

Choose a reason for hiding this comment

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

보통 DELETE 요청의 대한 응답에는 메세지 Body가 없습니다!(204 No Content)
클라이언트에 true false로 알려주기 보단 비즈니스 로직 중간에서 예외를 throw 하고 그걸 GlobalExceptionHandler에서 캐치해서 응답을 생성하는 방식으로 클라이언트에 처리결과를 알려주고 성공하면 204를 줍니다

Spring에서 ResponseEntity의 status를 204로 설정하거나 그냥 메서드 반환형을 void로 하고 @ResponseStatus 어노테이션을 적용하면 됩니다

@snowykte0426 snowykte0426 changed the title Task/jw 과제제출 과제 1-1 976520 과제제출 Feb 21, 2025
Copy link
Member

@snowykte0426 snowykte0426 left a comment

Choose a reason for hiding this comment

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

누워있다가 일어나 보니 Commit이 있자나???

비즈니스 로직을 담당하는 클래스 인터페이스-클래스로 나누란게 저런 형태를 말하는게 아니긴 했는데 더 고수준의 설계를 가져와서 할말이 없네

Comment on lines 1 to 38
package com.gsm._8th.class4.backed.task._1._1.domain;

import com.gsm._8th.class4.backed.task._1._1.global.entity.BaseIdxEntity;

import jakarta.persistence.Entity;
import jakarta.persistence.Table;
import lombok.AccessLevel;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Entity
@Table(name = "articles")
public class Article extends BaseIdxEntity {
private String title;
private String content;

@Builder
public Article(String title, String content) {
this.title = title;
this.content = content;
}

public void update(String title, String content) {
this.title = title;
this.content = content;
}

public void setTitle(String title) {
this.title = title;
}

public void setContent(String content) {
this.content = content;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Entity는 Domain 보단 그냥 별도의 Entity 패키지를 만드는게 좋을 듯 합니다

Comment on lines 26 to 37
public void update(String title, String content) {
this.title = title;
this.content = content;
}

public void setTitle(String title) {
this.title = title;
}

public void setContent(String content) {
this.content = content;
}
Copy link
Member

Choose a reason for hiding this comment

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

이런 도우미 메서드는 그냥 Setter 사용 아닌가요?
Setter는 죄악입니다ㅎㅎ...
이런 방식보단 다른 방식을 찾아보시면 좋겠습니다

Comment on lines 7 to 27
public ArticleDTO(String title, String content) {
this.title = title;
this.content = content;
}

public String getTitle() {
return title;
}

public void setTitle(String title) {
this.title = title;
}

public String getContent() {
return content;
}

public void setContent(String content) {
this.content = content;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Lombok의 ``@Getter`같은 어노테이션으로 보일러 플레이트 코드를 줄여주세요

그리고 DTO에서의 Setter 사용은 Entity에서 보단 용인되기도 하는데...그래도 이왕이면 사용을 지양하여 주세요

Copy link
Member

@snowykte0426 snowykte0426 left a comment

Choose a reason for hiding this comment

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

진짜ㅣ 다 왔다ㅏ

Comment on lines +1 to +23
package com.gsm._8th.class4.backed.task._1._1.util;

import java.util.List;
import java.util.Optional;

public class ExceptionHandlerUtil {

public static <T> T handleException(Throwable ex, T defaultValue) {
return defaultValue;
}

public static <T> Optional<T> handleOptionalException(Throwable ex) {
return Optional.empty();
}

public static <T> List<T> handleListException(Throwable ex) {
return List.of();
}

public static Boolean handleBooleanException(Throwable ex) {
return false;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

오...GlobalExceptionHandler를 쓰지 말란 말이 아니였는데 사실 이렇게 해도 기능은,,,뭐 통일성 있게 작동하니까 여기까진 선택의 영역으로...

Comment on lines +57 to +69
if (articleDTO.getTitle() == null || articleDTO.getTitle().isBlank()) {
throw new IllegalArgumentException("Title에 뭐라도 써주세요...");
}
if (articleDTO.getTitle().length() > 100) {
throw new IllegalArgumentException("Title은 100자를 넘을 수 없습니다.");
}
if (articleDTO.getContent() == null || articleDTO.getContent().isBlank()) {
throw new IllegalArgumentException("Content에 뭐라도 써주세요...");
}
if (articleDTO.getContent().length() > 1000) {
throw new IllegalArgumentException("Content는 1000자를 넘을 수 없습니다.");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Jakarta의 @SiZe@nonblank,@NotNull 등을 쓰면 DTO 단에서 거르고 메서드를 만들기 까지는 필요가 없을 수도 있을거 같습니다

Comment on lines +11 to +25
public String getTitle() {
return title;
}

public void setTitle(String title) {
this.title = title;
}

public String getContent() {
return content;
}

public void setContent(String content) {
this.content = content;
}
Copy link
Member

Choose a reason for hiding this comment

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

@Setter,@Getter적용해서 보일러플레이트 코드 제거 ㄱㄱ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants