-
Notifications
You must be signed in to change notification settings - Fork 0
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-REFACTOR] 배틀 기능 MongoDB 사용하도록 리팩토링 #338
Conversation
…4-pokerogue-helper into be/refactor/#331-refactor-battle-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 미아!
코드를 보니 계산이 들어가서 그런지 꽤나 복잡하네요 🥹 정말 수고 많으셨습니다
몇가지 의견을 달았는데 리뷰 혹은 반영 주세요!
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/DataInitializer.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/data/TypeMatching.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/data/Weather.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleMultiplier.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleService.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleService.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/TypeMultiplierProvider.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/test/java/com/pokerogue/helper/battle/data/WeatherTest.java
Outdated
Show resolved
Hide resolved
...kerogue/src/test/java/com/pokerogue/helper/battle/repository/TypeMatchingRepositoryTest.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
짚을 만한 건 이미 폴라가 다 짚어서 approve합니다!
구현 수고하셨습니다!
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleMultiplier.java
Outdated
Show resolved
Hide resolved
/noti @폴라 리뷰 반영 및 코멘트 달았습니당! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 반영 고생하셨습니다!!
반영해주신 부분에 의견 두가지 달았는데 소소한거라 RC 보다는 읽고 반영해주시면 될 것 같아요!
어푸룹 하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다👍
코드가 꼼꼼하네요
저는 계산 기능은 서비스보다 도메인에 있는게 자연스럽다고 생각해요
서비스 계층에 있기엔 비즈니스로직은 아니고 하나의 도메인으로 분리 될만큼 코어한 기능이라고 생각합니다.
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleCalculator.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleMultiplier.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleMultiplier.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/TypeMultiplierProvider.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/TypeMultiplierProvider.java
Show resolved
Hide resolved
...kerogue/src/test/java/com/pokerogue/helper/battle/service/WeatherMultiplierProviderTest.java
Show resolved
Hide resolved
backend/pokerogue/src/test/java/com/pokerogue/helper/battle/service/WeatherServiceTest.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/test/java/com/pokerogue/helper/pokemon/data/PokemonTest.java
Outdated
Show resolved
Hide resolved
backend/pokerogue/src/test/java/com/pokerogue/helper/battle/service/BattleMultiplierTest.java
Show resolved
Hide resolved
backend/pokerogue/src/test/java/com/pokerogue/helper/battle/service/BattleMultiplierTest.java
Outdated
Show resolved
Hide resolved
/noti @종이 : 모두 반영 및 코멘트 남겼습니다! |
backend/pokerogue/src/main/java/com/pokerogue/helper/battle/service/BattleMultiplier.java
Show resolved
Hide resolved
e1ad0b3
to
ee8ff8e
Compare
🍄 PR 확인 사항
PR이 다음 요구 사항을 충족하는지 확인하세요. :
현재 작업은 어떤 이슈를 해결한 것인지 설명해주세요.
기존 코드에서 변경된 점이 있다면 설명해주세요. (추가 X)
이 부분을 위주로 봐주세요!
이 PR은 앞선 PR들 머지되고 충돌 해결 후 머지할 예정입니다. 코드 리뷰 때문에 미리 올려요~!
배틀 패키지에서 패키지 분리가 된 클래스들 위주로 리뷰해주시면 됩니다 😄 + 테스트코드
고민사항
도메인
패키지가 없어서BattleMultiplier
를 그냥 서비스 패키지에 넣었는데 고민됩니다..도메인 패키지를 만드는게 좋을까요?
TypeMultiplierProvider
이름 괜찮나요??TypeBattingMultiplierProvider
가 나을까요?고민하다가 둘 다 그저 그래서 의견을 받고 싶습니다 😅