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단계 - ν…ŒμŠ€νŠΈλ₯Ό ν†΅ν•œ μ½”λ“œ 보호 #698

Open
wants to merge 23 commits into
base: jinan159
Choose a base branch
from

Conversation

jinan159
Copy link

κ·œλ‚¨λ‹˜ μ•ˆλ…•ν•˜μ„Έμš”.
μˆ˜κ°•μƒ 김진완 μž…λ‹ˆλ‹€.

μ˜€λžœλ§Œμ— 리뷰 μš”μ²­ λ“œλ¦½λ‹ˆλ‹€πŸ˜…

μ‹œκ°„ 관계상 ν†΅ν•©ν…ŒμŠ€νŠΈλŠ” μž‘μ„±ν•˜μ§€ λͺ»ν–ˆκ³ , μ• ν”Œλ¦¬μΌ€μ΄μ…˜ μ„œλΉ„μŠ€μ˜ λ‹¨μœ„ν…ŒμŠ€νŠΈλ§Œ μž‘μ„±ν–ˆμŠ΅λ‹ˆλ‹€.


μ•„λž˜λŠ” ν…ŒμŠ€νŠΈ μΌ€μ΄μŠ€λ“€ μž…λ‹ˆλ‹€. (총 60개)

image
image
image
image

- μ£Όλ¬Έ μƒνƒœλ₯Ό ν•œλˆˆμ— λ³Ό 수 μžˆλ„λ‘ μ£Όλ¬Έ μš”κ΅¬μ‚¬ν•­ λ³€κ²½
Copy link

@kyucumber kyucumber left a comment

Choose a reason for hiding this comment

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

μš”κ΅¬μ‚¬ν•­ 별 λˆ„λ½ 없이 κΌΌκΌΌν•˜κ³  κΉ”λ”ν•˜κ²Œ ν…ŒμŠ€νŠΈλ₯Ό 잘 μž‘μ„±ν•΄μ£Όμ‹  것 κ°™λ„€μš”.

κ°„λ‹¨ν•œ μ½”λ©˜νŠΈ λ‚¨κ²¨λ‘μ—ˆμœΌλ‹ˆ 확인 ν›„ μš”μ²­ μ£Όμ‹œλ©΄ λ¨Έμ§€ν•˜λ„λ‘ ν•˜κ² μŠ΅λ‹ˆλ‹€. πŸ™‡

Comment on lines -7 to +8
public interface MenuGroupRepository extends JpaRepository<MenuGroup, UUID> {
public interface MenuGroupRepository {

Choose a reason for hiding this comment

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

JpaRepository와 λ„λ©”μΈμ˜ λ ˆνŒŒμ§€ν† λ¦¬λ₯Ό 잘 λ‚˜λˆ„μ–΄μ£Όμ…¨λ„€μš” πŸ‘

import java.util.List;
import java.util.UUID;

public interface JpaProductRepository extends ProductRepository, JpaRepository<Product, UUID> {

Choose a reason for hiding this comment

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

ProductRepository와 같은 λ ˆνŒŒμ§€ν† λ¦¬λ“€μ΄ 도메인 νŒ¨ν‚€μ§€μ— μžˆλŠ” 것은 이해가 κ°€λŠ”λ°μš”, μΈν„°νŽ˜μ΄μŠ€κΈ΄ ν•˜μ§€λ§Œ 사싀상 Jpa와 직접 μ—°κ΄€λ˜μ–΄ 있기 λ•Œλ¬Έμ— domain μ˜μ—­μ΄ μ•„λ‹ˆλΌκ³  λ³Ό 수 μžˆμ„ 것 κ°™μ•„μš”. 이에 λŒ€ν•΄ μ–΄λ–»κ²Œ μƒκ°ν•˜μ‹œλ‚˜μš”?

Copy link
Author

Choose a reason for hiding this comment

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

λ§μ”€ν•˜μ‹ λŒ€λ‘œ, Jpa κ΄€λ ¨ κΈ°λŠ₯이닀 λ³΄λ‹ˆ domain λ³΄λ‹€λŠ” infra μ˜μ—­μ— μžˆλŠ”κ²Œ μ–΄μšΈλ¦΄ 것 κ°™μŠ΅λ‹ˆλ‹€!

import kitchenpos.testsupport.FakeMenuGroupRepository
import kitchenpos.testsupport.MenuGroupFixtures.createMenuGroup

class MenuGroupServiceFindAllTest : ShouldSpec({

Choose a reason for hiding this comment

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

FindAllμ΄λ‚˜ Create와 같은 λ©”μ„œλ“œ λ‹¨μœ„λ‘œ λ‚˜λˆ„κ²Œ λ˜λŠ” 경우 이후 λ§Žμ€ λ©”μ„œλ“œκ°€ 생기면 ν…ŒμŠ€νŠΈ ν΄λž˜μŠ€κ°€ λ„ˆλ¬΄ λ§Žμ•„μ§ˆ 것 κ°™μ•„μš”.

쑰회 / μˆ˜μ • / μ‚­μ œμ™€ 같은 λ‹¨μœ„λ‘œ λ‚˜λˆ„κ±°λ‚˜ ν˜Ήμ€ λ³΅μž‘λ„κ°€ λ†’μ•„μ§€λŠ” μ‹œμ μ— 클래슀 자체λ₯Ό λ‚˜λˆ„λŠ” 것도 방법일 것 κ°™μŠ΅λ‹ˆλ‹€.

Copy link
Author

Choose a reason for hiding this comment

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

쑰회 / μˆ˜μ • / μ‚­μ œ λ‹¨μœ„λ‘œ λ‚˜λˆ„κ±°λ‚˜, ν•˜λ‚˜λ‘œ μž‘μ„±ν–ˆλ‹€ λ³΅μž‘λ„κ°€ λ†’μ•„μ§€λŠ” μ‹œμ μ— λΆ„λ¦¬ν•˜λŠ” 방법도 μ’‹λ‹€κ³  μƒκ°ν•©λ‹ˆλ‹€!
λ‹€λ§Œ, 각 κΈ°λŠ₯(λ©”μ„œλ“œ)λ§ˆλ‹€ ν•„μš”ν•œ Fixture 의 μƒνƒœλ‚˜ ν…ŒμŠ€νŠΈ 쑰건이 μ‘°κΈˆμ”© λ‹¬λΌμ„œ, λΆ„λ¦¬ν•˜κΈ° μœ„ν•΄ λ³„λ„μ˜ 클래슀둜 λ‚˜λˆ΄μ—ˆμŠ΅λ‹ˆλ‹€.

이처럼 클래슀λ₯Ό 많이 λ‚˜λˆ„κ²Œ 되면 λ°œμƒν•  수 μžˆλŠ” 단점을 생각해보면,,
μ €λŠ” λ‹Ήμž₯ μƒκ°λ‚˜λŠ”κ±΄ 도메인 변경이 λ°œμƒν–ˆμ„λ•Œ, μˆ˜μ •ν•΄μ•Όν•˜λŠ” μ˜μ—­μ΄ λ§Žμ•„μ§ˆ 수 μžˆκ² λ‹€λŠ” 생각이 λ“€μ—ˆλŠ”λ°μš”.
ν•˜μ§€λ§Œ 클래슀λ₯Ό λ‚˜λˆ”μœΌλ‘œμ„œ ν…ŒμŠ€νŠΈ μž‘μ„±κ³Ό 관리가 μš©μ΄ν•œ μž₯점도 μžˆμ–΄μ„œ μ €λŠ” 클래슀λ₯Ό λ‚˜λˆ„λŠ” 선택을 ν–ˆμ—ˆμŠ΅λ‹ˆλ‹€.

ν˜Ήμ‹œ ν…ŒμŠ€νŠΈ ν΄λž˜μŠ€κ°€ λ„ˆλ¬΄ 많으면 또 μ–΄λ–€ 단점이 μžˆμ„κΉŒμš”?

import kitchenpos.domain.OrderStatus
import kitchenpos.domain.OrderTable

class FakeOrderRepository : OrderRepository {

Choose a reason for hiding this comment

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

Fakeλ₯Ό μ΄μš©ν•΄ ν…ŒμŠ€νŠΈλ₯Ό 잘 μž‘μ„±ν•΄μ£Όμ…¨λ„€μš”. πŸ‘

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