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

임채현(B) - Spring Project with JPA and H2 Database #4

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

Conversation

Dolchae
Copy link

@Dolchae Dolchae commented Jul 11, 2024

1. 스프링 부트 + 로컬 데이터 베이스 연동

image

2. 다음 조건의 REST API를 JPA를 통해 구현

특정 학생의 수강 과목 추가
image

특정 학생의 수강 과목 삭제
image

특정 학생의 수강 과목 조회
image


어설픈 부분들이 아주 많습니다😂 더 많겠지만 일단 제가 찾은 문제점들입니다!

  1. STUDENT와 COURSE 테이블 데이터 등록은 H2 console로 진행하였습니다. (그래서 student,course 컨트롤러와 서비스 구현이 되어있지 않습니다.)
  2. 특정 학생의 이름과 과목의 이름이 아닌, 학생의 id와 과목 id를 사용하여 enrollment를 추가합니다.
  3. 학생의 수강 과목 조회 시(GET매핑), 같은 정보들이 반복해 나오는 문제가 있습니다.(코드에 문제가 있을 것 같습니다.)

제가 제대로 공부하지 못한 부분들이 많은 것 같습니다 더 열심히 공부해보겠습니다..!😄

@Dolchae
Copy link
Author

Dolchae commented Jul 11, 2024

Copy link
Contributor

@coke98 coke98 left a comment

Choose a reason for hiding this comment

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

끝까지 노력하셨던 점이 보여서 좋았어요. 아쉬웠던 점에 대해서는 프로젝트를 진행하면서 앞으로도 지금보다 더 복잡한 도메인 관계를 다뤄볼 기회가 많을거에요. 그때마다 도메인, 레이어 간의 관계에 대해 많은 고민을 해보시고, 개선해보시면 좋을 것 같습니다. 정말 고생했어요 :)

return enrollmentRepository.save(enrollment);
}

public void cancelEnrollment(Student student, Course course) {
Copy link
Contributor

Choose a reason for hiding this comment

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

트랜잭션을 활용해보는 걸 고민해보면 좋을 것 같아요
트랜잭션을 왜 쓰고 그럼 어떤 단위로 어노테이션을 달아야할지 생각해보면 좋을 것 같습니다

public class EnrollmentController {
private final StudentRepository studentRepository;
private final CourseRepository courseRepository;
private final EnrollmentRepository enrollmentRepository;
Copy link
Contributor

Choose a reason for hiding this comment

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

컨트롤러에서 레포지토리를 참조하는 건 일반적인 방법은 아니긴해요, 컨트롤러, 서비스, 레포지토리가 어떤 아키텍처를 이루고 있는지 공부해보면 좋을 것 같습니다!

Course course = courseRepository.findById(courseId)
.orElseThrow(() -> new IllegalArgumentException("ID에 해당하는 과목을 찾을 수 없습니다: " + courseId));

enrollmentService.cancelEnrollment(student, course);
Copy link
Contributor

Choose a reason for hiding this comment

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

보시면 현재 데이터 베이스에 접근하는 코드가 컨트롤러, 서비스 둘다 존재하고 있죠?
서비스내 cancelEnrollment(student, course)에서 데이터베이스 접근과 관련된 모든 관심사를 해결해보는게 좋을 것 같아요.
그리고 해당 메서드에 트랜잭션을 달아놓는다면 명확한 관심사 분리, 트랜잭션의 장점을 모두 가질 수 있을 겁니다~!

}

@GetMapping("/{studentId}/courses")
public List<Enrollment>getEnrolledCourses(@PathVariable int studentId) {
Copy link
Contributor

@coke98 coke98 Jul 12, 2024

Choose a reason for hiding this comment

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

JPA에서 엔티티 간의 관계를 가져올 때 반복이 발생한다는 건 서로간의 양방향 관계가 설정되어 있다보니 참조가 서로 반복되면서 일어나는 걸로도 의심돼요.
방법중 하나는 반환값으로 엔티티인 Enrollment 자체 대신 DTO 클래스를 두고 변환하여 사용해보면 어떨까 싶어요. 필요한 정보만 정확하게 전달할 수 있고, 엔티티와 표현 계층을 분리할 수 있어서 이점이 있을 겁니다

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Column(name = "course_id", nullable = false)
private int course_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

컬럼 명은 스네이크 케이스지만, 자바 변수에서는 모두 카멜 케이스로 쓰는게 네이밍 컨벤션에 맞겠죠!?

import java.util.List;

@Getter
@Setter
Copy link
Contributor

@coke98 coke98 Jul 12, 2024

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