You're reviewing a teammate's pull request. The function looks fine at first glance — it's async, it has a try/catch, it returns strings. Then you look closer and realise it has a bug that will silently swallow errors and return the wrong thing in production.
Let's walk through it.
The code
async function isWinner(country) {
try {
country = await db.getWinner(country);
if (country === Error('Country Not Found')) {
return `${country.name} never was a winner`;
}
if (country.continent !== 'Europe') {
return `${country.name} is not what we are looking for because of the continent`;
}
let results = await db.getResults(country.id);
if (results === Error('Results Not Found')) {
return `${country.name} never was a winner`;
}
if (results.length < 3) {
return `${country.name} is not what we are looking for because of the number of times it was champion`;
}
return (
`${country.name} won the FIFA World Cup in ` +
results.map((result) => result.year).join(', ') +
' winning by ' +
results.map((result) => result.score).join(', ')
);
} catch (e) {
if (e.message === 'Country Not Found') {
return `${country} never was a winner`;
}
}
}
This function queries a database for a country's FIFA World Cup wins. It checks if a country exists, if it's in Europe, if it won at least 3 times, and then formats the results.
Reasonable goal. Shaky execution.
Bug #1: comparing errors with ===
if (country === Error('Country Not Found')) {
This will never be true.
Error('Country Not Found') creates a brand new Error object every time it's called. In JavaScript, objects are compared by reference, not by value. So country === Error('Country Not Found') is asking "is country the exact same object in memory as this new Error I just created?" — and the answer is always no.
const a = Error('Country Not Found');
const b = Error('Country Not Found');
console.log(a === b); // false — different objects
The same bug appears further down with results === Error('Results Not Found').
What was probably intended:
The database likely throws when a country isn't found, rather than returning an Error object. In that case, the catch block handles it. If the db genuinely returns an Error instance, the correct check is:
if (country instanceof Error) { ... }
// or
if (country?.message === 'Country Not Found') { ... }
Bug #2: overwriting the parameter
async function isWinner(country) {
try {
country = await db.getWinner(country); // ← overwrites the input
The function receives country as a string (e.g. "Brazil"), then immediately reassigns it to the database result object. This is why the catch block has to do this:
} catch (e) {
if (e.message === 'Country Not Found') {
return `${country} never was a winner`; // country is now an object, not a string
}
}
If db.getWinner throws before assigning, country is still the original string — fine. But if it throws after a partial assignment, country could be anything. And if db.getResults throws, country is already the database object, so ${country} would render as [object Object] never was a winner.
Fix: use a separate variable.
async function isWinner(countryName) {
const country = await db.getWinner(countryName);
...
}
Bug #3: the catch block returns undefined
} catch (e) {
if (e.message === 'Country Not Found') {
return `${country} never was a winner`;
}
// what happens here?
}
If the error is anything other than 'Country Not Found' — a network failure, a malformed response, a typo in the db call — the if condition is false and the catch block falls through without returning anything. The function returns undefined.
The caller gets back undefined with no indication that something went wrong. No rethrow, no fallback message, silent failure.
Fix: always handle the unexpected case.
} catch (e) {
if (e.message === 'Country Not Found') {
return `${countryName} never was a winner`;
}
throw e; // let unexpected errors propagate
}
A cleaner version
async function isWinner(countryName) {
let country;
try {
country = await db.getWinner(countryName);
} catch (e) {
if (e.message === 'Country Not Found') {
return `${countryName} never was a winner`;
}
throw e;
}
if (country.continent !== 'Europe') {
return `${country.name} is not what we are looking for because of the continent`;
}
let results;
try {
results = await db.getResults(country.id);
} catch (e) {
if (e.message === 'Results Not Found') {
return `${country.name} never was a winner`;
}
throw e;
}
if (results.length < 3) {
return `${country.name} is not what we are looking for because of the number of times it was champion`;
}
return (
`${country.name} won the FIFA World Cup in ` +
results.map((r) => r.year).join(', ') +
' winning by ' +
results.map((r) => r.score).join(', ')
);
}
Each async operation gets its own try/catch scoped to its own error. The parameter is never overwritten. Unexpected errors are rethrown rather than swallowed.
The three takeaways
1. Never compare Error objects with ===. Errors are objects — two errors with the same message are not the same object. Use instanceof Error or check .message on a caught error.
2. Don't reuse parameters as working variables. The moment you reassign a parameter, you lose the original value everywhere — including the catch block where you might need it most.
3. A catch block that doesn't rethrow is a promise to handle everything. If you only handle one specific error and silently drop the rest, you're hiding bugs. Either handle all cases or rethrow what you can't handle.
Code review isn't about finding someone to blame. It's about finding the bugs before users do. This one had three of them hiding in plain sight behind reasonable-looking syntax.
Tags: javascript async debugging codereview webdev
Top comments (0)