mtgonline/backend/scripts/code_review.md

# MTG Card Interaction Pipeline - Code Review

## Overview

The interaction pipeline consists of four main modules:
1. `card_profile_extractor.py` - Extracts structured profiles from MTGJSON data
2. `interaction_determinator.py` - Determines interactions between card pairs
3. `interaction_recommender.py` - Generates recommendations based on interactions
4. `interaction_pipeline.py` - Orchestrates the full pipeline

## Issues Found

### 1. Import Inconsistency (Critical)
**File**: `interaction_pipeline.py`
**Issue**: Complex import for `sessionmaker`
```python
self.SessionLocal = __import__('sqlalchemy.orm', fromlist=['sessionmaker']).sessionmaker(bind=self.engine)
```
**Fix**: Use direct import at module level:
```python
from sqlalchemy.orm import sessionmaker
# ...
self.SessionLocal = sessionmaker(bind=self.engine)
```

### 2. Type Hints Inconsistency (Medium)
**File**: `interaction_pipeline.py`
**Issue**: Inconsistent type hints
```python
def extract_profiles(self, cards: List[Dict]) -> List:  # Missing type parameter
def determine_interactions(self, profiles) -> dict:  # Missing parameter type
```
**Fix**: Add proper type hints:
```python
def extract_profiles(self, cards: List[Dict]) -> List[CardProfile]:
def determine_interactions(self, profiles: List[CardProfile]) -> dict:
```

### 3. Complex Conditional Logic (High)
**File**: `interaction_pipeline.py`
**Issue**: Nested ternary operators for synergy_type and counter_type determination
```python
"synergy_type": "archetype" if interaction.metadata and interaction.metadata.get('common_archetypes') else
               "mechanic" if interaction.metadata and interaction.metadata.get('mechanics') else
               "mana" if interaction.metadata and interaction.metadata.get('colors') else
               "combo" if interaction.metadata and interaction.metadata.get('card_a_targets') else
               "support",
```
**Fix**: Extract to helper methods or use lookup dictionaries

### 4. Missing Validation (Medium)
**File**: `interaction_pipeline.py`
**Issue**: No validation for empty card lists or invalid data
**Fix**: Add validation at the start of methods

### 5. Evolution Type Mapping (High)
**File**: `interaction_pipeline.py`
**Issue**: Using `interaction.interaction_type` which is 'evolution' for all evolutions
**Fix**: Map to specific evolution types based on metadata

## Recommended Fixes

### Fix 1: Import Structure
```python
# At top of file
from sqlalchemy import create_engine, text
from sqlalchemy.orm import sessionmaker

# In __init__
self.SessionLocal = sessionmaker(bind=self.engine)
```

### Fix 2: Type Hints
```python
def extract_profiles(self, cards: List[Dict]) -> List[CardProfile]:
    return self.profile_extractor.extract_profiles_batch(cards)

def determine_interactions(self, profiles: List[CardProfile]) -> dict:
    return self.determinator.determine_all_interactions(profiles)
```

### Fix 3: Helper Methods for Type Determination
```python
def _determine_synergy_type(self, interaction) -> str:
    """Determine synergy type from interaction metadata."""
    metadata = interaction.metadata or {}
    
    if 'common_archetypes' in metadata:
        return 'archetype'
    elif 'mechanics' in metadata:
        return 'mechanic'
    elif 'colors' in metadata:
        return 'mana'
    elif 'card_a_targets' in metadata:
        return 'combo'
    else:
        return 'support'

def _determine_counter_type(self, interaction) -> str:
    """Determine counter type from interaction metadata."""
    metadata = interaction.metadata or {}
    
    if 'colors_a' in metadata:
        return 'color'
    elif 'power_a' in metadata:
        return 'stats'
    else:
        return 'keyword'

def _determine_evolution_type(self, interaction) -> str:
    """Determine evolution type from interaction metadata."""
    metadata = interaction.metadata or {}
    
    if 'card_name' in metadata:
        return 'reprint'
    else:
        return 'evolution'
```

### Fix 4: Add Validation
```python
def run_initial_load(self, set_code: Optional[str] = None):
    """Run initial load with validation."""
    if not set_code:
        logger.info("No set code provided, loading all cards")
    
    all_cards = self.load_cards_from_db(set_code)
    
    if not all_cards:
        logger.warning("No cards found in database")
        return
    
    # Continue with processing...
```

## Accuracy Review

### Profile Extraction
✅ **Correct**: Color extraction from mana cost
✅ **Correct**: Mechanic extraction using regex patterns
✅ **Correct**: Archetype extraction from subtypes
✅ **Correct**: Target extraction from oracle text
✅ **Correct**: Trigger and effect extraction
✅ **Correct**: Theme extraction based on characteristics

### Interaction Determination
✅ **Correct**: Archetype synergy detection
✅ **Correct**: Mana synergy detection
✅ **Correct**: Mechanic synergy detection (haste+trample, lifelink+combat)
✅ **Correct**: Combo synergy detection (targets + triggers)
✅ **Correct**: Support synergy detection
✅ **Correct**: Counter detection (color, stats, keywords)
✅ **Correct**: Evolution detection (reprints)

### Database Schema
✅ **Correct**: Synergies table with proper constraints
✅ **Correct**: Counters table with proper constraints
✅ **Correct**: Evolutions table with proper constraints
✅ **Correct**: Statistics table with proper aggregations
✅ **Correct**: Foreign key relationships
✅ **Correct**: Unique constraints to prevent duplicates

## Consistency Issues

### 1. File Organization
- All files in `/home/wall-o/projects/mtgonline/backend/scripts/`
- No clear separation between core logic and pipeline
- **Recommendation**: Keep as is for simplicity

### 2. Naming Conventions
- **Good**: Consistent use of snake_case for methods
- **Good**: Consistent use of CamelCase for classes
- **Issue**: Mixed use of `set_code` parameter naming
- **Recommendation**: Standardize on `set_code`

### 3. Error Handling
- **Good**: Try/finally blocks for database connections
- **Issue**: No specific exception handling for database errors
- **Recommendation**: Add specific exception types

### 4. Logging
- **Good**: Consistent logging format
- **Good**: Appropriate log levels (INFO, WARNING, ERROR)
- **Issue**: Missing DEBUG logging for development
- **Recommendation**: Add DEBUG level logging

## Summary

### Critical Issues (Must Fix)
1. ❌ Import structure for sessionmaker
2. ❌ Complex conditional logic for type determination

### High Priority Issues (Should Fix)
3. ❌ Missing type hints
4. ❌ Evolution type mapping
5. ❌ Missing validation

### Medium Priority Issues (Nice to Have)
6. ⚠️ Add specific exception handling
7. ⚠️ Add DEBUG logging
8. ⚠️ Standardize parameter naming

### Low Priority Issues (Can Defer)
9. ✅ All core logic is accurate and correct
10. ✅ Database schema is well-designed
11. ✅ Interaction determination logic is sound

## Next Steps

1. **Fix Critical Issues**: Update import structure and simplify conditional logic
2. **Fix High Priority**: Add proper type hints and validation
3. **Test**: Run pipeline with sample data to verify functionality
4. **Document**: Add docstrings and inline comments for complex logic