-
Notifications
You must be signed in to change notification settings - Fork 29
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
[김보배] Good-Night-3rd-Hackathon-Backend 제출 #15
base: main
Are you sure you want to change the base?
Conversation
private LocalDate created_at; | ||
private String is_confirm = WishConfirmState.PENDING.getKorean(); | ||
private LocalDateTime deleted_at; |
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.
@Kimbobae1 자바에서는 컨벤션이 CamelCase입니다. 위에는 Camel 여기 3개 컬럼은 snake case로 사용했는데 자바 공식 컨벤션 맞춰야될거 같습니다.
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.
컨벤션을 잘 숙지하지 못하고 코드를 작성했습니다😭
CamelCase로 수정하겠습니다!
컨벤션 잘 확인하고 손에 익도록 하겠습니다..!
import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
import org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration; | ||
|
||
@SpringBootApplication(exclude = {DataSourceAutoConfiguration.class}) |
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.
@Kimbobae1 이거는 datasource 아직 세팅안해서 어플리케이션 안떠서 이렇게한건가요?
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.
API를 구현할 때 데이터베이스를 연결하지 않았는데 사진과 같은 오류가 발생했습니다.
찾아보니 application.properties 에 데이터베이스 연결정보가 없는 상태에서
Spring boot가 데이터베이스 연결을 하려고 해서 발생하는 문제인 것 같았습니다.
그래서 Spring boot가 데이터베이스 설정을 건너뛸 수 있도록 했습니다.
정확하게 알고 작성한 것이 아니라서 더 좋은 방법이 있을지 찾아보겠습니다! 😀
@PostMapping("/comments/{wishId}") | ||
public void saveComment(@RequestBody CommentDto commentDto, @PathVariable Long wishId) { | ||
commentService.saveComment(commentDto, wishId); | ||
} | ||
|
||
@GetMapping("/comments/{wishId}") |
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.
comment에 대한 컨트롤러이기 때문에 wishId가 pathVariable로 들어가는건 적절해보이지 않습니다. requestBody안으로 들어가면 괜찮을거 같아요
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.
제공해주신 참고문서 읽고 wishId가 requestBody로 들어가도록 수정하였습니다.
REST API에 대해 더 공부해야 할 것 같습니다. 🥲
감사합니다!
public void saveWish(@RequestBody WishDto.SaveDto wishDto) { | ||
wishService.saveWish(wishDto); |
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.
WishDto
에 static으로 사용하기 보다 CreateWithRequestDto
와 같이 클래스 분리하는게 좋을거 같습니다.
현업에서는 static class를 잘 사용하진 않습니다.
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.
Dto를 여러개 만들어야 할 때 어떻게 하는게 좋을지 고민했었는데
말씀해주신대로 클래스를 따로 분리하는 방법이 좋은 것 같습니다 😀
수정하겠습니다!
@PutMapping("/wishs/{id}") | ||
public void confirmWishById(@PathVariable Long id, @RequestBody WishDto.ConfirmDto wishDto) { | ||
wishService.confirmWishById(id, wishDto); | ||
} |
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.
wishService는 비즈니스 레이어이고, wishDto는 presentation layer인데,넘길때 dto가 그대로 넘어가기 보다는 값만 넘기도 될거같아요
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.
말씀해주셔서 이번에 처음 알게 되었습니다, 감사합니다!
Controller에서 받은 Dto로 Wish 객체를 만들고 그 객체를 Service로 넘겨주는 것으로 수정했습니다. 😊
private Long comment_id; | ||
private Long wish_id; | ||
private String content; | ||
private LocalDate created_at; | ||
private boolean is_deleted = false; |
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.
camelcase로 수정필요
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.
넵, 수정했습니다! 😄
@Data | ||
public class CommentDto { | ||
private String content; | ||
private LocalDate created_at; |
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.
여기도 camelCase
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.
수정했습니다! 😊
|
||
import java.time.LocalDate; | ||
@Data | ||
public class WishDto { |
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.
개인적으로는 static class를 사용안하고, 클래스 분리를 하면 좋을거 같습니다. 저도 static class로 dto를 매핑하는건 처음봤는데, 취향의 문제이긴합니다.
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 MemoryCommentRepository implements CommentRepository { | ||
private static Map<Long, Comment> memory = new HashMap<>(); | ||
private Long index = 0L; | ||
|
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 void saveComment(CommentDto commentDto, Long wishId) { | ||
Comment comment = new Comment(); | ||
comment.setWish_id(wishId); | ||
comment.setContent(commentDto.getContent()); | ||
comment.setCreated_at(LocalDate.now()); | ||
commentRepository.save(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.
Comment.ofCreate(wishId, content, LocalDate.now)
와 같이 정적팩토리메서드를 사용하면 가독성이 더 좋을거 같습니다.
그리고 WisthDto를 전달하기 보다 컨트롤러에서 서비스 레이어로 넘길때 인자를 전달하거나 별도 객체를 만들어서 전달하면 좋을거 같아요.
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.
말씀해주신대로 정적팩토리메서드를 만들어 사용하는 것으로 수정했습니다!
이렇게 하니 사용하기도 편하고 가독성도 훨씬 좋아졌습니다 👍👍
그리고 Dto를 넘겨주는 것에서 Wish 객체를 만들어 넘겨주는 것으로 수정했습니다!
CommentDto commentDto = new CommentDto(); | ||
commentDto.setContent(comment.getContent()); | ||
commentDto.setCreated_at(comment.getCreated_at()); |
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.
이부분도 정적팩토리메서드를 만들어 사용하는 것으로 수정했습니다!
Wish wish = new Wish(); | ||
wish.setTitle(wishDto.getTitle()); | ||
wish.setContent(wishDto.getContent()); | ||
wish.setCategory(wishDto.getCategory()); | ||
wish.setCreated_at(LocalDate.now()); | ||
wishRepository.save(wish); |
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.
이 부분도 정적팩토리 메서드 만들어서 사용하였습니다! 😊
} | ||
|
||
public void deleteWishById(Long id) { | ||
Wish targetWish = wishRepository.findById(id); |
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.
헉 이부분을 생각하지 못했었네요,, 예외처리 부분 추가하였습니다!
@Kimbobae1 전체적으로 자바 컨벤션이랑 Layer끼리 참조한느게 잘 안맞는거 같아요. 그리고 테스트코드 작성하면 좋을거 같아요 |
|
||
@Repository | ||
public class MemoryCommentRepository implements CommentRepository { | ||
private static Map<Long, Comment> memory = new HashMap<>(); |
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.
동시성때문에 CocurrentHashMap을 사용해야합니다.
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.
이번에 ConcurrentHashMap과 AtomicLong 에 대해서 처음 알게 되었습니다, 감사합니다! 동시성과 같은 중요하고 세세한 부분까지 신경쓰도록 노력하겠습니다 😊
@Repository | ||
public class MemoryCommentRepository implements CommentRepository { | ||
private static Map<Long, Comment> memory = new HashMap<>(); | ||
private Long index = 0L; |
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.
index도 동시성때문에 AtomLong을 사용해야합니다.
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.
이부분도 수정했습니다!
Spring boot로 구현했습니다.
추가 기능 구현, DB 사용은 아직 안해서 시간 나는대로 구현해보겠습니다.
많이 부족합니다, 피드백 주시면 최대한 반영해서 수정해보겠습니다..!