مستشار إعادة هيكلة الكود: تحسينات قبل وبعد مع درجة مخاطرة لكل منها

Refactor 1: Duplicated validation logic — Risk: Low

Before:

if (!user.email || user.email.length === 0) { throw new Error('Email required'); }
if (!user.name || user.name.length === 0) { throw new Error('Name required'); }
if (!user.phone || user.phone.length === 0) { throw new Error('Phone required'); }

After:

const requiredFields = ['email', 'name', 'phone'];
for (const field of requiredFields) {
  if (!user[field]?.length) throw new Error(`${field} required`);
}

Verify before merging: Confirm error message format isn't parsed elsewhere by exact string match — the field name capitalization changed slightly.

Refactor 2: Unclear boolean flag parameter — Risk: Low

Before:

function processOrder(order, flag) {
  if (flag) { applyDiscount(order); }
  ...
}

After:

function processOrder(order, { applyLoyaltyDiscount }) {
  if (applyLoyaltyDiscount) { applyDiscount(order); }
  ...
}

Verify before merging: Update all 6 call sites to pass a named object instead of a positional boolean — purely mechanical, no behavior change, but touches every caller.

Refactor 3: Deeply nested conditionals — Risk: Medium

Before:

if (user.active) {
  if (user.subscription) {
    if (user.subscription.tier === 'pro') {
      // 15 more lines
    }
  }
}

After:

if (!user.active) return;
if (!user.subscription) return;
if (user.subscription.tier !== 'pro') return;
// 15 more lines, now unindented

Verify before merging: Early returns change control flow if there's an else branch further down handling the negative case elsewhere in the original function — check for one before applying. Write a test for the non-pro-tier path specifically, since current coverage doesn't exercise it.

Refactor 4: Function does three unrelated things — Risk: High

Before: Single 400-line function handles validation, discount calculation, and email notification in sequence.

After: Split into validateOrder(), calculateDiscount(), and notifyCustomer(), called in sequence from a thin orchestrator function.

Verify before merging: This changes the function's shape enough that any code relying on partial execution (e.g. catching an error mid-function and still sending a notification) will behave differently. Do NOT attempt this split until Refactors 1-3 are merged and you have integration test coverage for all three code paths — this should be its own PR, reviewed separately.

Summary

This function is moderately unhealthy but not unsalvageable. Refactors 1-2 are safe, mechanical wins you can ship today. Refactor 3 needs one new test first but is still low-stakes. Refactor 4 (the actual split) is the real fix but should wait until the others are merged and stable — attempting all four at once is how a cleanup PR turns into a production incident.