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 GyeongminKang #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

redbean00
Copy link

@redbean00 redbean00 commented Apr 17, 2023

과제를 풀면서 아직 객체지향적인 코드를 짜는 것이 미숙하다는 것을 깨달았습니다. 코드 리뷰 부탁드립니다!

AuthMethod1("FINGERPRINT"),
AuthMethod2("PATTERN"),
AuthMethod3("PIN"),
AuthMethod4("FACE");

Choose a reason for hiding this comment

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

enum은 서로 연관된 상수들의 집합이기 때문에 변수명을 의미있고 다른 상수들과 구분지을 수 있게 지어야 합니다. AuthMethod1~4 의 네이밍 같은 경우 해당 상수가 어떤 상수인지 의미를 알기 어렵기 때문에 개발 시 authMothodName에 접근을 해야 알게 되기 때문에 개발 편의성도 떨어지구요.

return copy;
}

copy = user;

Choose a reason for hiding this comment

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

copy에 parameter로 넘어온 user를 할당하면 35번줄에서 메모리에 새로 띄운 User객체가 아닌 parameter로 넘어온 user의 값을 변경하게 되므로 아래에서 return되는 copy는 user 그자체를 넘기게 되는 형태가 됩니다.
따라서 이 줄은 삭제해야합니다


//제조 회사로 찾아서 하나의 배열로 반환
public Electronic[] groupByCompanyName(Electronic.Company company){
Electronic[] ans = new Electronic[electronicList.length];

Choose a reason for hiding this comment

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

저라면 리턴되야할 배열의 크기를 모를 땐 컬랙션(ArrayList)를 사용할 것 같아요. loop 다 돌고 배열로 변환 후 리턴해주면 리턴타입 문제는 해결되니깐요. 그리고 electronicList.length 만큼의 배열에 필터된 값들이 들어간다고 하더라고 리턴되는 배열의 length 또한 필터링 되기 전 전체 배열의 사이즈와 같기 때문에 외부에서 필터링된 배열의 사이즈로 처리하는 로직이 들어간다면 의도한 대로 작동하지 않을거에요

}

public Electronic[] groupByAutoMethod(Electronic.AuthMethod authMethod){
Electronic[] ans = new Electronic[electronicList.length];

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