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 JunHoMun #34

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

Conversation

gromit-95
Copy link

요번 과제 많이 어렵네요... 코드리뷰 부탁드립니다... ㅠ

@MinChul-Son MinChul-Son self-requested a review April 20, 2023 14:19
Copy link

@MinChul-Son MinChul-Son left a comment

Choose a reason for hiding this comment

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

안녕하세요 준호님~~

과제 진행하신다고 고생많으셨습니다!!

코드를 작성하실 때는 자바에서 권장하는 코드 스타일에 맞게 코드를 작성해주시면 좋을 것 같아요!

Google Code Style Guide를 참고해보시죠!

깃 커밋을 조금 더 세분화해서 작업의 단위를 잘게 쪼개보는 것도 좋을 것 같아요.

그리고 pr을 올리실 때 본인만의 템플릿을 가지고 계시면 좋을 것 같습니다.
ex)

  • 내가 중점적으로 생각한 부분
  • 작업 내용
  • 궁금한 점
  • 함께 이야기하고 싶은 부분

과 같은 것들이 포함되면 좋습니다!

대부분이 중복된 코멘트라 최대한 줄였어요~! 이해가 되지 않는 부분은 편하게 이야기해주세요.


public class Electronic {

public enum companyName {SAMSUNG,LG,APPLE}

Choose a reason for hiding this comment

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

Enum도 별도의 클래스로 분리시켜주시면 좋을 것 같습니다~

Choose a reason for hiding this comment

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

Suggested change
public enum companyName {SAMSUNG,LG,APPLE}
public enum CompanyName {SAMSUNG,LG,APPLE}

private authMethod[] authMethods;
private companyName company;
private String productNo;
private static int productionCount=1;

Choose a reason for hiding this comment

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

static 변수는 일반 멤버 변수보다 위로 위치시켜주는 것이 관례입니다😀

private authMethod[] authMethods;
private companyName company;
private String productNo;
private static int productionCount=1;

Choose a reason for hiding this comment

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

Suggested change
private static int productionCount=1;
private static int productionCount = 1;

사소한거지만 코드 스타일도 맞춰주시면 좋을 것 같아요!! 인텔리제이의 자동정렬 기능을 활용해보시죠~

public class Electronic {

public enum companyName {SAMSUNG,LG,APPLE}
public enum authMethod {FINGERPRINT, PATTERN, PIN, FACE}

Choose a reason for hiding this comment

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

Suggested change
public enum authMethod {FINGERPRINT, PATTERN, PIN, FACE}
public enum AuthMethod {FINGERPRINT, PATTERN, PIN, FACE}


StringBuffer generateProductNo;
generateProductNo = new StringBuffer(LocalDate.now().getYear()%100);
generateProductNo.append(String.format("%02",LocalDate.now().getDayOfMonth()));

Choose a reason for hiding this comment

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

월이니까 getMonthValue을 사용해야하지 않을까요?

return null;
}

public User copy(User user) {// Deep copy

Choose a reason for hiding this comment

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

👍🏻

private static final int DEFAULT =10;
private int size;
private int capacity;
private Electronics(){

Choose a reason for hiding this comment

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

Suggested change
private Electronics(){
private Electronics(){

public class Electronics {

private static Electronics allElectronics;
public Electronics getInstance(){

Choose a reason for hiding this comment

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

static이어야 하지 않을까요?

size = 0;
}

public static Electronics getAllElectronics() {

Choose a reason for hiding this comment

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

getInstance만을 통해 객체 인스턴스를 제공받을 수 있게하면 좋을 것 같아요.

이 메서드는 어디서 사용되나요?

return allElectronics;
}

public static void setAllElectronics(Electronics allElectronics) {

Choose a reason for hiding this comment

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

조금 헷갈리셨던 부분이 있으셨던거 같네요

Electronic[] groupByAuthMethodArray= new Electronic[size];
int j=0;
for(int i=0;i<size;i++){
if(electronicList[i].getCompany().equals(authMethod.name())){

Choose a reason for hiding this comment

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

getCompany()가 아니라 getAuthMethod()가 되어야할것 같고 반환형이 AuthMethod[]라서 코드가 바뀌어야할 것 같네요~

newUser.setUserPhoneNumber(user.getUserPhoneNumber());
newUser.setUserEmail(user.getUserEmail());
newUser.setUserBirthDate(user.getUserBirthDate());
newUser.setElectronicDevices(user.getElectronicDevices());

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