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 Son young jun #25

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

Conversation

sohn919
Copy link

@sohn919 sohn919 commented Apr 17, 2023

Practice03 에서 groupByAuthMethod() 리뷰 부탁드립니다.
이중 for문 말고 더 좋은 형태가 있을까요?

@sohn919 sohn919 changed the title Son young jun Java Assignment3 upload by Son young jun Apr 17, 2023
@sohn919 sohn919 requested a review from ecsimsw April 17, 2023 12:44
Copy link
Member

@ecsimsw ecsimsw left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.
잘하셨네요.

private String dateOfMade;
private AuthMethod[] authMethod;

Electronic() {
Copy link
Member

Choose a reason for hiding this comment

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

접근 제한자를 default로 한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

이건 제가 깜빡하고 접근 제어자를 지정하지 않은 것 같습니다.

@Override
public int hashCode() {
int result = Objects.hash(userId, userPassword, userPhoneNumber, userEmail, userBirthDate, registerTime);
result = 31 * result + Arrays.hashCode(electronicDevices);
Copy link
Member

Choose a reason for hiding this comment

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

오오 result에 31을 곱한 이유가 뭐예요?

private static Users instance = null;

private Users(){
};
Copy link
Member

Choose a reason for hiding this comment

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

for(Electronic electronic : electronicList) {
for(AuthMethod auth : electronic.getAuthMethod()) {
if(auth == authMethod)
arr.add(electronic);
Copy link
Member

Choose a reason for hiding this comment

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

이미 추가된 electronic의 경우 그 다음 authMethod를 보지 않아도 되겠네요.
맞나요? ㅎㅎ

for(Electronic electronic : electronicList) {
             for(AuthMethod auth : electronic.getAuthMethod()) {
                 if(auth == authMethod) {
                     arr.add(electronic);
                     break;
                 }
          }
}

Copy link
Author

Choose a reason for hiding this comment

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

그 부분은 생각을 못했었네요 감사합니다👍

}

public User(String userId, String userPassword, String userPhoneNumber, String userEmail, String userBirthDate, Electronic[] electronicDevices) {
this();
Copy link
Member

Choose a reason for hiding this comment

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

오오 리뷰 중에 this()로 생성자 로직 중복을 막은 첫 코드네요.
잘하셨습니다.

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