✨ [feat] 무값 계산기 기능 추가 (#29) #142
Conversation
- TurnipPrice 모델 및 TurnipPricePredictor 패턴 예측 알고리즘 구현 - CoreData TurnipPriceEntity 추가 - TurnipPriceStorage 구현 (CoreData 저장/조회) - TurnipCalculatorViewController/Reactor 구현 - 무 가격 패턴 분석 (변동형/3기형/감소형/4기형) 기능 - Dashboard 메뉴에서 무값 계산기 접근 가능 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughAdds a Turnip Calculator feature: Core Data model/entity and storage, domain models and predictor, ReactorKit reactor, UI (view controller, cells, prediction/summary views), dashboard route, and CoreData error extension. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VC as TurnipCalculatorVC
participant Reactor as TurnipCalculatorReactor
participant Storage as TurnipPriceStorage
participant Predictor as TurnipPricePredictor
participant CoreData as Core Data
Note over User,VC: Open calculator
User->>VC: open
VC->>Reactor: viewDidLoad
Reactor->>Storage: fetchCurrentWeekPrice()
Storage->>CoreData: query TurnipPriceEntity
CoreData-->>Storage: entity?
Storage-->>Reactor: TurnipPrice?
Reactor-->>VC: set state (turnipPrice)
Note over User,VC: Enter prices
User->>VC: updateBuyPrice / updatePrice(index)
VC->>Reactor: actions
Reactor->>Predictor: predict(turnipPrice)
Predictor-->>Reactor: [TurnipPrediction]
Reactor-->>VC: update predictions/range
Note over User,VC: Save flow
User->>VC: tap Save
VC->>Reactor: save()
Reactor->>Storage: savePrice(turnipPrice)
Storage->>CoreData: upsert TurnipPriceEntity
CoreData-->>Storage: persisted
Storage-->>Reactor: saved TurnipPrice
Reactor-->>VC: reflect saved state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/TurnipPriceStorage.swift (1)
16-16: Consider returningCompletablefor consistency.The
updatePricemethod is the only operation that doesn't return an observable result. While fire-and-forget updates may be intentional, returningCompletablewould provide callers the ability to:
- Wait for the update to complete before proceeding
- Handle errors consistently with other operations
- Chain reactive operations properly
🔎 Proposed signature change
-func updatePrice(_ turnipPrice: TurnipPrice) +func updatePrice(_ turnipPrice: TurnipPrice) -> CompletableThen update the implementation in
CoreDataTurnipPriceStorageto returnCompletable.create { ... }.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPriceInputCell.swift (1)
86-101: Potential subscription accumulation ifconfigureis called multiple times.If
configureis called multiple times on the same cell beforeprepareForReuse, the RxSwift subscription will accumulate since thedisposeBagis only reset inprepareForReuse. Consider resetting the disposeBag at the start ofconfigureto ensure clean state.🔎 Proposed fix
func configure(dayLabel: String, price: Int?, index: Int, onPriceChanged: @escaping (Int, String?) -> Void) { + disposeBag = DisposeBag() self.dayLabel.text = dayLabel.localized if let price = price {Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (2)
37-71: Unused lazy properties.
buyPriceSection,priceInputSection, andpredictionSectionare declared but never accessed. ThesetUpViews()method usescreateSectionContainerdirectly instead. Consider removing these unused properties to reduce code clutter.
175-193: Unused helper method.
createSectionView(title:)is never called. OnlycreateSectionContainer(title:content:)is used. Consider removing this dead code.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift (1)
80-82:saveaction is defined but never triggered.The
Action.savecase exists and has implementation, but theTurnipCalculatorViewControllernever dispatches this action. If auto-save viaupdatePriceis the intended behavior, consider removing the unusedsaveaction to reduce confusion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.xcdatamodeld/UserCollectionEntity.xcdatamodel/contentsAnimal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swiftAnimal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/EntityMapping/TurnipPriceEntity+Mapping.swiftAnimal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/TurnipPriceStorage.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Coordinator/DashboardCoordinator.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/ViewControllers/DashboardViewController.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/ViewModels/DashboardReactor.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPredictionView.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPriceInputCell.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipSummaryView.swift
🧰 Additional context used
🧬 Code graph analysis (7)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.xcdatamodeld/UserCollectionEntity.xcdatamodel/contents (4)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/VillagersLikeStorage/EntityMapping/VillagersLikeEntity+Mapping.swift (1)
_(11-61)Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/VillagersLikeStorage/CoreDataNPCLikeStorage.swift (2)
coreDataStorage(11-59)update(42-58)Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/VillagersHouseStorage/EntityMapping/VillagersHouseEntity+Mapping.swift (1)
_(11-62)Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/VillagersLikeStorage/EntityMapping/NPCLikeEntity+Mapping.swift (1)
_(11-41)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (4)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPredictionView.swift (2)
setUpViews(60-85)configure(87-112)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPriceInputCell.swift (2)
setUpViews(59-84)configure(86-101)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipSummaryView.swift (4)
setUpViews(47-73)setUpViews(116-130)configure(75-78)configure(132-135)Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (2)
updateBuyPrice(81-89)updatePrice(67-78)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/ViewModels/DashboardReactor.swift (1)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Coordinator/DashboardCoordinator.swift (1)
transition(55-151)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (1)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift (1)
updatePrice(77-97)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Coordinator/DashboardCoordinator.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Animals/Coordinator/AnimalsCoordinator.swift (3)
transition(33-64)animals(10-69)showItemList(72-86)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Coordinator/CatalogCoordinator.swift (2)
transition(34-55)items(10-60)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/TurnipPriceStorage.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift (5)
fetchCurrentWeekPrice(20-40)fetchAllPrices(42-58)savePrice(60-75)updatePrice(77-97)deletePrice(99-120)Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (1)
updatePrice(67-78)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift (3)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (2)
updateBuyPrice(81-89)updatePrice(67-78)Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift (3)
updatePrice(77-97)fetchCurrentWeekPrice(20-40)savePrice(60-75)Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (2)
predict(33-58)getExpectedPriceRange(61-71)
🪛 SwiftLint (0.57.0)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift
[Warning] 68-68: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift
[Warning] 39-39: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 62-62: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 257-257: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 174-174: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 39-39: Variable name 'p' should be between 2 and 40 characters long
(identifier_name)
[Warning] 119-119: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
[Warning] 173-173: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
[Warning] 212-212: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift
[Warning] 117-117: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 185-185: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift
[Warning] 29-29: Conditional statements should always return on the next line
(conditional_returns_on_newline)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check Build
🔇 Additional comments (21)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/ViewControllers/DashboardViewController.swift (1)
113-120: LGTM!The menu integration follows the established pattern from similar components like
PreferencesViewController. The action binding is properly disposed, and the localized menu items are correctly mapped to reactor actions.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/ViewModels/DashboardReactor.swift (1)
16-62: LGTM!The turnip calculator menu integration is consistent with existing menu items. The flow from raw value → localized transformation → coordinator transition is properly implemented.
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.xcdatamodeld/UserCollectionEntity.xcdatamodel/contents (2)
82-89: LGTM!The Core Data entity definition is correctly structured with appropriate attribute types and optionality. The bidirectional relationship with
UserCollectionEntityfollows the same pattern as existing entities (e.g.,DailyTaskEntity,NPCLikeEntity).
99-99: LGTM!The relationship definition follows the established pattern for other
UserCollectionEntityrelationships and maintains proper bidirectional consistency.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/Coordinator/DashboardCoordinator.swift (1)
25-25: LGTM!The turnip calculator route and transition implementation follow the established coordinator pattern for modal presentations, consistent with the
.aboutand.settingroutes.Also applies to: 139-143
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipSummaryView.swift (2)
10-79: LGTM!The view structure is well-organized with clear separation of concerns. The use of stack views and constraint-based layout follows iOS best practices.
137-139: LGTM!The price display logic correctly treats non-positive values as invalid and displays a placeholder, which aligns with the domain where turnip prices should be positive integers.
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPredictionView.swift (1)
87-112: LGTM!The configuration method properly formats the prediction data and provides visual feedback through pattern-specific colors. The probability rounding to whole percentages and animated progress updates enhance the user experience.
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/EntityMapping/TurnipPriceEntity+Mapping.swift (2)
13-20: LGTM!The convenience initializer properly handles optional values using sentinel values (-1 for nil prices, 0 for nil buyPrice) that are correctly reversed in the
toDomain()method.
22-35: LGTM!The
toDomain()mapping correctly reverses the sentinel value encoding and provides sensible defaults. The 12-element prices array corresponds to Monday-Saturday AM/PM slots, which is correct for the turnip price tracking domain.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPriceInputCell.swift (1)
11-57: LGTM!The cell implementation follows good practices: proper reuse identifier, lazy UI components, and correct disposeBag reset in
prepareForReuse.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (1)
335-341: LGTM!The safe subscript extension is a good defensive pattern for array access.
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift (1)
111-146: LGTM!The
loadCurrentWeekPricemethod handles both success and error cases appropriately, with proper loading state management and error propagation.Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (2)
105-111:startOfWeekmay vary by locale.The comment indicates Sunday as the week start, but
Calendar.currentrespects the user's locale settings where the first day of week may be Monday (ISO 8601) or other days. For Animal Crossing's turnip mechanics (bought on Sunday, sold Mon-Sat), consider explicitly using a calendar with Sunday as the first weekday.🔎 Proposed fix for consistent Sunday-based week start
var startOfWeek: Date { - let calendar = Calendar.current + var calendar = Calendar.current + calendar.firstWeekday = 1 // Sunday let components = calendar.dateComponents([.yearForWeekOfYear, .weekOfYear], from: self) return calendar.date(from: components) ?? self }
10-59: LGTM!The
TurnipPatternenum andTurnipPricestruct are well-designed with proper immutability patterns and sensible defaults.Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift (2)
20-40: LGTM!The fetch implementation properly uses background context, handles the optional result, and maps errors appropriately.
84-87: Sentinel values for nil prices are correctly handled.The
toDomain()method inTurnipPriceEntity+Mapping.swiftproperly interprets the sentinel values:-1for nil slot prices (line 25) and0(or negative) for nilbuyPrice(line 31). The bidirectional mapping is consistent—nil values are stored as sentinels during persistence and correctly restored on retrieval.Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (4)
10-25: LGTM!The
TurnipPredictionstruct is well-defined with a sensible staticunknownfallback.
32-58: LGTM!The prediction logic is structured well, computing all four pattern predictions and sorting by probability.
256-261: LGTM!The variance calculation is mathematically correct.
226-243: Range creation inanalyzeSmallSpikecould also havemin > max.Similar to
analyzeLargeSpike, the range calculations here could result inmin > maxfor certainbuyPricevalues and indices. Consider adding the same safeguard.🔎 Proposed fix pattern
let min = Int(Double(buyPrice) * max(0.4, 0.9 - Double(index) * 0.08)) let max = Int(Double(buyPrice) * max(0.5, 0.95 - Double(index) * 0.06)) - return min...max + return Swift.min(min, max)...Swift.max(min, max)Likely an incorrect or invalid review comment.
...iki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift
Show resolved
Hide resolved
| let expectedPrices: [ClosedRange<Int>?] = (0..<12).map { index in | ||
| if index < 4 { | ||
| // 초반 감소 | ||
| let min = Int(Double(buyPrice) * (0.4 - Double(index) * 0.05)) | ||
| let max = Int(Double(buyPrice) * (0.9 - Double(index) * 0.03)) | ||
| return max(min, 40)...max | ||
| } else if index < 7 { | ||
| // 스파이크 구간 | ||
| let min = Int(Double(buyPrice) * 0.9) | ||
| let max = Int(Double(buyPrice) * 6.0) | ||
| return min...max | ||
| } else { | ||
| // 후반 감소 | ||
| let min = Int(Double(buyPrice) * 0.4) | ||
| let max = Int(Double(buyPrice) * 0.9) | ||
| return min...max | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential invalid ClosedRange when min > max.
At high indices (e.g., index=3), the calculation 0.4 - Double(index) * 0.05 = 0.25 produces a min ratio, and 0.9 - Double(index) * 0.03 = 0.81 produces max ratio. After max(min, 40) clamping, if buyPrice is low enough (e.g., 100 bells), the clamped min (40) could exceed max (81), causing a runtime crash when creating min...max.
🔎 Proposed fix
if index < 4 {
// 초반 감소
let min = Int(Double(buyPrice) * (0.4 - Double(index) * 0.05))
let max = Int(Double(buyPrice) * (0.9 - Double(index) * 0.03))
- return max(min, 40)...max
+ let clampedMin = Swift.max(min, 40)
+ let safeMax = Swift.max(max, clampedMin)
+ return clampedMin...safeMax🤖 Prompt for AI Agents
In Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift
around lines 137–154, the computed ClosedRange can be invalid when the clamped
min exceeds the computed max; compute explicit minVal and maxVal integers (e.g.
let minVal = max(Int(Double(buyPrice) * ratioMin), 40) and let maxVal =
Int(Double(buyPrice) * ratioMax)), then ensure maxVal >= minVal (if not, set
maxVal = minVal or swap) before returning minVal...maxVal; apply this pattern to
all branches so you never construct a ClosedRange with min > max.
...p/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift
Show resolved
Hide resolved
...p/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift
Show resolved
Hide resolved
.../Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift
Outdated
Show resolved
Hide resolved
| private func updatePriceAndPredict(_ turnipPrice: TurnipPrice) -> Observable<Mutation> { | ||
| let predictions = TurnipPricePredictor.predict(turnipPrice: turnipPrice) | ||
| let range = TurnipPricePredictor.getExpectedPriceRange(turnipPrice: turnipPrice) | ||
|
|
||
| // 자동 저장 | ||
| storage.updatePrice(turnipPrice) | ||
|
|
||
| return Observable.concat([ | ||
| Observable.just(Mutation.setTurnipPrice(turnipPrice)), | ||
| Observable.just(Mutation.setPredictions(predictions)), | ||
| Observable.just(Mutation.setExpectedRange(min: range?.min ?? 0, max: range?.max ?? 0)) | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Silent failure on storage update.
storage.updatePrice(turnipPrice) returns Void and any errors are only logged via debugPrint. Consider providing user feedback if the save fails, or at minimum logging this more prominently for debugging purposes.
🤖 Prompt for AI Agents
In
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift
around lines 148–160, the call to storage.updatePrice(turnipPrice) silently
swallows failures (returns Void and only debugPrints). Change the storage API to
surface outcome (preferably return Observable<Void> or throw/Result) or wrap it
in a do/catch, then handle the result: if success continue emitting the three
existing Mutations, if failure log with a non-debug logger and emit a new
Mutation representing save failure (or a user-facing error state) so the UI can
show feedback; ensure any Observable returned from storage is merged into the
Observable.concat sequence and errors are caught/mapped to a failure Mutation
instead of being ignored.
- TurnipPricePredictor 예측 알고리즘 개선 - CoreDataTurnipPriceStorage 저장 로직 개선 - TurnipCalculatorReactor 리팩토링 - TurnipPredictionView UI 개선 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Observable 체인을 별도 변수로 분리 - 타입 추론 힌트 추가 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (1)
137-154: Fix invalidClosedRangewhen clamped min exceeds max.At high indices (e.g., index=3) with low buy prices (e.g., 40 bells), the calculation produces
min = 10andmax = 32. After clamping min to 40, you get40...32, which is an invalid range and will crash at runtime.🔎 Proposed fix
let expectedPrices: [ClosedRange<Int>?] = (0..<12).map { index in if index < 4 { // 초반 감소 let min = Int(Double(buyPrice) * (0.4 - Double(index) * 0.05)) let max = Int(Double(buyPrice) * (0.9 - Double(index) * 0.03)) - return max(min, 40)...max + let clampedMin = max(min, 40) + let safeMax = max(max, clampedMin) + return clampedMin...safeMax } else if index < 7 {Based on past review comments.
🧹 Nitpick comments (4)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (1)
59-70: Consider surfacing save errors.The
saveContext()method catches errors but only logs them viadebugPrint. With the newsaveErrorcase added toCoreDataStorageError, consider refactoring this method to throw or return errors so callers (likeCoreDataTurnipPriceStorage) can detect and handle save failures appropriately.🔎 Proposed refactor
-extension NSManagedObjectContext { - func saveContext() { - if self.hasChanges { - do { - try save() - } catch { - let nsError = error as NSError - debugPrint("Unresolved error \(nsError), \(nsError.userInfo)") - } - } - } -} +extension NSManagedObjectContext { + func saveContext() throws { + if self.hasChanges { + do { + try save() + } catch { + throw CoreDataStorageError.saveError(error) + } + } + } +}Note: This change would require updating all call sites to handle the thrown error.
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (1)
250-279: Reactive binding correctly implemented.The table view now properly binds to
reactor.state.map { $0.turnipPrice.prices }withdistinctUntilChanged(), ensuring cells automatically update when state changes. This correctly addresses the previous review concern about stale cell data.The
PriceItemtypealias uses a 3-member tuple, which exceeds SwiftLint's recommended limit. Consider using a lightweight struct for better semantics:🔎 Optional refactor
- private func setUpTableView(reactor: TurnipCalculatorReactor) { - typealias PriceItem = (dayLabel: String, price: Int?, index: Int) + private struct PriceItem { + let dayLabel: String + let price: Int? + let index: Int + } + + private func setUpTableView(reactor: TurnipCalculatorReactor) { - let pricesObservable: Observable<[PriceItem]> = reactor.state + let pricesObservable: Observable<[PriceItem]> = reactor.state .map { $0.turnipPrice.prices } .distinctUntilChanged() .map { prices -> [PriceItem] in let dayLabels = TurnipPrice.dayLabels return dayLabels.enumerated().map { index, dayLabel in let price: Int? = prices[safe: index] ?? nil - return (dayLabel: dayLabel, price: price, index: index) + return PriceItem(dayLabel: dayLabel, price: price, index: index) } }Based on learnings, SwiftLint prefers tuples with at most 2 members.
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift (1)
164-182: Consider surfacing storage errors to users.The
saveMutationobservable silently swallows all storage errors by returning.empty()on both success and failure (lines 176-179). While the fire-and-forget approach provides responsive UI, repeated save failures could lead to data loss without user awareness. Consider adding error tracking or occasional user notification for persistent failures.🔎 Recommended enhancement
private func updatePriceAndPredict(_ turnipPrice: TurnipPrice) -> Observable<Mutation> { let predictions = TurnipPricePredictor.predict(turnipPrice: turnipPrice) let range = TurnipPricePredictor.getExpectedPriceRange(turnipPrice: turnipPrice) let uiMutations = Observable.concat([ Observable.just(Mutation.setTurnipPrice(turnipPrice)), Observable.just(Mutation.setPredictions(predictions)), Observable.just(Mutation.setExpectedRange(min: range?.min ?? 0, max: range?.max ?? 0)) ]) - let saveMutation = storage.updatePrice(turnipPrice) - .asObservable() - .flatMap { _ -> Observable<Mutation> in .empty() } - .catch { _ -> Observable<Mutation> in .empty() } + let saveMutation = storage.updatePrice(turnipPrice) + .asObservable() + .flatMap { _ -> Observable<Mutation> in .empty() } + .catch { error -> Observable<Mutation> in + debugPrint("⚠️ Failed to save turnip price: \(error)") + // Optionally: track consecutive failures and emit setError mutation if > threshold + return .empty() + } return Observable.merge(uiMutations, saveMutation) }Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (1)
172-178: Consider usingwhereclause for cleaner iteration.The
forloop with a singleifcondition can be simplified using awhereclause.🔎 Proposed refactor
var isDecreasing = true - for i in 1..<validRatios.count { - if validRatios[i] > validRatios[i-1] + 0.02 { - isDecreasing = false - break - } + for i in 1..<validRatios.count where validRatios[i] > validRatios[i-1] + 0.02 { + isDecreasing = false + break }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swiftAnimal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swiftAnimal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/TurnipPriceStorage.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPredictionView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPredictionView.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/DailyTaskStorage/CoreDataDailyTaskStorage.swift (2)
updateTask(59-79)insertTask(41-57)Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/UserInfoStorage/CoreDataUserInfoStorage.swift (1)
updateUserInfo(30-44)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (4)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPredictionView.swift (2)
setUpViews(60-85)configure(98-123)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipPriceInputCell.swift (2)
setUpViews(59-84)configure(86-101)Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/Views/TurnipSummaryView.swift (4)
setUpViews(47-73)setUpViews(116-130)configure(75-78)configure(132-135)Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (2)
updateBuyPrice(81-89)updatePrice(67-78)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/TurnipPriceStorage.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift (5)
fetchCurrentWeekPrice(20-40)fetchAllPrices(42-58)savePrice(60-62)updatePrice(64-67)deletePrice(107-128)Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (1)
updatePrice(67-78)
🪛 SwiftLint (0.57.0)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift
[Warning] 133-133: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 207-207: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift
[Warning] 29-29: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift
[Warning] 251-251: Tuples should have at most 2 members
(large_tuple)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift
[Warning] 68-68: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift
[Warning] 39-39: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 62-62: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 257-257: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 174-174: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 39-39: Variable name 'p' should be between 2 and 40 characters long
(identifier_name)
[Warning] 119-119: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
[Warning] 173-173: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
[Warning] 212-212: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
🔇 Additional comments (24)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (1)
13-13: LGTM! New error case properly defined.The
saveErrorcase is correctly added with an appropriate localized error description, following the same pattern as existing error cases.Also applies to: 21-22
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/TurnipPriceStorage.swift (1)
11-19: LGTM! Clean protocol design.The protocol provides a well-defined contract for turnip price storage operations using RxSwift
Singletypes for asynchronous operations. The optional return type forfetchCurrentWeekPrice()appropriately handles the case when no data exists for the current week, and the@discardableResultonupdatePriceenables fire-and-forget updates when needed.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (3)
281-288: LGTM! Removed conflictingreloadData()call.The method now only updates
buyPriceTextFieldand correctly relies on the reactive binding to update the table view automatically. The inline comment at line 287 appropriately documents whyreloadData()is unnecessary.
290-310: LGTM! Prediction display logic is sound.The method correctly rebuilds the prediction stack, handles the empty/unknown state with appropriate placeholder text, and limits display to the top 3 predictions for a clean UI.
315-319: Remove the private Array extension; use the existing Collection extension instead.The codebase already provides a safe subscript operator via
Collection+extension.swift, which is automatically available toArraysince it conforms to theCollectionprotocol. The privateArrayextension at these lines is redundant and should be removed.Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swift (4)
72-94: LGTM! Clear action now persists properly.The
clearAllaction now correctly persists the cleared state to storage (line 78) and includes graceful error handling. The optimistic UI update in the catch block (lines 87-94) ensures a responsive user experience even if storage fails.
127-162: LGTM! Loading logic properly handles all states.The method correctly manages loading state, handles missing data by creating a new
TurnipPricewith the current week start, and only calculates predictions whenbuyPriceis available. Error handling appropriately emitssetErrormutation.
184-204: LGTM! Explicit save with proper error handling.Unlike the fire-and-forget save in
updatePriceAndPredict, this method properly surfaces storage errors via thesetErrormutation (line 199), providing user feedback when manual saves fail.
206-209: LGTM! Price parsing logic is straightforward.The helper correctly converts string input to optional integer, returning
nilfor empty or invalid input. The guard statement style is readable despite the SwiftLint warning.Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swift (5)
10-36: LGTM! Well-defined pattern enum.The
TurnipPatternenum correctly models the turnip price patterns from Animal Crossing with appropriate localized names and user-friendly descriptions for each pattern type.
38-59: LGTM! Robust domain model with validation.The
TurnipPricestruct is well-designed with appropriate defaults and validation. The initializer ensures thepricesarray always contains exactly 12 elements (line 56), preventing potential index-out-of-bounds issues.
61-103: LGTM! Helper methods are well-implemented.The helper methods correctly implement immutable updates, include proper bounds checking (lines 68, 98), and guard against edge cases like division by zero in
priceRatio. This addresses the previous review concern about missing bounds validation.
116-119: LGTM! Week comparison logic is straightforward.The
isSameWeekmethod correctly leverages thestartOfWeekproperty to compare normalized week start dates.
105-114: No action required. ThestartOfWeekimplementation correctly handles year boundaries usingyearForWeekOfYearandweekOfYearcomponents, which is the proper Calendar API usage for this use case.Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (6)
33-58: LGTM!The prediction entry point correctly validates the buy price, computes ratios, and aggregates pattern analyses. The sorting by probability ensures the most likely pattern appears first.
61-71: LGTM!The method provides a sensible default range when no pattern can be determined, and correctly delegates to the top prediction otherwise.
76-108: LGTM!The fluctuating pattern analysis correctly uses variance to detect genuine fluctuation and adjusts probability based on how well the observed ratios fit the expected range.
166-202: LGTM!The decreasing pattern analysis correctly detects monotonic decline with reasonable tolerance (+0.02) and safely constructs price ranges with proper clamping.
205-252: LGTM!The small spike pattern analysis correctly identifies moderate price increases (1.0-2.0x) and generates valid price ranges across all three phases.
256-261: LGTM!The variance calculation is mathematically correct and properly guards against empty input.
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift (4)
20-40: LGTM!The method correctly uses background task execution for thread safety and appropriately finds the current week's price by comparing date components.
42-58: LGTM!The fetch-all operation correctly retrieves and sorts prices by week in descending order, which is suitable for displaying recent data first.
60-98: LGTM!The upsert logic correctly prevents duplicate entries by checking for existing entities by ID before creating new ones. The past concern about duplicate creation has been properly addressed through the conditional logic at line 82.
101-105: LGTM!The entity update logic correctly handles optional values, using -1 as a sentinel for nil prices, which aligns with the mapping layer conventions.
| func deletePrice(_ turnipPrice: TurnipPrice) -> Single<TurnipPrice> { | ||
| return Single.create { single in | ||
| self.coreDataStorage.performBackgroundTask { context in | ||
| do { | ||
| let object = try self.coreDataStorage.getUserCollection(context) | ||
| let priceEntities = object.turnipPrices?.allObjects as? [TurnipPriceEntity] ?? [] | ||
|
|
||
| guard let entityToDelete = priceEntities.first(where: { $0.id == turnipPrice.id }) else { | ||
| single(.failure(CoreDataStorageError.notFound)) | ||
| return | ||
| } | ||
|
|
||
| object.removeFromTurnipPrices(entityToDelete) | ||
| context.saveContext() | ||
| single(.success(entityToDelete.toDomain())) | ||
| } catch { | ||
| single(.failure(CoreDataStorageError.readError(error))) | ||
| } | ||
| } | ||
| return Disposables.create() | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider more specific error mapping for delete operations.
Line 123 maps caught errors to CoreDataStorageError.readError, but this method performs a delete operation. While the error could originate from the initial fetch, mapping to a more specific error type (like deleteError if available) would provide clearer error semantics.
🤖 Prompt for AI Agents
In
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/TurnipPriceStorage/CoreDataTurnipPriceStorage.swift
around lines 107 to 128, the catch block currently maps all errors to
CoreDataStorageError.readError even though this function performs a delete;
change the mapping to a more specific delete error (e.g.
CoreDataStorageError.deleteError) or create that case if it doesn't exist, and
pass the caught error into that deleteError case so delete failures are reported
with correct semantics; keep the existing notFound handling as-is and ensure the
rest of the flow (removal, context.saveContext, and returning the deleted domain
object) is unchanged.
- named tuple 사용으로 타입 추론 단순화 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (2)
248-277: Reactive table binding correctly addresses previous review feedback.The implementation now properly binds the table view to reactive state changes, which resolves the prior concern about cells not reflecting updates. The use of a typealias for
PriceItemimproves readability.Minor: Redundant nil coalescing on line 258.
The expression
prices[safe: index] ?? nilis redundant sinceprices[safe: index]already returnsInt?. The nil coalescing operator adds no value here.🔎 Suggested simplification
let dayLabels = TurnipPrice.dayLabels return dayLabels.enumerated().map { index, dayLabel in - let price: Int? = prices[safe: index] ?? nil + let price: Int? = prices[safe: index] return (dayLabel: dayLabel, price: price, index: index) }
313-317: LGTM!The safe array subscript implementation is correct and follows a common pattern for defensive array access. The
privatescope is appropriate for file-local usage.If this pattern is used in multiple files, consider extracting it to a shared utilities file to promote reuse and maintain consistency across the codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check Build
🔇 Additional comments (7)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (7)
1-17: LGTM!The imports and class structure follow standard ReactorKit patterns correctly. The
finalclass modifier and proper protocol conformance are appropriate.
19-73: LGTM!UI component declarations are well-structured with appropriate lazy initialization. The use of
.numberPadkeyboard type for price inputs andPublishRelayfor handling cell input events demonstrates good reactive programming practices.
77-181: LGTM!The view setup and layout logic is comprehensive and well-organized. The fixed table height calculation on line 137 (
CGFloat(12 * 52)) is appropriate given the fixed 12-slot weekly price structure. ThecreateSectionContainerhelper promotes good code reuse.
185-246: LGTM!The ReactorKit binding logic is well-implemented with proper patterns:
- Correct use of
distinctUntilChanged()to prevent unnecessary updates- Proper memory management with
[weak self]- Main thread observation for UI updates
- Clean separation of input actions and output state bindings
279-286: LGTM!The
updatePriceInputsmethod correctly updates only the buy price text field while relying on reactive table view bindings for price updates, which addresses the previous review concern about conflicting data source management.
288-308: LGTM!The prediction view updates are handled cleanly with proper empty state handling. The use of
prefix(3)to limit displayed predictions is a sensible UX choice.
248-277: No memory leak issue present.TurnipPriceInputCellalready correctly resets itsdisposeBaginprepareForReuse(), which disposes of all previous subscriptions before the cell is reused.
| buyPriceTextField.rx.controlEvent(.editingDidEnd) | ||
| .withLatestFrom(buyPriceTextField.rx.text) | ||
| .map { Reactor.Action.updateBuyPrice($0) } | ||
| .bind(to: reactor.action) | ||
| .disposed(by: disposeBag) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*TurnipCalculatorReactor*" -type fRepository: leeari95/ACNH-wiki
Length of output: 176
🏁 Script executed:
rg -l "TurnipCalculatorReactor" --type swift | head -5Repository: leeari95/ACNH-wiki
Length of output: 403
🏁 Script executed:
rg "case \.updateBuyPrice" --type swift -A 5 -B 2Repository: leeari95/ACNH-wiki
Length of output: 1294
🏁 Script executed:
rg "class TurnipCalculatorReactor" --type swift -A 50Repository: leeari95/ACNH-wiki
Length of output: 7298
🏁 Script executed:
cat Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewModels/TurnipCalculatorReactor.swiftRepository: leeari95/ACNH-wiki
Length of output: 7615
🏁 Script executed:
rg "parsePrice" --type swift -A 10Repository: leeari95/ACNH-wiki
Length of output: 3319
🏁 Script executed:
rg "TurnipPrice" --type swift -l | head -10Repository: leeari95/ACNH-wiki
Length of output: 882
🏁 Script executed:
cat Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPrice.swiftRepository: leeari95/ACNH-wiki
Length of output: 3712
Add input validation for price values in TurnipCalculatorReactor and TurnipPrice.
The parsePrice method only validates that the input string is non-empty, then directly converts it to Int. This allows negative values and arbitrarily large numbers to be accepted. Similarly, both updateBuyPrice and updatePrice in the TurnipPrice model bypass validation entirely.
Add validation to reject:
- Negative values
- Unrealistic bounds (e.g., prices exceeding expected Animal Crossing turnip price ranges)
Consider adding validation in parsePrice or as guard statements in the action handlers to ensure only sensible prices reach the state.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- analyzeLargeSpike, analyzeDecreasing, analyzeSmallSpike 메서드의 map 클로저를 별도 메서드로 분리 - Swift 표준 라이브러리의 min/max 함수와 변수명 충돌 방지를 위해 Swift.min/Swift.max 사용 - 컴파일러 type-check 타임아웃 문제 해결 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (1)
154-157: Critical: InvalidClosedRangewhenmaxValue < adjustedMin.This issue was previously flagged but remains unresolved. When
buyPriceis low (less than ~50), the calculationInt(Double(buyPrice) * (0.9 - Double(index) * 0.03))at higher indices (e.g., index=3) can produce amaxValuethat is less than the clampedadjustedMin(40), causing a runtime crash when constructingadjustedMin...maxValue.For example, with
buyPrice = 45andindex = 3:
maxValue = Int(45 * 0.81) = 36adjustedMin = max(Int(45 * 0.25), 40) = 40- Range
40...36is invalid and will crash.🔎 Proposed fix
if index < 4 { // 초반 감소 let minValue = Int(Double(buyPrice) * (0.4 - Double(index) * 0.05)) let maxValue = Int(Double(buyPrice) * (0.9 - Double(index) * 0.03)) let adjustedMin = Swift.max(minValue, 40) - range = adjustedMin...maxValue + let adjustedMax = Swift.max(maxValue, adjustedMin) + range = adjustedMin...adjustedMax
🧹 Nitpick comments (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (2)
137-137: Consider extracting magic numbers as constants.The hard-coded calculation
CGFloat(12 * 52)embeds the row count and row height. Extracting these as named constants would improve maintainability if these values need to change.🔎 Proposed refactor
At the top of the class, add constants:
private enum Constants { static let priceInputRowCount = 12 static let priceInputRowHeight: CGFloat = 52 }Then update line 137:
- priceTableView.heightAnchor.constraint(equalToConstant: CGFloat(12 * 52)) + priceTableView.heightAnchor.constraint(equalToConstant: CGFloat(Constants.priceInputRowCount) * Constants.priceInputRowHeight)And line 54:
- tableView.rowHeight = 52 + tableView.rowHeight = Constants.priceInputRowHeight
308-308: Consider documenting the prediction limit.The
prefix(3)limit restricts the displayed predictions to three. Adding a brief comment explaining the rationale (e.g., UI space constraints, user experience) would improve code clarity.🔎 Proposed documentation
- // Add prediction views (max 3) - for prediction in predictions.prefix(3) { + // Add prediction views (limit to top 3 for readability) + let maxPredictions = 3 + for prediction in predictions.prefix(maxPredictions) { let predictionView = TurnipPredictionView() predictionView.configure(with: prediction) predictionStackView.addArrangedSubview(predictionView) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swiftAnimal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift
[Warning] 39-39: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 62-62: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 280-280: Conditional statements should always return on the next line
(conditional_returns_on_newline)
[Warning] 183-183: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 39-39: Variable name 'p' should be between 2 and 40 characters long
(identifier_name)
[Warning] 119-119: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
[Warning] 182-182: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
[Warning] 227-227: Variable name 'i' should be between 2 and 40 characters long
(identifier_name)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check Build
🔇 Additional comments (11)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift (1)
253-282: Table view binding properly implements reactive state updates.The reactive binding correctly observes
reactor.state.turnipPrice.pricesand rebuilds the data source when prices change. This properly addresses the previous concern about cells not reflecting state updates.Animal-Crossing-Wiki/Projects/App/Sources/Models/TurnipPricePredictor.swift (10)
11-25: LGTM!The
TurnipPredictionstruct is well-designed with appropriate properties and a useful staticunknownmember for fallback cases.
33-58: LGTM!The main prediction logic is well-structured with proper input validation, clear ratio calculations, and appropriate sorting by probability.
61-71: LGTM!The expected price range calculation appropriately reuses the
predict()method and provides a sensible fallback range when predictions are unavailable.
76-108: LGTM!The fluctuating pattern analysis uses appropriate heuristics with variance-based probability adjustments. The range construction is safe as
minRatio < maxRatiois guaranteed.
111-146: LGTM!The large spike pattern detection logic appropriately identifies spikes above 1.5x ratio and adjusts probabilities based on early decreasing patterns.
175-205: LGTM!The decreasing pattern analysis correctly identifies continuous decline with appropriate tolerance for minor fluctuations.
207-217: LGTM!The decreasing pattern price generator is safe. The
max()clamping ensuresmaxRatio(clamped to ≥0.4) is always greater thanminRatio(clamped to ≥0.3), preventing invalid range construction.
220-250: LGTM!The small spike pattern detection appropriately identifies moderate price increases (1.0-2.0x) and distinguishes them from large spikes.
252-275: LGTM!The small spike pattern price generator is safe. The
max()clamping ensures the minimum ratio floor (0.4) is less than the maximum ratio floor (0.5), preventing invalid range construction across all phases.
279-284: LGTM!The variance calculation is mathematically correct with appropriate guard for empty input.
| buyPriceTextField.rx.controlEvent(.editingDidEnd) | ||
| .withLatestFrom(buyPriceTextField.rx.text) | ||
| .map { Reactor.Action.updateBuyPrice($0) } | ||
| .bind(to: reactor.action) | ||
| .disposed(by: disposeBag) | ||
|
|
||
| // Price input changes | ||
| priceInputRelay | ||
| .map { Reactor.Action.updatePrice(index: $0.0, price: $0.1) } | ||
| .bind(to: reactor.action) | ||
| .disposed(by: disposeBag) |
There was a problem hiding this comment.
Input validation for price values remains unimplemented.
This concern was raised in a previous review but has not yet been addressed. The buy price and weekly price inputs are passed directly to the reactor without client-side validation. This allows negative values, zero, and unrealistically large numbers (e.g., "-100" or "999999999") to be accepted and propagated to the predictor and storage layers.
In Animal Crossing, typical turnip price ranges are:
- Buy price (Sunday): ~90–110 bells
- Sell prices (Mon–Sat AM/PM): ~20–660 bells
Consider adding validation guards before dispatching actions:
🔎 Proposed validation logic
private func bindInputs(reactor: TurnipCalculatorReactor) {
// View did load
Observable.just(Reactor.Action.viewDidLoad)
.bind(to: reactor.action)
.disposed(by: disposeBag)
// Buy price changes with validation
buyPriceTextField.rx.controlEvent(.editingDidEnd)
.withLatestFrom(buyPriceTextField.rx.text)
.filter { text in
guard let text = text, !text.isEmpty, let price = Int(text) else { return true }
return price >= 0 && price <= 1000 // Reject unrealistic values
}
.map { Reactor.Action.updateBuyPrice($0) }
.bind(to: reactor.action)
.disposed(by: disposeBag)
// Price input changes with validation
priceInputRelay
.filter { _, priceText in
guard let text = priceText, !text.isEmpty, let price = Int(text) else { return true }
return price >= 0 && price <= 1000 // Reject unrealistic values
}
.map { Reactor.Action.updatePrice(index: $0.0, price: $0.1) }
.bind(to: reactor.action)
.disposed(by: disposeBag)
// Clear button
clearButton.rx.tap
.map { Reactor.Action.clearAll }
.bind(to: reactor.action)
.disposed(by: disposeBag)
}Alternatively, validation could be added in the reactor's parsePrice method or action handlers. Consider showing user feedback (e.g., alert or text color change) when invalid input is detected.
🤖 Prompt for AI Agents
In
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/TurnipCalculator/ViewControllers/TurnipCalculatorViewController.swift
around lines 198–208, the buy price and weekly price inputs are sent straight to
the reactor with no validation; add input validation before mapping to
Reactor.Action.updateBuyPrice and Reactor.Action.updatePrice to reject
non-numeric, empty, negative, zero (if desired) and unrealistically large values
(e.g. >1000), by filtering the observable streams
(buyPriceTextField.rx.controlEvent(.editingDidEnd).withLatestFrom(...) and
priceInputRelay) to only allow valid Ints in the expected range, and when
filtering out invalid input emit a user-facing indication (alert, inline error
state, or text color) instead of dispatching the action; alternatively perform
the same validation in the reactor parsePrice/action handler if you prefer
centralized validation.
Summary
Changes
TurnipPrice모델 및TurnipPricePredictor패턴 예측 알고리즘TurnipPriceEntity및TurnipPriceStorage구현TurnipCalculatorViewController/TurnipCalculatorReactor구현TurnipPriceInputCell,TurnipPredictionView,TurnipSummaryViewTest plan
Closes #29
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.