Feature Envy and Shotgun Surgery: When Your Classes Are Secretly Dating

A developer adds a discount calculation to an e-commerce checkout service. The change requires editing OrderService, CartService, PricingService, UserService, and PromotionService. Five files. One feature. Three different developers need to review the PR because each file has a different owner.

That's shotgun surgery.

Meanwhile, the PromotionService class spends 80% of its code calling methods on User objects. It grabs user.getLoyaltyTier(), user.getTotalSpend(), user.getJoinDate(), user.getLastPurchaseDate() and does all the interesting work itself. The User class just sits there exposing data like a glorified struct.

That's feature envy.

These two smells travel in packs. When you see one, start looking for the other. They're symptoms of the same underlying disease: a domain model that doesn't match reality.

Why These Smells Multiply

Feature envy and shotgun surgery don't appear overnight. They grow. The pattern goes like this:

Sprint 1: Someone creates a clean User class with basic data. Someone else creates OrderService to handle order logic. Reasonable split.

Sprint 4: A new requirement needs user data during order processing. Instead of putting the logic in User, a developer adds a method to OrderService that calls three User getters. Small shortcut. PR gets approved in 10 minutes.

Sprint 12: OrderService now has 15 methods that operate on User data. PricingService has 8 more. NotificationService has 6. The User class is still mostly getters. To add a new user-related feature, you touch six files. Every time.

The disease is primitive obsession crossed with anemic domain model. Classes become data bags. Services become procedure collections that operate on those bags. Object-oriented design in name only.

Spotting Feature Envy (Without Reading Every Line)

The symptom: a method references another object more than its own class.

// This method is in PromotionService
// Count the references to 'user' vs references to 'this'
calculateDiscount(user: User, order: Order): number {
  const loyaltyTier = user.getLoyaltyTier();          // user reference
  const totalSpend = user.getTotalSpend();            // user reference
  const daysSinceJoin = this.daysBetween(             // this reference
    user.getJoinDate(),                               // user reference
    new Date()
  );
  const lastPurchase = user.getLastPurchaseDate();    // user reference
  
  // All the actual logic uses user data
  if (loyaltyTier === 'GOLD' && totalSpend > 10000) {
    return order.getSubtotal() * 0.15;                // order reference
  }
  if (daysSinceJoin > 365) {
    return order.getSubtotal() * 0.10;                // order reference
  }
  // ... 40 more lines like this
  return 0;
}

Seven references to other objects. One reference to this (and it's just a utility function). This method desperately wants to live somewhere else.

Quick heuristic: grep for this. versus object parameter references. If external references outnumber internal ones by 3:1 or more, investigate.

The Tooling Shortcut

Static analysis catches this mechanically. Tools look for methods where external object coupling exceeds a threshold. IntelliJ's inspections flag it. SonarQube tracks it as a metric. You can configure CodeClimate to warn when a method's "afferent coupling" is too high.

But most teams don't configure these rules, so the smells pile up until someone reads a Martin Fowler book and starts pointing fingers at standups.

Shotgun Surgery: Death by a Thousand Commits

Feature envy is localized. Shotgun surgery is the system-wide consequence.

The symptom: every logical change requires commits to multiple files in different domains.

Actual git log from a real project (names changed):

commit abc123 - "Add birthday discount feature"
  M src/services/PricingService.ts
  M src/services/UserService.ts
  M src/services/OrderService.ts
  M src/models/User.ts
  M src/models/Order.ts
  M src/api/routes/checkout.ts
  M src/utils/dateUtils.ts
  M tests/pricing.test.ts
  M tests/checkout.test.ts

Nine files for one discount rule. Three services, two models, one route, one utility, two test files. Adding a discount should touch maybe two files: the discount logic and its tests.

Measuring It

Run this against your git history:

# Count files changed per commit (excluding merges)
git log --no-merges --pretty=format:'' --name-only |   awk 'NF' |   sort |   uniq -c |   sort -rn |   head -20

If your top commits consistently touch 8+ files across different directories, you have a shotgun surgery problem. One or two such commits is normal (migrations, renames). Half your commits being this way means architectural problems.

The Fix: Move Behavior to Data

The refactoring is conceptually simple: behavior should live with the data it operates on. In practice, getting there without breaking production is the hard part.

For Feature Envy: Extract and Move

Step 1: Identify methods that belong elsewhere. (Use the coupling heuristic above.)

Step 2: Extract the envious code into a new method on the current class.

Step 3: Move that method to the class it envies.

Step 4: Make the original method delegate.

// BEFORE: PromotionService envies User
class PromotionService {
  calculateDiscount(user: User, order: Order): number {
    const loyaltyTier = user.getLoyaltyTier();
    const totalSpend = user.getTotalSpend();
    // ... 40 lines using user data
  }
}

// STEP 2: Extract the user-specific logic
class PromotionService {
  calculateDiscount(user: User, order: Order): number {
    const userDiscount = this.calculateUserDiscount(user);
    return order.getSubtotal() * userDiscount;
  }
  
  private calculateUserDiscount(user: User): number {
    // All the envious logic isolated here
  }
}

// STEP 3: Move to User class
class User {
  // All the data is here anyway
  private loyaltyTier: LoyaltyTier;
  private totalSpend: number;
  private joinDate: Date;
  
  getDiscountRate(): number {
    // The logic now lives with its data
    if (this.loyaltyTier === 'GOLD' && this.totalSpend > 10000) {
      return 0.15;
    }
    // ...
  }
}

// STEP 4: Original method delegates
class PromotionService {
  calculateDiscount(user: User, order: Order): number {
    return order.getSubtotal() * user.getDiscountRate();
  }
}

The PromotionService method went from 40 lines to 1 line. The User class gained a meaningful method instead of just exposing raw data. Win-win.

When Moving Isn't Possible

Sometimes you can't modify the envied class. Third-party library. Generated code. "Owned" by another team with a 6-week change request queue.

Options:

Extension class: Create a UserDiscountCalculator that takes User as a constructor parameter and encapsulates all the discount logic. Not as clean as modifying User directly, but it groups the envious behavior in one place.

Domain event: If the classes are in different bounded contexts, maybe they shouldn't talk directly at all. Publish a UserEligibilityRequested event, let the user domain respond with eligibility data. More infrastructure, but better isolation.

Admit defeat: Sometimes coupling is correct. A ReportGenerator that pulls data from 15 different entities isn't envious — it's doing its job. Not every smell indicates a problem.

Fixing Shotgun Surgery: Find the Missing Abstraction

Shotgun surgery usually means a concept exists in your domain that doesn't exist in your code.

The birthday discount example: "discount rule" is a concept. It has behavior (calculate eligibility, calculate amount), it has data (trigger conditions, discount percentage), it can be tested independently. But in the codebase, discount logic is scattered across services.

// The missing abstraction
interface DiscountRule {
  isApplicable(context: DiscountContext): boolean;
  calculate(context: DiscountContext): Money;
}

// Now each discount is self-contained
class BirthdayDiscountRule implements DiscountRule {
  isApplicable(context: DiscountContext): boolean {
    return context.user.isBirthday() && context.order.subtotal.gte(50);
  }
  
  calculate(context: DiscountContext): Money {
    return context.order.subtotal.multiply(0.20);
  }
}

class LoyaltyDiscountRule implements DiscountRule {
  isApplicable(context: DiscountContext): boolean {
    return context.user.loyaltyTier === 'GOLD';
  }
  
  calculate(context: DiscountContext): Money {
    return context.order.subtotal.multiply(0.15);
  }
}

// Adding a new discount: ONE file, ONE test file
class FridayFlashSaleRule implements DiscountRule {
  // ...
}

Adding a new discount rule now touches two files: the rule implementation and its tests. No more shotgun surgery.

The Pattern Behind the Pattern

Shotgun surgery often indicates missing Strategy, Visitor, or Plugin patterns. The scattered logic wants to be polymorphic but ended up as switch statements scattered across the codebase.

Look for:

• Multiple switch/case or if/else chains on the same enum across different files
• Methods named handleX, processX, calculateX for different X values in different services
• The phrase "and then you also have to update..." in your PR descriptions

Each of these is a concept trying to escape the procedural prison you built for it.

The Incremental Migration Strategy

Rewriting everything at once isn't realistic. But you can fix these smells incrementally without breaking production.

The Parallel Path

Keep the old code. Write the new abstraction alongside it. Migrate callers one by one.

// OLD: scattered across services
class OrderService {
  applyDiscounts(order: Order, user: User): void {
    // Birthday discount inline
    if (user.isBirthday()) {
      order.discount = order.subtotal * 0.20;
    }
  }
}

// NEW: introduce abstraction, but keep old code working
class OrderService {
  constructor(
    private discountEngine?: DiscountEngine // optional during migration
  ) {}
  
  applyDiscounts(order: Order, user: User): void {
    if (this.discountEngine) {
      // New path
      const discount = this.discountEngine.calculate(order, user);
      order.discount = discount;
    } else {
      // Old path - still works
      if (user.isBirthday()) {
        order.discount = order.subtotal * 0.20;
      }
    }
  }
}

Feature flag the new path. Run both in production (shadow mode). Compare results. When they match for two weeks, delete the old path.

The Strangler for Methods

Apply the strangler fig at method level, not service level. Each envious method gets migrated independently.

Track progress:

PromotionService methods:
[x] calculateDiscount - moved to User.getDiscountRate()
[x] determineEligibility - moved to User.isEligibleForPromotion()
[ ] applyPromoCode - TODO
[ ] validateCoupon - TODO, blocked on CouponService refactor

When all methods are migrated, the service becomes a thin facade or disappears entirely.

What Automated Tools Catch

Manual smell detection is slow. Automated analysis is fast but requires configuration.

Useful metrics:

• LCOM (Lack of Cohesion of Methods): High LCOM means methods in a class don't share instance variables. Often indicates feature envy — the methods operate on something external.
• Afferent Coupling: How many external classes reference this class. High numbers suggest the class is a dependency magnet.
• Efferent Coupling: How many classes this class depends on. High numbers suggest the class is doing too much.
• Change coupling (temporal coupling): Files that frequently change together in commits. Indicates hidden dependencies.

ScanMyCode.dev runs these analyses automatically and flags classes with problematic coupling patterns. The output shows exactly which methods have high external coupling and suggests where behavior might need to move. Useful when you're looking at a 50-service codebase and don't know where to start.

When the Smell Is Actually Fine

Not all feature envy needs fixing. Some patterns legitimately require cross-object coordination:

Mappers/Transformers: A UserDTOMapper that reads from User and writes to UserDTO is supposed to touch both objects heavily. That's the point.

Facades: A class that orchestrates multiple services will reference them all. The facade pattern is intentional.

Event handlers: A handler responding to UserCreated might touch User, Email, Analytics, and Audit services. That's coordination, not envy.

The test: does this method's behavior logically belong to a single domain object, or is it genuinely cross-cutting? If cross-cutting, maybe it's correct. If it should belong to one object, move it.

Preventing Future Accumulation

Code review is the main defense. But reviewers are human and miss stuff, especially under deadline pressure.

Add automated checks:

# Example: Danger.js rule for coupling
warn("This method references other objects more than 'this'") if
  git.modified_files.any? do |file|
    # Crude but effective
    content = File.read(file)
    external_refs = content.scan(/[a-z]+.[a-z]+(/i).length
    internal_refs = content.scan(/this./i).length
    external_refs > internal_refs * 3
  end

Not perfect. Will have false positives. But it surfaces potential problems early, before they get merged and forgotten.

Architecture Decision Records (ADRs) help too. When someone proposes adding a discount calculation to OrderService, a previous ADR titled "Discount logic belongs in DiscountRules" provides ammunition to push back.

The Payoff

After six months of incremental refactoring, that e-commerce codebase looked different:

Adding a new discount rule: 2 files (rule + test). Previously: 5-8 files.

Bug in loyalty calculations: 1 file to investigate. Previously: grep through 4 services.

Average PR size: down 40%. Fewer files per change means smaller, faster reviews.

New developer onboarding: "Look at the DiscountRule interface" instead of "Let me explain how discounts work across these 6 services."

None of this was a big-bang rewrite. Just persistent, incremental movement of behavior to where it belongs. One method at a time. One PR at a time.

If you're looking at a codebase full of these patterns and want an objective assessment of where to focus, run a code review audit. You'll get a report in 24 hours showing which classes have the worst coupling and where refactoring will have the highest impact.