Java Assignment3 upload by HyunsooJung #21

hyunsb · 2023-04-17T08:11:38Z

실습과제 3 제출합니다.

멘토님의 정성스러운 코드리뷰 너무 잘봤습니다.
이렇게까지 해주실 줄 몰랐는데 감동받았습니다. 감사합니다😃
이번에도 따끔하게 리뷰 부탁드립니다!

질문

실습 1. Electoronic 클래스의 hashCode() 구현
만약 Electoronic클래스의 인스턴스가 가지는 registrationNo 멤버변수가 중복이 거의 불가능하다고 가정한다면
해당 변수를 hashCode로 사용하는 것이 가능할까요?
이게 가능하다면 equals() 구현 시에도 hashCode를 통한 빠른 비교가 가능할 것 같아서 질문드립니다!
실습 2. Users 클래스의 깊은 복사 기능 구현
깊은 복사 기능을 위해 User 클래스에 private으로 All Argument 생성자를 추가 구현하였습니다.
setter를 통하여 구현할 경우 User 클래스의 멤버가 추가 혹은 삭제됨에 따라 변경할 코드가 조금 더 늘어날 것 같다는 개인적인 판단입니다.
혹시 더 효율적인 다른 방법이 있을까요?
실습 3. Electronics 클래스의 groupByCompanyName() 구현
NPE를 방지하기 위해 Optional을 사용해봤습니다.
Electronics 클래스의 52 ~ 56 라인과 80 ~ 84 라인의 로직이 동일한데
해당 로직은 함수화 시켜 호출하는 게 좋을까요?
-> 개인적인 판단으로 유틸클래스로 분리하고 함수화 시켜봤습니다!

멤버들을 구분하는 개행 규칙? 컨벤션이 따로 있나요?

아래 예시 처럼 해당 변수의 타입에 맞게 구분 해놓을 것인지

private static final int DEFAULT_CAPACITY = 10; // Default initial capacity
private static final Electronic[] EMPTY_ELECTRONIC_LIST = {};

private static Electronic[] electronicList;
private static Electronics electronicsInstance;

private int size;
private int capacity;

아래 예시 처럼 연관 되는 변수 끼리 구분 해놓을 것인지

private static Electronics electronicsInstance;

private static Electronic[] electronicList;
private static final int DEFAULT_CAPACITY = 10; // Default initial capacity
private static final Electronic[] EMPTY_ELECTRONIC_LIST = {};
private int size;
private int capacity;

그냥 개인적인 코딩 스타일 일까요?

hyunsb · 2023-04-17T12:41:45Z

me/day05/practice/Electronics.java

+    public Optional<Electronic> findByProductNo(String productNo){
+        for (Electronic electronic : electronicList)
+            if (productNo.equals(electronic.getProductNo()))
+                return Optional.of(electronic);
+
+        return Optional.empty();
+    }


equals() 메소드를 사용할 때 모종의 상황 발생으로
electronic이 Null일 경우 NPE가 발생할 수 있다고 생각하였습니다.

다시 생각해보니 productNo도 인자 값을 Null으로 넘기는 경우에 NPE가 발생하는데
널 참조 방지 코드를 추가하는 게 좋을까요?

이정도면 충분할 것 같습니다. (사실 해도 됩니다. null 체크야 해서 나쁠건 없지만, 지금 코드가 보편적이긴 합니다.)
Electronic쪽에서 productNo는 null이 안되도록 강제하면 더 좋겠네요.

효율적인 문제로 사용 메소드 변경 관련 자료: https://yeonyeon.tistory.com/294

hyunsb · 2023-04-17T12:56:12Z

me/day05/practice/Electronic.java

+    public boolean isContainAuthMethod(AuthMethod authMethod){
+        for (AuthMethod auth : this.authMethod)
+            if (authMethod.equals(auth)) return true;
+        return false;
+    }


이 부분은 요구사항에는 없었지만
Electronics클래스의 groupByAuthMethod()를 구현하다가
있었으면 좋겠다는 느낌이 들어 추가했습니다.

굳굳!!
근데 is contain이 문법적으로 맞나요?

꼭 단어나 문법이 정확하진 않아도 되지만 가급적이면 지켜주는게 좋아요.
map의 containsKey가 있으니 저라면 'contains~'로 지었을거 같네요.

헉! 지금까지 계속 contain으로 사용하고 있었네요 맙소사,, 감사합니다

hyunsb · 2023-04-17T14:43:42Z

me/day05/practice/Electronics.java

+        for (Electronic electronic : electronicList)
+            // 테스트용 임시 조건 ( electronic != null )
+            if (electronic != null && electronic.getCompanyName().equals(company))
+                temp.add(electronic);


이 부분은 electronic에 대한 Null 체크가 진행되어서
electronic의 companyName에 대한 equals()를 호출하였습니다.

setRegistrationNo -> resetRegistrationNo

제네릭으로 범용성을 증가시키려 했지만 실패!,, ClassCastException 발생 List<Electronic> -> Electronic[] 으로 변경하는 유틸 클래스 추가

hyunsb · 2023-04-21T06:16:21Z

me/day05/practice/Electronics.java

+        Electronic[] authMethodNameGroup =
+                temp.isEmpty() ? null : ElectronicArrayUtil.listToArray(temp);


이부분을 제네릭을 사용한 유틸 클래스(ArrayUtil)의 사용으로
변경하고 싶었는데 ClassCastException 발생으로 실패했습니다..

따라서 임시방편으로 Electronic 클래스를 위한 유틸 클래스로 대체했습니다.
나중에 해당 문제의 해답을 찾으려 합니다.

여기서 의문이 생겼습니다.
만약에 List 을 Electronic[] 으로 캐스팅하여 반환하는 유틸성 메서드를 구현하고자 한다면
해당 메서드는 어느 클래스에 선언되는 것이 적합할까요?

새로운 유틸 클래스를 만드는 방법?
타입과 밀접한 연관이 있는 Electronic ?
실제 사용 위치와 관련된 Electronics ?

저는 개인적으로 최선책은 유틸 클래스에 선언,
차선책은 Electronic에 선언하는 것이라고 생각합니다.

저라면 Electronics에 선언했을 것 같습니다.
음 셋 중에 static method로 정의하지 않아도 되는, private 인스턴스 메서드는 Electronics에서 선언하는거 하나네요?
그 말은 객체로서의 기능으로 사용되는 상황은 Electronics 이 친구 하나라고 생각하네요. 나머지는 말씀대로 그저 '유틸성' 기능. 객체랑 전혀 상관없는.

일단 넘어갑시다 ㅎㅎ
조금만 더 지나고 다시 대화해봐요.

하 이렇게 하면 또 궁금하셔서 잠 못 주무실테니 키워드는 'static 객체지향' 이런 쪽으로 검색해보세요
https://www.yegor256.com/2014/05/05/oop-alternative-to-utility-classes.html

hyunsb · 2023-04-21T08:46:41Z

me/day05/practice/Electronic.java

+    private void setProductNo(){
+        if (registrationNo > MAX_REGISTRATION_NUMBER) resetRegistrationNo();
+        productNo = dateOfMade + String.format("%4d", registrationNo).replace(" ", "0");
+    }
+
+    private void resetRegistrationNo(){
+        registrationNo = 1;
+    }


멘토님의 코드리뷰를 통해 기능을 무분별하게 작은 함수로 쪼개는 것은
코드의 가독성을 해칠 수 있다는 것을 깨달았습니다.

이 코드에서는 조금 의문이 생겼습니다.
과제의 요구사항 상 registrationNo가 4자리로 제한 되어 있습니다.
만약 registrationNo가 4자리를 넘어가게 된다면 다시 '어떠한 값' 으로 초기화를
시켜줘야 한다고 생각하고 코드를 작성하였습니다.

setProductNo()메소드에서 registrationNo를 체크하고 초기화하는 코드까지 만들자니
메소드명과 어울리지 않는 기능을 담당한다고 생각이 들었습니다.
따라서 registrationNo를 초기화 하는 resetRegistrationNo() 메소드를 생성하여 코드를 분리하였습니다.

이런 상황에서는 메소드를 분리하는 것이 맞을까요?
메소드 명을 변경하고 기능을 분리하지 않는 것이 맞을까요?

이건 제 기준인데 저는 메서드 분리에 가장 큰 근거로 그 로직이 얼마나 많이 중복되어 사용되는가를 봅니다.
예를 들면 똑같은 A로직이 6번 사용된다면 A로직에 조금이라도 변경이 생겼을 때 6개나 영향을 미치니 이걸 메서드로 빼면 한번만 바꾸면 되겠구나? 라는 생각을 해요.
지금 'resetRegistrationNo'은 한번 밖에 안쓰이네요? 이 기준으라면 분리하지 않아도 되었겠네요.

또 저는 로직에 이름을 달기 위함이라는 근거를 들어요. 개인적으로는 'registrationNo = 1;' 자체가 'resetRegistrationNo'보다 더 한눈에 잘보이네요 ㅋㅋ

이건 제 취향이고 더 중요한건 이제 현수님의 취향인거 같아요.

계속 학습하면서 저만의 취향과 기준을 만들어야 겠네요.
멘토님의 기준과 좋은 말씀 감사합니다!

ecsimsw

고생하셨습니다.
고민하신 흔적이 많이 보이네요.
재밌는 질문들이 많아서 너무 깊어진거 아닌가 걱정이네요.

싱글톤의 생성자 제한이랑 static 메서드로 빼셨던 로직의 필요, class 내부에 main 문 넣어서 테스트 하셨던 부분들은 다시 한번 생각해보셨으면 좋겠습니다.
너무 어렵거나 이해가 잘 안되시면 언제든 대화 요청해주세요.
화이팅입니다.

ecsimsw · 2023-04-21T09:45:09Z

me/day05/practice/ArrayUtil.java

+        for (int i = 0; i < resultArray.length; i++)
+            resultArray[i] = list.get(i);
+
+        return resultArray;


이 메서드는 사용되는 코드인가요?
사용되지 않는 코드는 지워주세요.

ecsimsw · 2023-04-21T09:49:48Z

me/day05/practice/Electronic.java

+    private void setProductNo(){
+        if (registrationNo > MAX_REGISTRATION_NUMBER) resetRegistrationNo();
+        productNo = dateOfMade + String.format("%4d", registrationNo).replace(" ", "0");
+    }
+
+    private void resetRegistrationNo(){
+        registrationNo = 1;
+    }


이건 제 기준인데 저는 메서드 분리에 가장 큰 근거로 그 로직이 얼마나 많이 중복되어 사용되는가를 봅니다.
예를 들면 똑같은 A로직이 6번 사용된다면 A로직에 조금이라도 변경이 생겼을 때 6개나 영향을 미치니 이걸 메서드로 빼면 한번만 바꾸면 되겠구나? 라는 생각을 해요.
지금 'resetRegistrationNo'은 한번 밖에 안쓰이네요? 이 기준으라면 분리하지 않아도 되었겠네요.

또 저는 로직에 이름을 달기 위함이라는 근거를 들어요. 개인적으로는 'registrationNo = 1;' 자체가 'resetRegistrationNo'보다 더 한눈에 잘보이네요 ㅋㅋ

이건 제 취향이고 더 중요한건 이제 현수님의 취향인거 같아요.

ecsimsw · 2023-04-21T09:52:45Z

me/day05/practice/Electronic.java

+    public boolean isContainAuthMethod(AuthMethod authMethod){
+        for (AuthMethod auth : this.authMethod)
+            if (authMethod.equals(auth)) return true;
+        return false;
+    }


굳굳!!
근데 is contain이 문법적으로 맞나요?

꼭 단어나 문법이 정확하진 않아도 되지만 가급적이면 지켜주는게 좋아요.
map의 containsKey가 있으니 저라면 'contains~'로 지었을거 같네요.

ecsimsw · 2023-04-21T09:55:44Z

me/day05/practice/ElectronicArrayUtil.java

+
+public class ElectronicArrayUtil {
+
+    static Electronic[] listToArray(List<Electronic> list) {


이 메서드가 분리된 유틸 클래스에 있는 것과 Electronic에 있는 것, 어떤 차이가 있을까요?
왜 분리하고 싶었는지가 궁금합니다!!

Users 에서도 groupBy 기능이 생길 것 같아서
추후 확장을 위해 제네릭으로 선언하고 싶었습니다.

제네릭으로 해당 유틸 로직을 만드는 데 실패해서 임시방편으로 Electronic 클래스를 위한 유틸클래스로 분리했습니다

ecsimsw · 2023-04-21T10:05:33Z

me/day05/practice/Electronics.java

+        Electronic[] authMethodNameGroup =
+                temp.isEmpty() ? null : ElectronicArrayUtil.listToArray(temp);


저라면 Electronics에 선언했을 것 같습니다.
음 셋 중에 static method로 정의하지 않아도 되는, private 인스턴스 메서드는 Electronics에서 선언하는거 하나네요?
그 말은 객체로서의 기능으로 사용되는 상황은 Electronics 이 친구 하나라고 생각하네요. 나머지는 말씀대로 그저 '유틸성' 기능. 객체랑 전혀 상관없는.

일단 넘어갑시다 ㅎㅎ
조금만 더 지나고 다시 대화해봐요.

하 이렇게 하면 또 궁금하셔서 잠 못 주무실테니 키워드는 'static 객체지향' 이런 쪽으로 검색해보세요
https://www.yegor256.com/2014/05/05/oop-alternative-to-utility-classes.html

ecsimsw · 2023-04-21T10:08:36Z

me/day05/practice/Electronics.java

+        electronicList[size++] = electronic;
+    }
+
+    public static void main(String[] args) {


오잉? 이건 지우는게 좋을 것 같아요.
혼자 테스트 하는 용도라도 진짜 프로젝트 main문에서 해보실까요?

Electronics class에서 하시면 안될 것 같아요!

ㅎㅎ,, 이번 과제는 구현과제라서 저 혼자 테스트 해보고
멘토님도 테스트 해보실 줄 알고 놔뒀습니다.

다음부턴 지우거나
테스트코드를 JUnit을 사용해서 따로 작성하던가 하겠습니다!

좋습니다!

ecsimsw · 2023-04-21T10:08:49Z

me/day05/practice/Electronics.java

+        electronics.add(iPhone12);
+        electronics.add(galaxyS22);
+
+//        System.out.println(electronics);


의미 없는 코드는 제거해주세요.

ecsimsw · 2023-04-21T10:10:42Z

me/day05/practice/Electronics.java

+
+    Electronics(){
+        electronicList = EMPTY_ELECTRONIC_LIST;
+    }


싱글톤으로 사용하고자 한다면 생성자는 private으로 막아주는 것이 일반적입니다.
ELECTRONIC_LIST가 비어있기 때문에 상관없다고 생각할 수 있으나 당장 생성에는 문제가 없어서 이 코드를 사용하는 다른 개발자에게 혼란을 주기 쉽거든요.

헉 너무 기본적인 실수,, 열번 백번 더 확인하겠습니다,,!

ecsimsw · 2023-04-21T10:10:56Z

me/day05/practice/User.java

+//        private String formatUserEmail(String userPhoneNumber){ validateUserEmailFormat(String userPhoneNumber) }
+//
+//        private void validateUserBirthDateFormat(String userBirthDate){}
+//        private String formatUserBirthDate(String userBirthDate){ validateUserBirthDateFormat(String userBirthDate) }


사용되지 않는 코드는 지워주세요.

ecsimsw · 2023-04-21T10:20:41Z

me/day05/practice/Electronic.java

+    @Override
+    public int hashCode() {
+        return Objects.hash(productNo, modelName, companyName, dateOfMade, Arrays.hashCode(authMethod));
+    }


꼭 모든 프로퍼티가 아니라 특정 몇개의 프로퍼티로 hashCode를 만들기도 하죠. 좋은 아이디어네요.
다만 hashCode는 속도 비교가 아니라 registrationNo가 Electoronic의 해시값이 될 수 있도록 골고루 분산되어 있을까를 더 고민해야합니다.

이 부분은 한번 찾아보고, 고민해 보겠습니다

사용하지 않는 테스트, main 메서드, 유틸 클래스 삭제 유틸성 메서드 Electronics 클래스에 정의

메서드 이름 변경 isContainAuthMethod -> isContainsAuthMethod (최대한 문법을 지키도록 노력하자) resetRegistrationNo() 메서드 삭제 (가독성과 코드 집중화를 고려해보자)

불필요한 개행 처리 삭제 주석처리 삭제

생성자의 접근제어자 변경 (익숙한 것에서 실수가 나온다 다시 한 번 확인하자) 불필요한 테스트 위주 코드 삭제

hyunsb added 2 commits April 17, 2023 16:23

Java Assignment3 upload by HyunsooJung

7c1cac0

style: Modify Wrapping Lines

e99be9a

hyunsb requested a review from ecsimsw April 17, 2023 10:27

hyunsb commented Apr 17, 2023

View reviewed changes

refactor: change method replaceAll() to replace()

b4110f8

효율적인 문제로 사용 메소드 변경 관련 자료: https://yeonyeon.tistory.com/294

hyunsb commented Apr 17, 2023

View reviewed changes

hyunsb added 3 commits April 21, 2023 14:26

refactor: Electonic 클래스 하드코딩 부 변수화

c4d5d74

refactor: Electonic 클래스 메서드명 변경

8335a7a

setRegistrationNo -> resetRegistrationNo

refactor: list를 배열로 변경하는 유틸 클래스 추가

8c81625

제네릭으로 범용성을 증가시키려 했지만 실패!,, ClassCastException 발생 List<Electronic> -> Electronic[] 으로 변경하는 유틸 클래스 추가

hyunsb commented Apr 21, 2023

View reviewed changes

refactor: setProductNo() 메서드 registrationNo 체크 로직 부등호 변경

82a9719

hyunsb commented Apr 21, 2023

View reviewed changes

ecsimsw suggested changes Apr 21, 2023

View reviewed changes

refactor: 코드리뷰 반영

b532bf1

사용하지 않는 테스트, main 메서드, 유틸 클래스 삭제 유틸성 메서드 Electronics 클래스에 정의

hyunsb force-pushed the HyunsooJung branch from ad9d25a to b532bf1 Compare April 21, 2023 12:13

hyunsb added 3 commits April 21, 2023 21:14

refactor: 코드리뷰 반영(Electronic)

9b83298

메서드 이름 변경 isContainAuthMethod -> isContainsAuthMethod (최대한 문법을 지키도록 노력하자) resetRegistrationNo() 메서드 삭제 (가독성과 코드 집중화를 고려해보자)

style: 코드리뷰 반영(User)

0fce5ad

불필요한 개행 처리 삭제 주석처리 삭제

style: 코드리뷰 반영(Users)

fbec7c0

생성자의 접근제어자 변경 (익숙한 것에서 실수가 나온다 다시 한 번 확인하자) 불필요한 테스트 위주 코드 삭제

hyunsb changed the title ~~Java Assignment3 upload by HyunsooJung~~ [1차]Java Assignment3 upload by HyunsooJung Apr 22, 2023

hyunsb changed the title ~~[1차]Java Assignment3 upload by HyunsooJung~~ [1차] Java Assignment3 upload by HyunsooJung Apr 22, 2023

hyunsb changed the title ~~[1차] Java Assignment3 upload by HyunsooJung~~ Java Assignment3 upload by HyunsooJung Apr 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java Assignment3 upload by HyunsooJung #21

Java Assignment3 upload by HyunsooJung #21

hyunsb commented Apr 17, 2023 •

edited

Loading

hyunsb Apr 17, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 17, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 21, 2023

hyunsb Apr 17, 2023

hyunsb Apr 21, 2023 •

edited

Loading

ecsimsw Apr 21, 2023

hyunsb Apr 21, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 21, 2023

ecsimsw left a comment

ecsimsw Apr 21, 2023

ecsimsw Apr 21, 2023

ecsimsw Apr 21, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 23, 2023

ecsimsw Apr 21, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 21, 2023

ecsimsw Apr 22, 2023

ecsimsw Apr 21, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 21, 2023

ecsimsw Apr 21, 2023

ecsimsw Apr 21, 2023

hyunsb Apr 21, 2023

		Electronic[] authMethodNameGroup =
		temp.isEmpty() ? null : ElectronicArrayUtil.listToArray(temp);


		public class ElectronicArrayUtil {

		static Electronic[] listToArray(List<Electronic> list) {

Java Assignment3 upload by HyunsooJung #21

Are you sure you want to change the base?

Java Assignment3 upload by HyunsooJung #21

Conversation

hyunsb commented Apr 17, 2023 • edited Loading

질문

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyunsb Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecsimsw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyunsb commented Apr 17, 2023 •

edited

Loading

hyunsb Apr 21, 2023 •

edited

Loading