timothywarner/copilot-dev/legacy-code-refactor

// BEFORE — var is function-scoped and hoisted; assignment in loops leaks
var maxRetries = 3;
var i;
for (var i = 0; i < maxRetries; i++) { ... }
// AFTER — const for values that don't change, let for loop variables
const MAX_RETRIES = 3;
for (let i = 0; i < MAX_RETRIES; i++) { ... }

// BEFORE — nested callbacks; error handling at every level; hard to read
function getUserOrders(userId, callback) {
  db.findUser(userId, function(err, user) {
    if (err) return callback(err);
    db.findOrders(user.id, function(err, orders) {
      if (err) return callback(err);
      callback(null, orders);
    });
  });
}
// AFTER — linear, readable, error bubbles up naturally
async function getUserOrders(userId) {
  const user = await db.findUser(userId);
  const orders = await db.findOrders(user.id);
  return orders;
}

// BEFORE — prototype-based "class" pattern
function Animal(name) {
  this.name = name;
}
Animal.prototype.speak = function() {
  return this.name + ' makes a sound.';
};
// AFTER — ES2022 class with private fields
class Animal {
  #name; // private field — not accessible outside the class
  constructor(name) {
    this.#name = name;
  }
  speak() {
    return `${this.#name} makes a sound.`;
  }
}

// BEFORE — 86400000 and 7 have no context
if (Date.now() - lastLogin > 86400000 * 7) {
  lockAccount(user);
}
// AFTER — intent is crystal clear; one place to change the policy
const MS_PER_DAY = 24 * 60 * 60 * 1000;
const INACTIVITY_LOCK_DAYS = 7;
const INACTIVITY_LOCK_THRESHOLD_MS = MS_PER_DAY * INACTIVITY_LOCK_DAYS;
if (Date.now() - lastLogin > INACTIVITY_LOCK_THRESHOLD_MS) {
  lockAccount(user);
}

// BEFORE — unhandled rejection crashes the process silently in older Node
async function fetchUserData(id) {
  const response = await fetch(`/api/users/${id}`);
  const data = await response.json();
  return data;
}
// AFTER — every failure path is handled explicitly
async function fetchUserData(id) {
  let response;
  try {
    response = await fetch(`/api/users/${id}`);
  } catch (networkError) {
    throw new Error(`Network request failed for user ${id}: ${networkError.message}`);
  }
  if (!response.ok) {
    throw new Error(`API returned ${response.status} for user ${id}`);
  }
  try {
    return await response.json();
  } catch (parseError) {
    throw new Error(`Failed to parse API response for user ${id}: ${parseError.message}`);
  }
}

// BEFORE
var message = 'Hello, ' + user.name + '! You have ' + count + ' messages.';
// AFTER
const message = `Hello, ${user.name}! You have ${count} messages.`;

// BEFORE — == coerces types; '5' == 5 is true
if (userId == null) { ... }   // catches null AND undefined — use this intentionally
if (status == 'active') { ... } // BAD: type coercion is a bug waiting to happen
// AFTER — === never coerces
if (userId === null || userId === undefined) { ... }  // explicit
if (status === 'active') { ... }  // clear intent

// BEFORE — one function does validation, persistence, AND response formatting
async function saveUser(req, res) {
  if (!req.body.email) { res.status(400).send('Email required'); return; }
  const user = await db.save(req.body);
  res.json({ id: user.id, email: user.email });
}
// AFTER — each function has one job
function validateUserInput(body) {
  if (!body.email) throw new ValidationError('Email is required');
}
async function persistUser(data) {
  return db.save(data);
}
async function createUserHandler(req, res) {
  validateUserInput(req.body);
  const user = await persistUser(req.body);
  res.json({ id: user.id, email: user.email });
}

// REFACTORED: var → const (never reassigned)
const MAX_RETRIES = 3;
// REFACTORED: magic number → named constant (86400000 ms = 1 day)
const MS_PER_DAY = 24 * 60 * 60 * 1000;
// REFACTORED: callback → async/await (same behavior, linear flow)
async function getUserOrders(userId) { ... }

// Example test structure for a refactored module
describe('getUserOrders', () => {
  it('returns orders for a valid user ID', async () => {
    // ... test the happy path
  });
  it('throws when the user is not found', async () => {
    // ... test that errors bubble up correctly
  });
});