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

[Feat] #5 videoAlias 정보를 랜덤하게 매핑해주는 기능을 구현했어요! #16

Merged
merged 19 commits into from
Sep 27, 2024

Conversation

BaeJunH0
Copy link
Contributor

@BaeJunH0 BaeJunH0 commented Sep 21, 2024

✨ 작업 내용

피드백 반영 사항

  • 도메인 별로 패키지 분리
  • 컨벤션에 맞게 패키지 명 변경 ( DTO -> dto )
  • 사용하던 생성자 모두 lombok으로 변경 ( 진짜 편하긴 하네요 ㅎㅎ; )

개발 사항

  • videoAlias를 어떻게 매핑할까 고민을 좀 하다보니, 결국 Alias 네이밍의 기준이 카테고리 ( 장소 분류 ) 를 따라가야 한다고 생각했습니다
  • 그래서 Map 형식으로 카테고리를 키로, 몇 개의 예시 구문을 값으로 만들어 static 구문으로 붙였습니다 ( 컴포넌트는 싱글톤 방식으로 관리되니, 이렇게 생성해도 되지 않을까 싶었습니다 )
  • 이외에도 weekly 브랜치 pull 하여 리펙토링 진행했습니다 ( 이 부분에서 컨벤션 좀 정해야 할 듯 합니당 )

✨ 참고 사항

  • videoAlias를 매핑하는 부분을 만들긴 했는데, 각 카테고리 별 Alias 수나, 저장과 매핑 방식 자체도 개선이 필요하다고 생각합니다, 추후에 개선 방향성을 제시해주신다면 기꺼이 수정해보겠습니다.
  • #3의 하위 이슈인 형식에 맞게 비디오 정보를 반환하는 기능 #4 videoAlias 정보를 랜덤하게 매핑해주는 기능 #5 를 개발했으니, 한 단위 개발이 완료되었다고 생각합니다, PR 승인 이후에 도메인, 서비스, 컨트롤러에 대한 테스팅 진행해보겠습니다!
  • Video 도메인에 대한 테스트 코드 작성 완료했습니다!
  • 9/23 월요일 회의 내용 반영 완료했습니다! ( dto, record, package 관련 )

⏰ 현재 버그

  • 발견하지 못했습니다

✏ Git Close

BaeJunho added 4 commits September 22, 2024 16:43
사용 지점과 최대한 가깝게 변수 선언 시점을 변경
메서드 체이닝을 통해 필요 없는 변수를 삭제
id를 제외한 필드를 사용하여 생성자를 만들고, 이를 통해 테스트를 진행하기 위하여 @nonnull@requiredargsconstructor을 사용하여 리펙토링
추후에 통합 테스트 필요 ( + POSTMAN을 이용한 API 테스트도 필요 )
@sanghee0820 sanghee0820 added the 📬 API 서버 API 통신 label Sep 23, 2024
BaeJunho added 3 commits September 24, 2024 14:13
추후에 통합 테스트 필요 ( + POSTMAN을 이용한 API 테스트도 필요 )
record 컨벤션, dto 컨벤션 반영 및 패키지 구조 변경
…videoAlias

# Conflicts:
#	src/test/java/team7/inplace/VideoTest/domain/VideoTest.java
#	src/test/java/team7/inplace/VideoTest/repository/VideoRepositoryTest.java
#	src/test/java/team7/inplace/VideoTest/service/VideoServiceTest.java
@BaeJunH0 BaeJunH0 added the ✨ Feature 기능 개발 label Sep 24, 2024
Copy link
Contributor

@suhyeon7497 suhyeon7497 left a comment

Choose a reason for hiding this comment

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

전반적으로 코드를 잘 작성하시네요! 제가 남긴 부분은 한번 생각해보시고 반영할 부분만 반영해주세요! Approve 하겠습니다.

"이(가) 추천하는 가게에서 정성스럽게 준비된 한식으로 든든한 한 끼를!",
"이(가) 극찬! 한식의 깊은 맛을 느껴보세요."
));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

어떻게 하면 좋을 진 잘 모르겠으나 template을 클래스로 분리해서 makeAlias()를 Service에서 분리하는게 어떨까 싶네요.
Service에서 해야할 일이 아닌것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

듣고보니 그런 것 같습니다 분리하겠습니당

import team7.inplace.place.domain.Place;
import team7.inplace.video.domain.Video;

public class VideoTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

이걸 Test한 목적이 무엇인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아직 도메인에 로직이 없긴한데, 나중에 해당 사항 테스트 할 수도 있고, 뭔가 관성적으로 한 것 같습니다

랜덤한 Alias를 매핑해주는 기능은 Service의 영역이 아니라는 피드백을 수용하여 이를 분리
@sanghee0820 sanghee0820 self-requested a review September 26, 2024 02:31
Copy link
Contributor

@sanghee0820 sanghee0820 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 +17 to +29
public class VideoController {
private final VideoService videoService;

// 내 인플루언서가 방문한 그 곳 ( 토큰 0 )
@GetMapping("/video")
public ResponseEntity<List<VideoResponse>> readByInfluencer(
@RequestParam(name = "influencer", required = false) List<String> influencers
) {
List<VideoInfo> videoInfos = videoService.findByInfluencer(influencers);
List<VideoResponse> videoResponses = videoInfos.stream().map(VideoResponse::new).toList();
return new ResponseEntity<>(videoResponses, HttpStatus.OK);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Swagger에 대한 명세가 존재하지 않네요!!

@BaeJunH0 BaeJunH0 changed the base branch from Master to weekly/2 September 26, 2024 11:11
Copy link
Contributor

@dong-yxxn dong-yxxn left a comment

Choose a reason for hiding this comment

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

In으로하면 리스트로 받을수 있다네요! approve하겠습니다

BaeJunho added 2 commits September 26, 2024 22:49
랜덤한 Alias를 매핑하는 기능을 Util 클래스와 Enum 클래스로 나눠 구현
DTO 클래스의 static factory method 구현
@sanghee0820 sanghee0820 self-requested a review September 27, 2024 01:38
Copy link
Contributor

@sanghee0820 sanghee0820 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 +24 to +29
List<Long> influencerIds = influencerRepository.findByNameIn(influencers).stream()
.map(Influencer::getId)
.toList();

// 인플루언서 정보로 필터링한 비디오 정보 불러오기
List<Video> savedVideos = videoRepository.findVideosByInfluencerIdIn(influencerIds);
Copy link
Contributor

Choose a reason for hiding this comment

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

나중에 Join해서 한번에 가져와도 괜찮을 것 같아요!


@Entity
@Getter
@NoArgsConstructor(access = lombok.AccessLevel.PROTECTED)
Copy link
Contributor

Choose a reason for hiding this comment

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

static 으로 나중에 변경할게요~!

@sanghee0820 sanghee0820 merged commit 0ef6809 into weekly/2 Sep 27, 2024
@sanghee0820 sanghee0820 deleted the feat/#5-videoAlias branch October 31, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📬 API 서버 API 통신 ✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants