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] 정다영 미션 제출합니다. #274

Open
wants to merge 38 commits into
base: day024
Choose a base branch
from

Conversation

day024
Copy link

@day024 day024 commented May 27, 2024

✔️안녕하세요 리뷰어님! 이번주도 미션하시느라고 고생이 많으셨습니다
pr 충돌이 발생하는데 pr먼저 보낸 후 수정중입니다만 ㅠㅠ
코드 자체가 꼬여서 이전으로 되돌렸는데 최대한 빨리해결해보겠습니다.
부족한 점이 많은 것 같아서 리뷰하시기 전까지 계속 리팩터링 진행해보겠습니다.

🍀[Spring Core commit]


✅중점으로 진행한 점

  • 기존의 repository -> dto service로 분리해서 구조를 수정했습니다. requestresponse로 나누어 진행했습니다.
  • 공통 피드백 내용 일부 반영

✅미션 수정해야할 사항들

  • date, time 형식을 수정으로 인해 부족한 예외처리 리팩터링
  • 구조 리팩터링

✅미션을 진행하며 생긴 궁금한 점들/봐주시면 좋은 점들

  • 코드가 보기에 불편한 점은 없는지
  • 특히 DTO를 나누면서 오류가 많았습니다. 여러 코드를 참고하였습니다.
    우선 제출은 하였지만 DTO에 대한 분리를 진행 못했습니다 ㅠㅠ.

이외에도 여러 방면에서 조언해주시면 반영하도록 하겠습니다.

@day024 day024 closed this May 27, 2024
@day024 day024 deleted the SpringCore branch May 27, 2024 17:35
@day024 day024 restored the SpringCore branch May 27, 2024 17:35
@day024 day024 changed the title Spring core [Spring core] 정다영 미션 제출합니다. May 27, 2024
@day024 day024 reopened this May 27, 2024
@day024 day024 changed the title [Spring core] 정다영 미션 제출합니다. [Spring Core] 정다영 미션 제출합니다. May 27, 2024

Choose a reason for hiding this comment

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

공통 피드백에서 나온 내용을 잘 반영하셨네요!

public long deleteById(long id) {
String sql = "DELETE FROM reservation WHERE id = ?";
int rowsAffected = jdbcTemplate.update(sql, id);
return rowsAffected > 0 ? id : -1;

Choose a reason for hiding this comment

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

rowsAffected가 0인 경우에는 controller에서 예외처리를 하셨는데
DAO 에서 바로 예외처리를하면 해당 예외가 데이터베이스와 관련된 것임을 명확히 할 수 있지 않을까요?

public long deleteById(long id) {
String sql = "DELETE FROM time WHERE id = ?";
int rowsAffected = jdbcTemplate.update(sql, id);
return rowsAffected > 0 ? id : -1;

Choose a reason for hiding this comment

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

reservationDAO 와 동일합니다!


return ResponseEntity.created(location).body(reservation);
}

Choose a reason for hiding this comment

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

findById와 save 메서드를 분리하신 이유가 있을까요?

Choose a reason for hiding this comment

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

service 계층의 역할이 DAO와 controller를 단순히 이어주는 역할만 하고 있네요!
서비스 계층은 왜 필요할까요?

https://kokodakadokok.tistory.com/entry/%EC%84%9C%EB%B9%84%EC%8A%A4-%EA%B3%84%EC%B8%B5%EC%9D%80-%EC%99%9C-%ED%95%84%EC%9A%94%ED%95%A0%EA%B9%8C
구글링하다 발견한 승용님이 쓰신 블로그인데 한번 참고해보시면 좋을 것 같아요!

private final ReservationDAO reservationDAO;

@Autowired
public ReservationService(DataSource dataSource) {

Choose a reason for hiding this comment

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

의존성 주입에서 new 키워드를 쓰지 않는것이 좋습니다!

이번주차 의존성 주입 학습테스트 부분을 한번 다시 복습해보시면 좋을 것 같네요 :)

@Kyuwon-Choi
Copy link

안녕하세요 다영님!
이번주도 리뷰 하시느라 고생 많으셨습니다!

pr 충돌때문에 그런지 DTO부분은 보이지 않네요 ㅠ

전반적으로 코드 끝 줄에 개행이 없어요! 그 부분 수정해주시면 좋을 것 같습니다!

후에 수정되시면 다시 리뷰 드리겠습니다!
고생 많으셨어요!

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