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

[서지민] Good-Night-3rd-Hackathon-Backend 제출 #14

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

Lauiee
Copy link

@Lauiee Lauiee commented Aug 24, 2024

스프링부트로 구현했습니다.
부족한 실력으로 최대한 열심히 구현했습니다.

initial commit
소원등록 API 구현완료
soft delete방식 소원 삭제 구현 완료
소원 승인상태 변경 API 구현 완료
description받는 방식 RequestBody -> RequestParam 으로 변경
단일 소원 API구현 완료 일단 승인 상태만 조회 가능하게 만듬
테스트코드는 작성 못함
6. 댓글 등록 API 구현 완료
7. 댓글 삭제 API 구현 완료
8. 댓글 조회 API 구현 완료
9. 검색 API 구현 완료
@Lauiee Lauiee self-assigned this Aug 24, 2024
Comment on lines 61 to 66
@PatchMapping("/{wishId}/status")
public ResponseEntity<Wishes> updateWishStatus(@PathVariable("wishId") Long wishId,
@RequestParam("description") String description) {
Wishes updatedWish = wishService.updateWishStatus(wishId, description);
return new ResponseEntity<>(updatedWish, HttpStatus.OK);
}

Choose a reason for hiding this comment

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

저는 pathVariable, requestParam 조합보다는 그냥 requestBody에 다 넣어서 사용하는데, 취향차이이긴한데 고려해보면 좋을거 같아요

Copy link
Author

Choose a reason for hiding this comment

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

다소 단순한 요청은 pathVariable,requestParam조합으로 진행하고,
요청 자체가 좀 복잡해지거나 무거워지면 requestBody를 사용하고 있는데 혹시 그냥 통일하는게 나을까요???

서비스단을 넘기는 요청객체를 도메인 객체로 변환하여 전달
@crossorigin으로 컨트롤러 하나에만 적용되던 cors설정을 WebConfig를 생성해 전역으로 적용하게 변경
DTO객체 전달 -> 도메인 객체 전달
불필요한 인자는 제거하고 내부적으로 값 설정하게 수정
빠진 파일들 마저 커밋
불필요했던 메서드 제거 및 서비스 레이어는 도메인 객체만 알도록 수정,
객체 명 및 필드명 수정으로 인해 발생하는 DB관련 오류 수정
기존에 만들어져있는 전체 조회 메서드에서 이미 status도 @RequestParam으로 받고 그걸 조건으로 반환해주고 있었어서 불필요한 코드라 판단해서 제거
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants