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

self.SessionLocal = __import__('sqlalchemy.orm', fromlist=['sessionmaker']).sessionmaker(bind=self.engine)

Fix: Use direct import at module level:

from sqlalchemy.orm import sessionmaker
# ...
self.SessionLocal = sessionmaker(bind=self.engine)

2. Type Hints Inconsistency (Medium)

File: interaction_pipeline.py Issue: Inconsistent type hints

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:

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

"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

# 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

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

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

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