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

Source: Dev.to

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)
const handlersLet = [];
for (let i = 0; i < 5; i++) {
  handlersLet.push(() => console.log(i));
}
handlersLet.forEach(h => h()); // 0 1 2 3 4 ✓

// ✅ Correct option 2 – just use `map`
const handlersMap = [0, 1, 2, 3, 4].map(i => () => console.log(i));
handlersMap.forEach(h => h()); // 0 1 2 3 4

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 appropriate array methods

// 🚫 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 – centralized 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

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