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

Feature/tape api #9

Merged
merged 4 commits into from
Jan 13, 2023
Merged

Feature/tape api #9

merged 4 commits into from
Jan 13, 2023

Conversation

junhyeok-18
Copy link
Member

테이브 생성
트랙 합치기
컬러 코드 연동

@junhyeok-18 junhyeok-18 requested a review from oereo January 13, 2023 15:46
@junhyeok-18 junhyeok-18 self-assigned this Jan 13, 2023
@junhyeok-18 junhyeok-18 temporarily deployed to develop January 13, 2023 15:46 — with GitHub Actions Inactive
Copy link
Member

@oereo oereo left a comment

Choose a reason for hiding this comment

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

Database mapping과 관련해서 추가사항 하나에 대해 리뷰남겨놓았습니다!
단순한 저의 의견이에요!!

this.audioLink = audioLink;
}

public void update(Long colorId, String audioLink) {
Copy link
Member

Choose a reason for hiding this comment

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

Tape의 수정 조건이
name, color색상 이렇게 바뀔 수 있을거에요!
update에 대해서 이렇게 한번 나눠서 해보는 것은 어떨까요??

public void update(Long colorId, String name)

public void updateAudioLink(String audioLink)

이렇게 나눈 이유는 위의 update의 경우에는 주인장이 한 화면에서 여러번에 걸쳐서 자주 바뀌는 값들이 될 것 같고
아래의 updateAudioLink의 경우에는 12명에 대한 조건일시에만 실행되는 method가 될 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

Tape의 수정 조건이 name, color색상 이렇게 바뀔 수 있을거에요! update에 대해서 이렇게 한번 나눠서 해보는 것은 어떨까요??

public void update(Long colorId, String name)

public void updateAudioLink(String audioLink)

이렇게 나눈 이유는 위의 update의 경우에는 주인장이 한 화면에서 여러번에 걸쳐서 자주 바뀌는 값들이 될 것 같고 아래의 updateAudioLink의 경우에는 12명에 대한 조건일시에만 실행되는 method가 될 것 같아요!

오오 알겠습니다!

Copy link
Collaborator

@Tarakyu Tarakyu left a comment

Choose a reason for hiding this comment

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

좋아요~!!

refactor : color entity, dto, repository 삭제
@junhyeok-18 junhyeok-18 temporarily deployed to develop January 13, 2023 17:40 — with GitHub Actions Inactive
@junhyeok-18 junhyeok-18 temporarily deployed to develop January 13, 2023 18:53 — with GitHub Actions Inactive
Copy link
Collaborator

@Tarakyu Tarakyu 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 5 to 8
public class RandomString {
public static String RandomString() {
Random random = new Random();
int length = 6;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public class RandomString {
public static String RandomString() {
Random random = new Random();
int length = 6;
public abstract class RandomStringUtils {
private static final Random random = new Random();
public static String getRandomString(int length) {

이러한 유틸성 클래스는 abstract로 추상 클래스로 만들어 다른 개발자가 실수로 이를 객체화하지 못하게 막아두는 편이에요. (이펙티브 자바에서는 기본 생성자를 private으로 만들어 객체화를 막는 방법을 권장하기도 합니다.)
그리고 유틸 클래스라는 것을 명시하기 위해 클래스 이름 뒤에 Util을 붙여보았어요.
또 메서드 실행시마다 Random 클래스가 객체화되는 것을 막기 위해 멤버 변수로 추가해보았구요,
length가 6이라는 맥락은 이 유틸 클래스에 있는 것이 아니라 외부의 정책에 의해 정해지는 것이기 때문에 매개변수로 빼봤어요.
마지막으로 메서드 이름은 동사로 시작하는 것이 좋다고 생각해서 getRandomString으로 바꿔보았습니다.

Comment on lines 22 to 32
private String tapeLink;
private String audioLink;

@Builder
public Tape(Long memberId, String colorCode, String name, String audioLink) {
this.memberId = memberId;
this.colorCode = colorCode;
this.name = name;
this.tapeLink = RandomString.RandomString();
this.audioLink = audioLink;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private String tapeLink;
private String audioLink;
@Builder
public Tape(Long memberId, String colorCode, String name, String audioLink) {
this.memberId = memberId;
this.colorCode = colorCode;
this.name = name;
this.tapeLink = RandomString.RandomString();
this.audioLink = audioLink;
}
private String tapeLink;
private String audioLink;
private static final int TAPE_LINK_LENGTH = 6;
@Builder
public Tape(Long memberId, String colorCode, String name, String audioLink) {
this.memberId = memberId;
this.colorCode = colorCode;
this.name = name;
this.tapeLink = RandomStringUtils.getRandomString(TAPE_LINK_LENGTH);
this.audioLink = audioLink;
}

length가 6이라는 맥락은 유틸 클래스에 있는 것이 아니라 외부의 정책에 의해 정해지는 것이기 때문에 TAPE에 보관하도록 빼봤어요.
그리고 RandomStringUtils.getRandomString(6)과 같이 코드를 작성하게 되면 다른 개발자가 저 6의 의미를 파악하기 어려울 수 있으므로 이러한 상수는 따로 static final 상수로 빼는 것이 좋다고 생각했어요.

Copy link
Member

Choose a reason for hiding this comment

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

private static final int TAPE_LINK_LENGTH = 6;
요 코드만 class 안의 맨 윗 부분에 위치하면 좋을 것 같아요!

@junhyeok-18 junhyeok-18 temporarily deployed to develop January 13, 2023 19:39 — with GitHub Actions Inactive
Copy link
Collaborator

@Tarakyu Tarakyu left a comment

Choose a reason for hiding this comment

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

좋아욤~~

@junhyeok-18 junhyeok-18 merged commit d4e78c6 into develop Jan 13, 2023
@junhyeok-18 junhyeok-18 deleted the feature/Tape-api branch January 13, 2023 19:42
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.

3 participants