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

[Spring Core] 정연희 미션 제출합니다 #278

Open
wants to merge 9 commits into
base: spig0126
Choose a base branch
from

Conversation

spig0126
Copy link

@spig0126 spig0126 commented Jun 1, 2024

No description provided.

Copy link

@kyY00n kyY00n left a comment

Choose a reason for hiding this comment

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

이번 미션에서 jpa 를 적용해보신 소감이 궁금합니다 :D
+) 그러넫 기존 코드들이 반영이 안되어서 코드가 안돌아가는 부분이 있어요! 체리픽 하기 전에 작업하시던 브랜치 있죠? 거기서 리뷰 드린거 반영하시고 같은 브랜치 이름으로 force push 해주시면 좋을 것 같아요. (PR 은 다시 올리지 않으셔도 돼요!)


@Service
public class TimeService {
@Autowired
Copy link

Choose a reason for hiding this comment

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

필드 주입의 단점을 알려주세요!

import java.util.List;

@Service
public class TimeService {
Copy link

Choose a reason for hiding this comment

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

서비스 계층을 나눠주셨군요? 계층을 나눴을 때의 장점은 어떤 것이 있었나요?

throw new TimeNotFoundException("Time with id " + id + " not found");
}
return timeRepository.getReferenceById(id);
}
Copy link

Choose a reason for hiding this comment

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

데이터베이스에 두 번 쿼리하는 과정을 한 번으로 줄여볼 수 없을까요?

if (!timeRepository.existsById(id)) {
throw new TimeNotFoundException("Time with id " + id + " not found");
}
Time time = getTimeById(id);
Copy link

Choose a reason for hiding this comment

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

getTimeById(id) 메서드에서 이미 존재하는 아이디인지 검증하고 있는 것 맞죠? 제거해도 좋을 것 같아요.

Reservation newReservation = reservationService.createReservation(reservation);
String uri = "/reservations/" + newReservation.getId();
return ResponseEntity.created(URI.create(uri)).body(newReservation);
}
Copy link

Choose a reason for hiding this comment

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

서비스 계층을 잘 분리해주셔서 컨트롤러 로직이 깔끔해보입니다!! :+1

public String setName(String name) {return this.name = name;}
public String setDate(String date) {return this.date = date;}
public String setTime(String time) {return this.time = time;}
}
Copy link

Choose a reason for hiding this comment

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

무분별한 setter 선언은 객체지향 프로그래밍을 지키기 어렵게 하기 때문에 지양하는 것이 좋습니다..!
다음 글을 한 번 읽어보세요. setter 쓰지 말라고만 하고 가버리면 어떡해요

if(!timeRepository.existsById(time.getId())) {
throw new TimeNotFoundException("Time with id " + time.getId() + " not found");
}
reservation.setTime(time);
Copy link

Choose a reason for hiding this comment

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

setter를 사용하지 않아도 reservation 객체의 time 필드는 그대로 일것 같습니다.



@Test
void 십단계() {
Copy link

Choose a reason for hiding this comment

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

오 이거 추가하셨군요 !! 굳입니당 :D

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