-
Notifications
You must be signed in to change notification settings - Fork 3
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
[BE] feat: 발자국 - image URL 필드 추가, 쿨타임 검증 추가, 마지막 발자국 조회 기능 구현 #122
Conversation
- 요청에서 Member 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.
리뷰 남겼어요~~ 고생하셨습니다 트레!!
|
||
@PostMapping("/image/{footprintId}") | ||
public ResponseEntity<UpdateFootprintImageResponse> updateFootprintImage( | ||
@PathVariable Long footprintId, | ||
@ModelAttribute UpdateFootprintImageRequest request | ||
) { |
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.
해당 핸들러는 이미지 업데이트를 위한 것 같은데
@PostMapping을 사용한 이유가 있을까요?
@PathVariable이랑 ModelAttribute를 같이 받기 보단 하나의 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.
patch로 바꿔 봐도 좋겠네요.
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.
+파일을 받으려면 ModelAttribute를 써야 한다고 하더라구요. 이 부분은 S3 연동 잘 되는지에 따라 결정해야 할 것 같아요.
import lombok.Builder; | ||
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Entity | ||
@Getter | ||
@NoArgsConstructor | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) |
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 setImageUrl(String imageUrl) { | ||
this.imageUrl = imageUrl; | ||
} |
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.
해당 메서드는 jpa 변경감지를 위해 만들어진 setter같은데,
변경 감지를 위해 다른 로직이 들어 갈 가능성도 있으니, 기존의 setter와 목적과 의도가 조금 다르다고 생각합니다. update~ 또는 change~ 와 같은 형태로 메소드명을 통일하면 어떨까 싶어요
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.
동의합니다 👍
@NotNull | ||
Long memberId, |
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.
이왕이면 @positive 검증도 해주면 좋겠네요
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.LocalDateTime; | ||
|
||
public record FindMyLatestFootprintTimeResponse( | ||
@JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss") LocalDateTime createdAt |
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.
TODO 써봐도 좋겠네요.
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
@Service | ||
@Transactional | ||
@RequiredArgsConstructor |
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 validateRecentFootprintExists(Long memberId) { | ||
boolean exists = footprintRepository.existsByMemberIdAndCreatedAtAfter( | ||
memberId, | ||
LocalDateTime.now().minusSeconds(FootprintCommandService.FOOTPRINT_COOLDOWN) | ||
); | ||
|
||
if (exists) { | ||
throw new FriendoglyException("마지막 발자국을 찍은 뒤 30초가 경과되지 않았습니다."); | ||
} | ||
} |
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.
해당 로직은 foorPrint 도메인 내부에 내려줄 수 있을 것 같은데 어떠신가요?
LocalDateTime startTime = LocalDateTime.now().minusHours(FOOTPRINT_DURATION_HOURS); | ||
List<Footprint> recentFootprints = footprintRepository.findByCreatedAtAfter(startTime); | ||
|
||
Location currentLocation = new Location(request.latitude(), request.longitude()); | ||
|
||
return recentFootprints.stream() | ||
.filter(footprint -> footprint.isNear(currentLocation)) | ||
.map(FindNearFootprintResponse::from) | ||
.map(footprint -> FindNearFootprintResponse.from(footprint, memberId)) |
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를 생성하는데, 일관성이 유지되면 좋을 것 같아요.
일단 저희 팀은 생성자로 만드는 걸 컨벤션으로 정했었던 것 같아요.!
- 매개변수가 2개 이상인 정적팩토리메소드의 네이밍 컨벤션은 일반적으로 of를 썼던 것 같아요.
@Autowired | ||
private JdbcTemplate jdbcTemplate; | ||
|
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.
저희 JPA 사용하고 있는데 초기 데이터를 jdbcTemplate 사용해서 넣어주는 이유가 있을까요?
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.
트레 고생하셨어요~ 리뷰 남겼으니 확인 부탁드려요!
double longitude) { | ||
|
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.
double longitude) { | |
double longitude | |
) { |
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.
아이고야
@NotNull | ||
@DecimalMin(value = "-180.0", message = "경도는 -180도 이상 180도 이하로 입력해 주세요.") | ||
@DecimalMax(value = "180.0", message = "경도는 -180도 이상 180도 이하로 입력해 주세요.") | ||
double longitude) { |
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.
double longitude) { | |
double longitude | |
) { |
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.
어익후야
Long id = footprintCommandService.save(request); | ||
return ResponseEntity.created(URI.create("/footprints/" + id)) | ||
.build(); | ||
} | ||
|
||
@GetMapping("/near") | ||
public List<FindNearFootprintResponse> findNear(FindNearFootprintRequest request) { | ||
return footprintQueryService.findNear(request); | ||
public ResponseEntity<List<FindNearFootprintResponse>> findNear(@Valid FindNearFootprintRequest request) { |
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.
List를 반환할 때에는 ResponseEntity를 생략해도 좋을 것 같아요.
추후 response 객체로 감쌀 것을 고려했을 때, 반환 타입이 너무 길어질 것 같습니다.
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 validateRecentFootprintExists(Long memberId) { | ||
boolean exists = footprintRepository.existsByMemberIdAndCreatedAtAfter( | ||
memberId, | ||
LocalDateTime.now().minusSeconds(FootprintCommandService.FOOTPRINT_COOLDOWN) |
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.
LocalDateTime.now().minusSeconds(FootprintCommandService.FOOTPRINT_COOLDOWN) | |
LocalDateTime.now().minusSeconds(FOOTPRINT_COOLDOWN) |
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 lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
@Service | ||
@Transactional | ||
@RequiredArgsConstructor |
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.
저희 컨벤션에서 @RequiredArgsConstructor
는 사용하지 않기로 했어요!
.hasMessage("위도 범위가 올바르지 않습니다."); | ||
} | ||
|
||
@DisplayName("생성 실패 - 올바르지 않은 위도 범위") |
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.
@DisplayName("생성 실패 - 올바르지 않은 위도 범위") | |
@DisplayName("위도 범위를 벗어난 값을 입력하는 경우 예외가 발생한다.") |
@DisplayName
은 컨벤션에 맞춰 작성해주세요!
Long footprintId, | ||
double latitude, | ||
double longitude, | ||
@JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss") LocalDateTime createdAt, |
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.
@JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss")
가 없으면 어떤 형식으로 반환되나요?
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.
{
"createdAt": "2024-07-20T08:42:51.613518"
}
@JsonFormat
없이 테스트 해보니 위와 같은 형태로 응답합니다. 소수점까지 함께 응답하네요.
이 부분은 추후에 다시 얘기 나눠봐요!
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.
확인을 까먹고 있었네요
공통 포맷 작성할 때 다시 이야기해 보면 좋겠네요 :)
footprint.getLocation().getLatitude(), | ||
footprint.getLocation().getLongitude(), | ||
footprint.getCreatedAt(), | ||
memberId.equals(footprint.getMember().getId()) |
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.
내 발자국인지 확인하는 로직은 도메인 로직 아닐까요? Footprint 도메인에 넣어주는 편이 좋겠습니다.
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로 memberId를 넘겨주기보단, service에서 footprint.isMine()
메서드를 호출하고 DTO에는 그 결과값인 boolean을 넘겨주면 되겠어요.
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.
좋은 생각이네요!
return recentFootprints.stream() | ||
.filter(footprint -> footprint.isNear(currentLocation)) | ||
.map(FindNearFootprintResponse::from) | ||
.map(footprint -> FindNearFootprintResponse.from(footprint, memberId)) | ||
.toList(); |
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.
return recentFootprints.stream()
.filter(footprint -> footprint.isNear(currentLocation))
.map(footprint -> FindNearFootprintResponse.from(footprint, footprint.isMine(memberId)))
.toList();
위에서 말씀드린 방식으로 변경하면 이런식으로 변경되겠쭁?
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.
반영 수고하셨슴다!!!!! 주말 화이팅
Long footprintId, | ||
double latitude, | ||
double longitude, | ||
@JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss") LocalDateTime createdAt, |
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.
{
"createdAt": "2024-07-20T08:42:51.613518"
}
@JsonFormat
없이 테스트 해보니 위와 같은 형태로 응답합니다. 소수점까지 함께 응답하네요.
이 부분은 추후에 다시 얘기 나눠봐요!
footprint.getLocation().getLatitude(), | ||
footprint.getLocation().getLongitude(), | ||
footprint.getCreatedAt(), | ||
footprint.isCreatedBy(memberId) |
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는 단순 데이터 전송의 책임만 가지는게 좋을 것 같아요.
도메인 로직 호출은 service에서 수행하고, DTO에는 그 결과값만 넣어주는게 좋겠어요.
#122 (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.
이것만 진행해주시고 DM 주시면 바로 어푸루부 가겠습니다!
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가 로직 호출하는 게 어색하네요~!~! 반영할게요 🙇♂️
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.
고생 많으셨습니다!!
this( | ||
footprint.getId(), | ||
footprint.getLocation().getLatitude(), | ||
footprint.getLocation().getLongitude(), | ||
footprint.getCreatedAt(), | ||
footprint.isCreatedBy(memberId) | ||
isMine |
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.
👍
이슈
개발 사항
전달 사항