## JavaScript Review Checklist

- [ ] Strict equality (===) used, not loose (==)
- [ ] No var, use const/let appropriately
- [ ] Promises properly chained with error handling
- [ ] No callbacks in modern code (use async/await)
- [ ] Array methods used correctly (map/filter/reduce)
- [ ] No prototype pollution risks
- [ ] Event listeners cleaned up
- [ ] Memory leaks checked (closures, listeners)

🔴 [blocking]  - Must fix before merge (bugs, security, breaking)
🟡 [important] - Should fix, but discuss if you disagree
🟢 [nit]       - Nice to have, not blocking
💡 [suggestion]- Alternative approach to consider

// ❌ Using var - hoisting issues, function scoped
var user = "John";
if (true) {
  var user = "Jane"; // Overwrites outer variable!
}
console.log(user); // 'Jane'

// ✅ Use const by default, let when reassignment needed
const user = "John";
if (true) {
  const user = "Jane"; // Block scoped, separate variable
}
console.log(user); // 'John'

// ❌ Using let when const would work
let config = { timeout: 5000 };
let users = [];
users.push({ name: "John" }); // Mutating, not reassigning

// ✅ Use const for references that don't change
const config = { timeout: 5000 };
const users = [];
users.push({ name: "John" }); // OK - mutating content, not reference

// ❌ String concatenation
const greeting = "Hello, " + name + "! You have " + count + " messages.";
const multiline = "Line 1\n" + "Line 2\n" + "Line 3";

// ✅ Template literals
const greeting = `Hello, ${name}! You have ${count} messages.`;
const multiline = `
Line 1
Line 2
Line 3
`;

// ❌ Anonymous callbacks
users
  .filter(function (user) {
    return user.active;
  })
  .map(function (user) {
    return user.name;
  });

// ✅ Arrow functions
users.filter((user) => user.active).map((user) => user.name);

// ❌ Repetitive property access
function processUser(user) {
  console.log(user.name);
  console.log(user.email);
  console.log(user.role);
}

// ✅ Destructuring
function processUser({ name, email, role }) {
  console.log(name);
  console.log(email);
  console.log(role);
}

// ✅ With defaults
function processUser({ name, email, role = "user" }) {
  // role defaults to 'user' if not provided
}

// ❌ Object.assign for shallow copy
const newConfig = Object.assign({}, config, { timeout: 10000 });
const allItems = array1.concat(array2);

// ✅ Spread operator
const newConfig = { ...config, timeout: 10000 };
const allItems = [...array1, ...array2];

// ❌ Loose equality - type coercion bugs
if (value == null) {
} // true for null AND undefined
if (value == 0) {
} // true for 0, '', false, []
if (value == false) {
} // true for 0, '', null, undefined
if ("5" == 5) {
} // true - string coerced!

// ✅ Strict equality
if (value === null || value === undefined) {
}
if (value === 0) {
}
if (value === false) {
}
if ("5" === 5) {
} // false - different types

// ✅ Exception: null/undefined check (intentional)
if (value == null) {
} // OK for specifically null/undefined check

// ❌ Falsy check when you want null check
function process(value) {
  if (!value) {
    // Fails for 0, '', false
    return "default";
  }
  return value;
}

process(0); // Returns 'default' - probably wrong!
process(""); // Returns 'default' - probably wrong!
process(false); // Returns 'default' - probably wrong!

// ✅ Nullish coalescing for null/undefined only
function process(value) {
  return value ?? "default"; // Only null/undefined trigger default
}

process(0); // Returns 0
process(""); // Returns ''
process(false); // Returns false
process(null); // Returns 'default'

// ❌ Logical OR treats 0, '', false as falsy
const count = options.count || 10; // 0 becomes 10!
const name = options.name || "Guest"; // '' becomes 'Guest'!

// ✅ Nullish coalescing
const count = options.count ?? 10; // 0 stays 0
const name = options.name ?? "Guest"; // '' stays ''

// ❌ Callback hell (pyramid of doom)
getUser(id, function (user) {
  getPosts(user.id, function (posts) {
    getComments(posts[0].id, function (comments) {
      getAuthor(comments[0].authorId, function (author) {
        // deeply nested, hard to maintain
      });
    });
  });
});

// ✅ Async/await
async function getPostData(id) {
  const user = await getUser(id);
  const posts = await getPosts(user.id);
  const comments = await getComments(posts[0].id);
  const author = await getAuthor(comments[0].authorId);
  return { user, posts, comments, author };
}

// ❌ Unhandled promise rejection
async function fetchData() {
  const response = await fetch("/api/data");
  return response.json();
}
fetchData(); // Rejection silently ignored!

// ✅ Always handle errors
async function fetchData() {
  try {
    const response = await fetch("/api/data");
    if (!response.ok) {
      throw new Error(`HTTP ${response.status}`);
    }
    return await response.json();
  } catch (error) {
    console.error("Fetch failed:", error);
    throw error; // Re-throw or handle appropriately
  }
}

fetchData().catch((error) => handleError(error));

// ❌ Sequential when parallel is possible
const user = await getUser(id);
const posts = await getPosts(id);
const settings = await getSettings(id);
// Each waits for the previous - slow!

// ✅ Parallel execution
const [user, posts, settings] = await Promise.all([
  getUser(id),
  getPosts(id),
  getSettings(id),
]);
// All run simultaneously - fast!

// ❌ One failure cancels everything
try {
  const results = await Promise.all([
    fetchUser(1),
    fetchUser(2), // If this fails...
    fetchUser(3),
  ]);
} catch (error) {
  // All results lost!
}

// ✅ Handle partial failures
const results = await Promise.allSettled([
  fetchUser(1),
  fetchUser(2),
  fetchUser(3),
]);

results.forEach((result, index) => {
  if (result.status === "fulfilled") {
    console.log(`User ${index + 1}:`, result.value);
  } else {
    console.log(`User ${index + 1} failed:`, result.reason);
  }
});

// ❌ Event listener not cleaned up - memory leak
class Widget {
  constructor() {
    window.addEventListener("resize", this.handleResize);
  }

  handleResize() {
    console.log("Resized");
  }
  // Widget removed but listener stays!
}

// ✅ Clean up listeners
class Widget {
  constructor() {
    this.handleResize = this.handleResize.bind(this);
    window.addEventListener("resize", this.handleResize);
  }

  handleResize() {
    console.log("Resized");
  }

  destroy() {
    window.removeEventListener("resize", this.handleResize);
  }
}

// ❌ Closure holding large data
function createHandler(largeData) {
  return function onClick() {
    console.log("Clicked");
    // largeData is captured in closure but never used!
    // Memory never released while handler exists
  };
}

// ✅ Don't capture unnecessary data
function createHandler() {
  return function onClick() {
    console.log("Clicked");
  };
}

// If data is needed, only capture what's necessary
function createHandler(userName) {
  // Just the name, not whole object
  return function onClick() {
    console.log(`${userName} clicked`);
  };
}

// ❌ Timer continues after component removed
function startPolling() {
  setInterval(() => {
    fetchData(); // Keeps running forever!
  }, 5000);
}

// ✅ Clear timer on cleanup
function startPolling() {
  const intervalId = setInterval(() => {
    fetchData();
  }, 5000);

  return () => clearInterval(intervalId);
}

// Usage
const stopPolling = startPolling();
// Later...
stopPolling();

// ❌ Observable not unsubscribed
class Component {
  init() {
    eventEmitter.on("update", this.handleUpdate);
    // Never unsubscribed - memory leak!
  }
}

// ✅ Unsubscribe on cleanup
class Component {
  init() {
    this.handleUpdate = this.handleUpdate.bind(this);
    eventEmitter.on("update", this.handleUpdate);
  }

  destroy() {
    eventEmitter.off("update", this.handleUpdate);
  }
}

// ❌ forEach for transformation (should use map)
const names = [];
users.forEach((user) => {
  names.push(user.name);
});

// ✅ map for transformation
const names = users.map((user) => user.name);

// ❌ filter + length for counting
const count = items.filter((item) => item.active).length;

// ✅ reduce for counting (no intermediate array)
const count = items.reduce((acc, item) => (item.active ? acc + 1 : acc), 0);

// Or just filter if readability matters more than perf
const count = items.filter((item) => item.active).length;

// ❌ sort mutates original array!
const sorted = users.sort((a, b) => a.age - b.age);
// users array is now sorted too!

// ✅ Copy before sorting
const sorted = [...users].sort((a, b) => a.age - b.age);
// Or
const sorted = users.toSorted((a, b) => a.age - b.age); // ES2023+

// Same issue with reverse()
const reversed = [...items].reverse();
// Or
const reversed = items.toReversed(); // ES2023+

// ❌ Dangerous - prototype pollution possible
function merge(target, source) {
  for (const key in source) {
    target[key] = source[key];
  }
  return target;
}
// Attacker could set __proto__!

// ✅ Check for dangerous keys
function merge(target, source) {
  for (const key of Object.keys(source)) {
    if (key === "__proto__" || key === "constructor" || key === "prototype") {
      continue;
    }
    target[key] = source[key];
  }
  return target;
}

// ✅ Or use Object.assign / spread
const merged = { ...target, ...source };

// ❌ for...in on arrays - iterates indices as strings, includes prototype
for (const item in array) {
  console.log(item); // '0', '1', '2' (strings!)
}

// ✅ for...of for arrays - iterates values
for (const item of array) {
  console.log(item); // actual values
}

// ✅ for...in for objects - iterates keys
for (const key in object) {
  if (Object.hasOwn(object, key)) {
    // Skip inherited
    console.log(key, object[key]);
  }
}

// ✅ Better: Object.entries
for (const [key, value] of Object.entries(object)) {
  console.log(key, value);
}