-
Notifications
You must be signed in to change notification settings - Fork 0
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 WooSeokLee #14
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 우석님~
일단 과제 수행하시느라 고생 많으셨습니다.
코드도 전반적으로 깔끔하게 작성되어 있고, 이해가 잘되는 코드네요:)
제가 남겨드린 코멘트 참고하여 수정해 볼 수 있는 부분은 수정해 보시길 바랍니다!
그리고 commit 단위가 조금 더 세분화되면 좋을 것 같아요.
실제로 현업에서 근무를 하게 되면 commit message 만으로도 어떤 작업이 수행되었는지 알 수 있도록 메세지를 작성하는 것이 일반적이랍니다.
다음 과제 제출 시에는 feat, doc, fix 등등 commit 메세지를 규격화하여 한번 작성해 보세요:)
수고하셨습니다~!
|
||
public Electronic( String modelName, Company companyName, String dateOfMade, AuthMethod[] authMethod) { | ||
objectNo++; | ||
this.productNo = LocalDate.now().format(DateTimeFormatter.ofPattern("yyMMdd")) + String.format("%04d", objectNo) ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋은 아이디어네요 잘하셨어요~ 다만 String 연산에서 + 는 속도가 많이 느리다고 알려져 있어요! 지금처럼 데이터양이 크지 않고 연산이 복잡하지 않은 경우에는 +도 괜찮지만 다른 방법이 있을 거예요! 한번 공부해볼까요!
this.productNo = LocalDate.now().format(DateTimeFormatter.ofPattern("yyMMdd")) + String.format("%04d", objectNo) ; | ||
this.modelName = modelName; | ||
this.companyName = companyName; | ||
this.dateOfMade = dateOfMade; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생산일자가 원하는 값으로 들어오지 않을 수 있으니, 유효성 검증 이후에 값을 매핑해주는 게 좋을 것 같아요. 유효성 검증에서 어긋나면 기본적으로 매핑해줄 값을 넣어주는 것도 방법이겠죠?ㅎㅎ
private Company companyName; //제조회사명 | ||
private String dateOfMade; //생산일자 | ||
|
||
private AuthMethod[] authMethod; //본인인증방법 , 배열로정의 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사소한 것이지만 인증 방법이 복수개니까 복수형의 느낌이 나는 authMethods로 바꿔보는 건 어떨까요~?ㅎㅎ
|
||
private AuthMethod[] authMethod; //본인인증방법 , 배열로정의 | ||
|
||
private static int objectNo = 0; //등록된제품순서. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static 변수는 일반 변수보다 위에 선언되는 게 좋습니다! 맨 위로 옮겨보세용~
this.userEmail = userEmail; | ||
this.userBirthDate = userBirthDate; | ||
this.electronicDevices = electronicDevices; | ||
this.registerTime = LocalDate.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LocalDate.now()의 반환 값은 날짜이기 때문에 시간을 뜻하는 자료 형으로 바껴야 하지 않을까요~?
public class Users { | ||
private User[] userList; | ||
|
||
private Users() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요것도 과감히 버립시다:)
} | ||
|
||
//2. 회원아이디 userId를 통해 인자로 주어진 회원번호에 해당하는 회원을 반환하는 함수를 작성하시오. | ||
public User findByUserId(String userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋아요 잘하셨어요! 지금도 잘하셨는데 숙제를 하나 내드리면! stream과 filter를 이용해서 한번 변경해 보세용!
if(instance == null) { | ||
instance = new Electronics(); | ||
} | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else의 경우 instance를 return 해야 하지 않을까요?
} | ||
|
||
//2.전자제품 일련번호 productNo를 통해 인자로 주어진 일련번호에 해당하는 전자제품을 반환하는 함수를 작성하시오. | ||
public Electronic findByProductNo(String productNo){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 stream과 filter를 이용해서 구현할 수 있을 것 같아요 😄
public Electronic[] groupByAuthMethod(AuthMethod authMethod){ | ||
List<Electronic> groupAuthList = new ArrayList<>(); | ||
for(Electronic electronic : electronicList) { | ||
if(electronic.getAuthMethod().equals(authMethod)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for문을 한번 더 돌아서 탐색해야 하지 않을까요? getAuthMethod()의 반환 값이 AuthMethod[] 이니까 equals로 비교가 바로 안될 것 같아요:)
제대로 한건지 잘모르겠네요... 코드리뷰부탁드리겠습니다.
특히나 실습3번문제에서 좀 어려움을 느꼈던것같습니다. 배열반환관련해서는 빡센리뷰 부탁드립니다.