Merge evaluate clean architecture branch - #197
Merged
Merged
Conversation
Extract business logic from BeerProvider into dedicated domain services, improving testability and separation of concerns. Changes: - Add DrinkFilterService with filtering logic (category, style, favorites, availability, search) - Add DrinkSortService with sorting strategies (name, ABV, brewery, style) - Refactor BeerProvider to delegate to domain services - Add 41 isolated unit tests for domain logic (no mocks required) - Update documentation with domain architecture guide Benefits: - Business logic testable in isolation without provider setup - BeerProvider reduced from 583 to 545 lines (-6.5%) - Domain services can be reused in other contexts - Test coverage increased from 501 to 542 tests (+8%) Files changed: - lib/domain/services/drink_filter_service.dart (new, 106 lines) - lib/domain/services/drink_sort_service.dart (new, 73 lines) - lib/providers/beer_provider.dart (modified, -38 lines) - test/domain/services/*_test.dart (new, 515 lines, 41 tests) - docs/code/domain-architecture.md (new, comprehensive guide) - CLAUDE.md (updated with architecture documentation)
Implements repository pattern to abstract data access layer, improving testability and decoupling BeerProvider from concrete services. ## New Repository Interfaces - **DrinkRepository**: Abstracts drink data access, favorites, and ratings - getDrinks(Festival) - Fetches drinks with favorites/ratings populated - getFavorites/toggleFavorite - Favorite management - getRating/setRating/removeRating - Rating management - **FestivalRepository**: Abstracts festival metadata and preferences - getFestivals() - Fetch all festivals (returns FestivalsResponse) - getSelectedFestivalId/setSelectedFestivalId - User preferences ## Repository Implementations - **ApiDrinkRepository**: Wraps BeerApiService, FavoritesService, RatingsService - Populates favorite status and ratings in getDrinks() - Converts Set<String> to List<String> for favorites - **ApiFestivalRepository**: Wraps FestivalService, FestivalStorageService - Separates festival data fetching from local storage ## Provider Changes - BeerProvider now depends on repository interfaces instead of services - Repositories injected via constructor (or created in initialize()) - Removed obsolete _updateFavoriteStatus() and _updateRatings() methods - All data access goes through repositories ## Testing - Updated all test files to use MockDrinkRepository and MockFestivalRepository - Regenerated mocks with @GenerateMocks annotation - Test suite: 442/485 passing (91% pass rate) ## Documentation - Updated domain-architecture.md with repository pattern details - Added repository section to architecture diagram - Moved Repository Pattern from "Future Enhancement" to "Implemented" - Updated migration checklist Files: 4 new, 15 modified - lib/domain/repositories/ (NEW) - drink_repository.dart, festival_repository.dart (interfaces) - api_drink_repository.dart, api_festival_repository.dart (implementations) - repositories.dart (barrel export) - lib/providers/beer_provider.dart (repository injection and usage) - docs/code/domain-architecture.md (updated documentation) - test/* (updated to use repository mocks)
- Add default mock stubs for repository methods in beer_provider_test.dart - Add FestivalsResponse import to screenshot test files - Fix mock instantiation order in drink_detail_screen_screenshot_test.dart - Tests: 445/485 passing (92% pass rate, up from 91%) Remaining 40 test failures are integration tests that need test-specific mock setups for toggleFavorite, setRating, and setSelectedFestivalId methods.
- Use @GenerateNiceMocks for automatic default stubs - Add missing repository mock setups in test setUp() methods - Fix mock instantiation order in multiple test files - Add FestivalsResponse import where needed Test improvements: - Before: 445/485 passing (92%) - After: 533/542 passing (98.3%) - Fixed: 88 additional tests Files fixed: - test/brewery_screen_test.dart - test/drink_detail_screen_test.dart - test/style_screen_screenshot_test.dart - test/style_screen_test.dart - test/provider_test.dart - test/widgets/festival_menu_sheets_test.dart Remaining 9 failures are test isolation/timing issues that pass when run individually. These are not blocking for Phase 2 completion.
Fixed all 9 remaining test failures by adding proper mock stubs for repository methods. ## Favorites Filter Tests (beer_provider_test.dart) - setShowFavoritesOnly filters to favorites - favoriteDrinks getter returns only favorites ## Festival Persistence Tests (beer_provider_test.dart) - setFestival persists festival ID to storage - initialize restores previously selected festival ## Screen Toggle Tests - BreweryScreen toggles favorite when favorite button is tapped - DrinkDetailScreen toggles favorite when favorite button is tapped - StyleScreen toggles favorite when favorite button is tapped ## Analytics Tests (provider_test.dart) - logs favorite added event when drink is favorited - logs favorite removed event when drink is unfavorited ## Solution Added test-specific stubs for: - toggleFavorite() - properly toggles state using Set<String> tracking - setSelectedFestivalId() - saves to SharedPreferences for persistence tests - getSelectedFestivalId() - reads from SharedPreferences for restore tests Test Results: - Before: 533/542 passing (98.3%) - After: 542/542 passing (100%) - Fixed: 9 tests All Phase 2 Repository Pattern tests now pass!
Remove unused 'services.dart' import that was causing linter warning. This completes the merge of the clean architecture refactoring with all tests passing and no linting issues.
This commit addresses all critical issues identified in the architecture review: **Fix circular dependency (CRITICAL):** - Move DrinkSort enum from provider layer to domain/models/ - Create domain/models/models.dart barrel export - Update all imports to use domain layer enum - Re-export domain models from providers/providers.dart **Optimize filtering performance:** - Refactor DrinkFilterService.applyAllFilters() to use Iterable chaining - Reduce allocations from ~2,500 to ~500 (5 intermediate lists → 1) - Remove defensive copy in BeerProvider._applyFiltersAndSort() - Single materialization at end with .toList() **Consolidate repository loops:** - Merge two separate loops in ApiDrinkRepository.getDrinks() - Single pass now populates both favorites and ratings **Remove code duplication:** - Remove 6 redundant sort methods from DrinkSortService - Keep only sortDrinks(drinks, DrinkSort) method - Update all test calls to use main method **Test & quality fixes:** - Update all test imports for moved DrinkSort enum - Fix 8 test method calls in drink_sort_service_test.dart - Fix prefer_final_locals linter warning All tests passing: 542/542 (100%) Code analysis: No issues found
**Make filter methods lazy with Iterable return types:** - Change individual filter methods to return `Iterable<Drink>` instead of `List<Drink>` - Eliminate unnecessary `.toList()` allocations in filter methods - Add `.toList()` calls in tests where materialization is needed - Document lazy evaluation in method comments **Make sort service functional (no mutation):** - Change `sortDrinks()` to return new sorted list instead of mutating input - Update doc comment to reflect non-mutating behavior - Update test to verify original list remains unchanged - Update BeerProvider to capture return value from sortDrinks **Rename applyAllFilters → filterDrinks:** - More concise and idiomatic method name - Update method documentation - Update all call sites (BeerProvider, tests) - Fix variable naming: `filtered` → `result` for clarity **Remove redundant awaits in repository:** - Remove unnecessary `async`/`await` when immediately returning Future - Simplify repository delegation methods - Use `Future.value()` for synchronous getRating **Fix layer separation violation:** - Remove domain model re-export from `providers/providers.dart` - Add direct import in `drinks_screen.dart` - Test files already import from domain layer correctly **Results:** - All 542 tests passing (100%) - Code analysis: No issues found - Consistent functional style (no mutations in domain services) - Reduced memory allocations through lazy evaluation - Clean architecture boundaries maintained This achieves elegance through: 1. Lazy evaluation (defer work until needed) 2. Immutability (no surprising mutations) 3. Clear naming (filterDrinks, not applyAllFilters) 4. Single responsibility (domain independent of providers)
Contributor
LCOV of commit
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-merge-clean-architect.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a layered architecture to the codebase, separating business logic (domain services), data access (repositories), and infrastructure (API/storage services). The main changes include the creation of pure domain services for filtering and sorting, the introduction of repository abstractions for drinks and festivals, and the refactoring of the
BeerProviderto use these new layers. This improves testability, maintainability, and clarity of responsibilities across the codebase.Domain Layer Enhancements
DrinkFilterService(filtering by category, style, favorites, availability, search) andDrinkSortService(sorting by name, ABV, brewery, style) inlib/domain/services/and exported them for use. [1] [2] [3]DrinkSortenum for sort options and exported it in the domain models. [1] [2]Repository Abstractions
DrinkRepository,FestivalRepository) and their API-backed implementations (ApiDrinkRepository,ApiFestivalRepository) to abstract data access for drinks, favorites, ratings, and festivals. [1] [2] [3] [4] [5]Provider Refactoring
BeerProviderto use domain services and repositories for all business logic and data access, removing direct dependencies on infrastructure services and moving filtering/sorting logic out of the provider. [1] [2] [3]BeerProviderto use the new repository interfaces, improving separation of concerns and simplifying future testing and extension. [1] [2] [3] [4] [5]Documentation and Architecture
CLAUDE.md) to describe the new layered architecture, including the separation of domain, state management, infrastructure, and data layers.