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

Java Assignment3 upload by HoonSubKim #12

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

Conversation

khsrla9806
Copy link

@khsrla9806 khsrla9806 commented Apr 16, 2023

코드 리뷰 빡세게 부탁드립니다 ! 🔥🔥🔥

  • Electronic 클래스의 hashCode() 메서드에서 어떤 필드의 값들을 해시코드로 넣어야할지 고민했습니다.
    • authMethod에 있는 Enum 값들도 하나씩 반복문을 돌면서 HashCode에 추가해줬어야 했는지.. 궁금합니다.

Copy link

@Jungdahee Jungdahee left a comment

Choose a reason for hiding this comment

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

안녕하세요 훈섭님~

먼저 과제 수행하신다고 고생 많으셨어요!
이번에도 역시나 빠르게 제출하는 성실함!ㅎㅎ아주 칭찬해 드리고 싶습니다.

일단 코드에서 고민한 흔적이 많이 보여서 좋았는데요, equals()나 hashCode()를 완벽하게 구현하지 못한다면 차라리 IDE의 자동 완성 기능을 활용하는 것도 좋을 것 같아요!
하나 하나 다 비교를 하게 되면 분명 놓치는 부분이 생길 수도 있으니, 이 부분은 한번 고민해 보시길 바라요!

그리고 commit 단위가 너무 커보여요. 지금은 과제 제출이니 세세하게 나누지 않아도 크게 문제되진 않지만, 현업에 가게 되면 commit message로 어떤 작업이 수행되었는지 쉽게 알 수 있도록 한답니다.

다음 과제 제출 시에는 이 부분 한번 고민해서 반영해 보시죠!
feat, fix 등과 같이 작업 내용을 보다 한 눈에 파악하기 쉽게 한번 표현해 봅시다:)

수고 많으셨어요~

import java.util.Objects;

public class Electronic {
private static int autoIncrementNumber = 0;

Choose a reason for hiding this comment

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

static 변수를 가장 상위에 위치하도록 해주셨네요. 아주 굿!

private String modelName;
private Company companyName;
private String dateOfDate;
private AuthMethod[] authMethod;

Choose a reason for hiding this comment

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

사소한 부분이지만, 배열의 형태이니 복수형을 의미하는 변수로 변경해 보는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

변수 이름도 신경쓰면서 작업해봐야겠네요. 감사합니다!

private String modelName;
private Company companyName;
private String dateOfDate;
private AuthMethod[] authMethod;

Choose a reason for hiding this comment

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

Suggested change
private AuthMethod[] authMethod;
private AuthMethod[] authMethods;

Company companyName,
String dateOfDate
) {
this.productNo = this.generateProductNo();

Choose a reason for hiding this comment

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

함수로 분리하는 센스! 너무 좋습니다.

}

private String generateProductNo() {
String id = String.format("%04d", ++autoIncrementNumber);

Choose a reason for hiding this comment

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

정규식 사용으로 코드가 한결 간단해 졌어요 아주 잘하셨습니다! 👍

Users users = (Users) obj;

if (userList.length != users.userList.length) return false;
for (int i = 0; i < users.userList.length; i++) {

Choose a reason for hiding this comment

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

고민의 흔적이 보이네요. 리스트까지 비교하는 세심함! 좋아요


@Override
public int hashCode() {
return Arrays.hashCode(userList);

Choose a reason for hiding this comment

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

자동 완성 기능을 한번 이용해 봅시다!

public class Electronics {
private static Electronics instance;
private Electronic[] electronicList;
private static final int DEFAULT_SIZE = 50;

Choose a reason for hiding this comment

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

얘도 마찬가지로 위로 올려주세용

return electronic;
}
}
throw new IllegalArgumentException("찾는 " + productNo + "에 해당하는 제품은 존재하지 않습니다.");

Choose a reason for hiding this comment

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

이런 센스 아주 좋습니다!

public Electronic[] groupByAuthMethod(AuthMethod authMethod) {
List<Electronic> electronics = new ArrayList<>();
for (Electronic electronic : electronicList) {
for (AuthMethod method : electronic.getAuthMethod()) {

Choose a reason for hiding this comment

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

놓치기 쉬운 부분인데 잘 캐치하셨어요 잘하셨습니다:) 이 부분도 역시나 stream으로 변경해 볼 수 있겠죠?

Copy link
Author

Choose a reason for hiding this comment

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

꼼꼼한 리뷰 감사합니다! 🙆🏻‍♂️

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.

2 participants