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

경북대 BE_권도윤_1주차 과제(1~3단계) #200

Open
wants to merge 10 commits into
base: doyooon
Choose a base branch
from

Conversation

doyooon
Copy link

@doyooon doyooon commented Jun 28, 2024

No description provided.

Copy link

@MangKyu MangKyu left a comment

Choose a reason for hiding this comment

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

안녕하세요 도윤님ㅎㅎ 만나서 반갑습니당 🙇
PR 올려주신 것 확인했고, 몇 개선할 부분이 보여서 코멘트들 남겨두었어요ㅎㅎ
확인해보시고, 필요한 부분 반영해주시면 좋을 것 같습니당! 파이팅입니다!! 🔥🔥🔥

# spring-gift-product
- [x] 상품을 조회, 추가, 수정, 삭제할 수 있는 HTTP API를 구현한다.
Copy link

Choose a reason for hiding this comment

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

요구사항 정리한 부분 좋네요 👍

@@ -2,10 +2,13 @@

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.context.annotation.Bean;
import org.springframework.web.filter.HiddenHttpMethodFilter;
Copy link

Choose a reason for hiding this comment

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

불필요한 import 문은 제거해도 좋을 것 같습니다 🙏

package gift;

public class Product {
Long id;
Copy link

Choose a reason for hiding this comment

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

private과 같은 적절한 접근제어자를 사용하면 접근을 제한할 수 있습니다ㅎㅎ
자바가 제공하는 접근제어자에 대해서도 알아보시면 좋을 것 같아요!

Suggested change
Long id;
private Long id;


}

public Product(Long id, String name, int price, String imageUrl){
Copy link

Choose a reason for hiding this comment

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

불필요한 set 관련 메서드는 최소화하는 것이 좋을 것 같습니다

id BIGINT AUTO_INCREMENT PRIMARY KEY,
name VARCHAR(255) NOT NULL,
price DECIMAL(10, 2) NOT NULL,
image_url VARCHAR(255)
Copy link

Choose a reason for hiding this comment

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

image url이 null이 될 수 없다면, 해당 부분에도 not-null 제약 조건을 걸어주는 것이 좋을 것 같습니다ㅎㅎ

return "redirect:/";
}

@PostMapping ("/update")
Copy link

Choose a reason for hiding this comment

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

수정을 위한 HTTP Method로는 PUT, PATCH 그리고 삭제를 위해서는 DELETE가 존재합니다.
따라서 용도에 맞는 HTTP Method를 활용하는 것이 바람직할 것 같습니다ㅎㅎ

Copy link

Choose a reason for hiding this comment

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

그리고 API 요청의 prefix로 /products 와 같이 구분을 두면 좋을 것 같습니다!
이후에 상품 외에 다른 추가/삭제/수정 요청이 생기면 문제가 생길 수 있습니다ㅎㅎ

@Autowired
public ProductRepository(JdbcTemplate jdbcTemplate, DataSource dataSource) {
this.jdbcTemplate = jdbcTemplate;
this.insertProduct = new SimpleJdbcInsert(dataSource)
Copy link

Choose a reason for hiding this comment

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

SimpleJdbcInsert를 활용한 부분 좋네요! 👍

@doyooon doyooon changed the base branch from main to doyooon July 9, 2024 11:48
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