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

학생 관리 프로그램 - 박대영 #3

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

Conversation

daeyoung0726
Copy link
Member

@daeyoung0726 daeyoung0726 commented Apr 5, 2024

1. 내가 개발한 기능

학생 등록, 학생 전체 조회, 학생 검색, 학생 정보 수정, 학생 삭제, 학번, 이름, 평균 순 정렬

2. 내가 개발할 때 유의깊게 개발한 부분

객체지향 고려.
하나의 메서드에서는 하나의 역할 수행하도록 고려하여 코드 작성.

3. 내가 개발하면서 들었던 의문 사항

학생 정보를 담을 때, Map을 사용하여 학생을 검색하는 시간복잡도를 줄이고자 하였음. 그러나, 정렬 후 출력하는 메서드를 만들었는데, 그 과정에서의 고민.
Map을 사용하면 학생 검색에서의 이점이 있으나, 정렬하기 위해서는 List에 다시 값을 담고 정렬을 진행 후 값을 출력하니 내가 작성한 프로그램에서는 한명의 학생을 검색하는 과정을 제외하고는 List가 더 이점이 있다고 생각.

Copy link
Member

@asn6878 asn6878 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 1등 이시네요. 따봉 드릴게요ㅎ 👍

Comment on lines +131 to +164
case 2 -> {
System.out.print("조회할 학생의 이름을 입력하세요: ");
String name = sc.next();
Student student = getStudent(name);

if (student == null) {
System.out.println("해당 학생은 존재하지 않습니다.");
continue;
}

read(student);
}
case 3 -> {
readAll();
}

case 4 -> {
System.out.print("수정할 학생의 이름을 입력하세요: ");
String name = sc.next();
Student student = getStudent(name);

if(student == null) {
System.out.println("해당 학생은 존재하지 않습니다.");
continue;
}

inputInfo("update", student);
}
case 5 -> {
System.out.print("삭제할 학생의 이름을 입력하세요: ");
String name = sc.next();

delete(name);
}
Copy link
Member

Choose a reason for hiding this comment

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

여기서 학생의 정보를 조회, 수정, 삭제 할때 학생의 존재여부를 검사하는 코드가 중복되어 나타나는 것으로 보입니다.
또한, 해당 로직이 delete의 경우에는 메소드 내부에 있고, 나머지는 switch문 내부에 작성이 되어있어 자칫 가독성이 떨어질 수 있다고 생각합니다!

getStudent() 메서드 내부에서 조회 실패시 발생하는 이벤트를 한번에 핸들링 해주면 중복 코드가 줄것같습니다!!

Comment on lines +5 to +14
private final Map<String, Student> students = new HashMap<>(); // 학생 정보 저장
private static final Scanner sc = new Scanner(System.in);

/* 학생 추가 */
private void add(int stdNum, String name, int kor, int eng, int math) {

Student student = new Student(stdNum, name, kor, eng, math);
students.put(name, student);
System.out.println(name + "학생 추가 완료");
}
Copy link
Member

@asn6878 asn6878 Apr 10, 2024

Choose a reason for hiding this comment

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

앞서 시간복잡도를 고려해 HashMap 구조를 선택한 것이 매우 인상적이었어요. 배워갑니다👍

다만, HashMap을 사용시 우려되는 문제점으로 동명이인 문제가 있어 보입니다.
현제 학생을 추가하는 부분에있어 이름이 같은 (Key 값이 동일한) 객체가 들어왔을때, value값이 업데이트되어 이전에 있던 학생의 정보가 덮어씌워져 버리는 구조인것 같아요!

이름 중복을 처리하기 위해 어떤 고민을 해보아야할지? 생각 해보실수 있는 좋은 기회라 생각합니다 :)

Copy link
Member

@jinlee1703 jinlee1703 left a comment

Choose a reason for hiding this comment

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

과제하느라 고생 많으셨습니다 :)

break;
}
switch (select) {
case 1 -> {
Copy link
Member

Choose a reason for hiding this comment

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

case문을 정수로 작성하게 되면 추후 유지보수 관점에서 해당 case문이 어떤 역할을 하는지 혼동이 올 수 있습니다. 어떻게 하면 가독성 측면에서 개선할 수 있을 지 고민해보시면 좋을 것 같습니다!



/* 정보 입력 메서드 (추가, 업데이트) */
private void inputInfo(String type, Student student) {
Copy link
Member

Choose a reason for hiding this comment

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

마찬기지로 type을 String으로 받게 되면 IDE의 도움을 받기 어렵습니다. 보다 IDE를 잘 활용하기 위해서 고민을 해보시면 좋을 것 같네요!

return avg;
}

public void update(int stdNum, String name, int kor, int eng, int math) {
Copy link
Member

Choose a reason for hiding this comment

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

예상치 못한 오류를 방지하기 위해 setter를 통해 일일이 값을 변경하는 것이 아닌 update 메서드를 별도로 선언하셨군요!

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.

3 participants