8 JavaScript Mistakes I See in Every Code Review (And How to Fix Them)

#javascript #webdev #codequality #beginners

After reviewing hundreds of PRs, these are the patterns that keep coming up. Let's fix them once and for all.

1. Using == instead of ===

// 🚫 Wrong - type coercion is unpredictable
if (user.age == "18") ...
if (count == null) ...
if (0 == false) ...   // true!
if ("" == false) ...  // true!

// ✅ Correct - strict equality, no surprises
if (user.age === 18) ...
if (count === null || count === undefined) ...
// Or better:
if (count == null) ...  // Only acceptable for null/undefined check

Exception: == null is fine when checking for both null AND undefined.

2. Mutating function parameters

// 🚫 Wrong - mutates the caller's object
function addTimestamp(user) {
  user.createdAt = new Date();
  return user;
}

const admin = { name: 'Alice', role: 'admin' };
const timestamped = addTimestamp(admin);
console.log(admin.createdAt); // Oops! admin was mutated too

// ✅ Correct - return a new object
function addTimestamp(user) {
  return { ...user, createdAt: new Date() };
}

3. Not handling async errors

// 🚫 Wrong - crashes the whole app on network error
async function fetchUser(id) {
  const response = await fetch(`/api/users/${id}`);
  return response.json();
}

// ✅ Correct - handle errors where they happen
async function fetchUser(id) {
  try {
    const response = await fetch(`/api/users/${id}`);
    if (!response.ok) throw new Error(`HTTP ${response.status}`);
    return { data: await response.json(), error: null };
  } catch (err) {
    return { data: null, error: err.message };
  }
}

const { data, error } = await fetchUser(42);
if (error) { /* handle */ }

4. Creating functions inside loops

// 🚫 Wrong - creates N closures, all referencing the same `i`
const handlers = [];
for (var i = 0; i < 5; i++) {
  handlers.push(() => console.log(i));
}
handlers.forEach(h => h()); // 5 5 5 5 5 — not what you want!

// ✅ Correct option 1 - use let (block scoping)
for (let i = 0; i < 5; i++) {
  handlers.push(() => console.log(i));
}
// 0 1 2 3 4 ✓

// ✅ Correct option 2 - just use map
const handlers = [0,1,2,3,4].map(i => () => console.log(i));

5. Forgetting to clean up side effects

// 🚫 Wrong - memory leak and stale state
function SearchComponent() {
  const [results, setResults] = useState([]);

  useEffect(() => {
    // If component unmounts before fetch completes, setState on unmounted component
    fetchSearch(query).then(data => setResults(data));
  }, [query]);
}

// ✅ Correct - clean up with AbortController
function SearchComponent() {
  const [results, setResults] = useState([]);

  useEffect(() => {
    const controller = new AbortController();

    fetchSearch(query, { signal: controller.signal })
      .then(data => setResults(data))
      .catch(err => {
        if (err.name !== 'AbortError') console.error(err);
      });

    return () => controller.abort(); // Cleanup!
  }, [query]);
}

6. Not using early returns

// 🚫 Wrong - deeply nested, hard to read
function processOrder(order) {
  if (order) {
    if (order.items.length > 0) {
      if (order.user.isVerified) {
        if (order.total > 0) {
          return submitOrder(order);
        }
      }
    }
  }
  return null;
}

// ✅ Correct - flat, readable, each failure is explicit
function processOrder(order) {
  if (!order) return null;
  if (order.items.length === 0) return null;
  if (!order.user.isVerified) return null;
  if (order.total <= 0) return null;

  return submitOrder(order);
}

7. Using Array.forEach when you need a result

// 🚫 Wrong - forEach doesn't return, forces mutable pattern
const doubled = [];
[1, 2, 3].forEach(n => doubled.push(n * 2));

// 🚫 Also wrong - mixing concerns
const results = [];
users.forEach(user => {
  if (user.active) {
    results.push({ ...user, displayName: user.name.trim() });
  }
});

// ✅ Correct - use the right method
const doubled = [1, 2, 3].map(n => n * 2);

const results = users
  .filter(user => user.active)
  .map(user => ({ ...user, displayName: user.name.trim() }));

8. Not using environment checks properly

// 🚫 Wrong - exposes sensitive info, breaks across environments
const API_URL = "https://api.production.com"; // Hardcoded!
const DEBUG = true; // Always enabled!

// 🚫 Also wrong - string comparison with typo risk  
if (process.env.NODE_ENV == "producton") { ... } // Typo!

// ✅ Correct - centralised config
// config.js
export const config = {
  apiUrl: process.env.REACT_APP_API_URL || 'http://localhost:3000',
  isDev: process.env.NODE_ENV === 'development',
  isProd: process.env.NODE_ENV === 'production',
};

// Use everywhere
if (config.isDev) console.log('Debug info:', data);

Quick Reference

Anti-pattern	Fix
`==` loose equality	`===` strict equality
Mutating params	Spread: `{ ...obj }`
Unhandled async	try/catch + error state
var in loops	let or map
No cleanup in useEffect	return cleanup function
Deep nesting	Early returns
forEach with result	map/filter/reduce
Hardcoded config	Environment variables

Did I miss any patterns you see often? Drop them in the comments!

DEV Community