Trade-Offs

Let’s look at how to handle trade-offs in code reviews. We’ll show you a simple example and explain why the feedback matters.

The Scenario

A developer has implemented a function to validate a user’s password. The code works but could be optimized for better performance at the cost of readability.

passwordValidation.ts


// Before
function validatePassword(password: string): boolean {
  // Simple but potentially slow for long passwords
  const hasUpperCase = /[A-Z]/.test(password)
  const hasLowerCase = /[a-z]/.test(password)
  const hasNumber = /[0-9]/.test(password)
  const hasSpecialChar = /[!@#$%^&*]/.test(password)
  const isLongEnough = password.length >= 8
 
  return (
    hasUpperCase && hasLowerCase && hasNumber && hasSpecialChar && isLongEnough
  )
}

How will you comment on this code?

Choose the option that you think is the most constructive and helpful.

Itsik Mamancommented on Apr 2, 2025

This is too slow. Optimize it.

Itsik Mamancommented on Apr 2, 2025

Consider using a single loop instead of multiple regex tests. This would be more efficient for long passwords.

Itsik Mamancommented on Apr 2, 2025

Thanks for implementing the password validation! While the current implementation using regex is clear and maintainable, we could optimize it for better performance with long passwords. The new version uses a single pass through the string instead of multiple regex tests. Here’s a suggestion that balances readability with performance:

passwordValidation.ts


// After
function validatePassword(password: string): boolean {
  // Optimized for performance but still readable
  if (password.length < 8) return false;
 
  let hasUpperCase = false;
  let hasLowerCase = false;
  let hasNumber = false;
  let hasSpecialChar = false;
 
  for (let i = 0; i < password.length; i++) {
    const char = password[i];
    if (char >= 'A' && char <= 'Z') hasUpperCase = true;
    else if (char >= 'a' && char <= 'z') hasLowerCase = true;
    else if (char >= '0' && char <= '9') hasNumber = true;
    else if ('!@#$%^&*'.includes(char)) hasSpecialChar = true;
  }
 
  return hasUpperCase && hasLowerCase && hasNumber && hasSpecialChar;
}

Understanding the Improvements

Click here to learn more

1. Performance vs Readability

Original code uses regex which is clear but runs multiple tests
New code uses a single pass through the string
Clear explanation of the performance benefit

2. Code Quality

Better performance for long passwords
Still maintains code clarity
Balanced approach to optimization

3. Future Maintenance

Easier to understand the performance considerations
Clear documentation of trade-offs
Better for team collaboration

Tips for Handling Trade-offs

Start with Something Positive
- Acknowledge the working code
- Show you understand the current approach
- Example: “Great work on the password validation!”
Explain the Trade-offs
- What are the competing concerns?
- How do they affect the code?
- Example: “This balances readability with performance”
Be Specific
- Suggest concrete improvements
- Explain why they matter
- Example: “Using a single pass improves performance for long passwords”
Keep It Friendly
- Focus on improvement, not criticism
- Use encouraging language
- Example: “Thanks for considering performance!”

Common Pitfalls to Avoid

Being Too Critical
- ❌ “This is too slow. Fix it.”
- ✅ “Good work! Here’s how we can balance performance with readability.”
Not Explaining Why
- ❌ “Use a for loop instead.”
- ✅ “Using a single pass through the string improves performance while maintaining readability.”
Ignoring the Positive
- ❌ “This needs optimization.”
- ✅ “Great work! Here’s how we can make it even more efficient.”

Remember: The goal is to help your teammate write better code while maintaining a positive and collaborative environment!

Last updated on April 11, 2025