As engineers, we live and learn through our pull requests—sometimes they fly through approvals, and sometimes they come back with pages of feedback. I recently submitted a PR for a backend task involving brand deduplication in a large dataset (20K+ records). What followed was one of the most detailed PR analyses I’ve received in my career, and it gave me a ton to reflect on. Here's what I learned—both what worked and what could’ve been better.
âś… What Went Well
1. Scoped Commits Matter
The feedback confirmed that my three commits were well-targeted, each focusing on specific enhancements: brand validation, mapping canonical brands, and ensuring unique associations.
🔍 Takeaway: Even if you're working solo, scoped commits make the reviewer’s job easier and tell a clearer story.
2. Appropriate Use of Sets for Deduplication
While tackling the getCompanyMapping() logic, I leaned on JavaScript Sets to handle unique brand relations. The analysis explicitly called this out as the right approach.
🔍 Takeaway: Using Sets over arrays isn’t just syntactic sugar—it's about intent and performance. Use them when uniqueness matters.
3. Descriptive Variable Names
Variables like canonicalizedFlatMapObject, groupToCanonicalCompany, and potentialMatches were commended for clarity and purpose.
🔍 Takeaway: Don’t underestimate how much good naming helps your future self (and your reviewers).
4. No Redundant Logic or Duplication
There was no repeated lowercase conversion or misuse of the same variable. That means the code passed one of the most common readability traps.
🔍 Takeaway: Clean code is often about what’s not there—no clutter, no over-processing.
⚠️ Areas for Growth
1. Low Commit Count
While the commits were scoped, the total number (3) leaned toward the lower side. More frequent commits could’ve demonstrated step-by-step problem-solving.
💡 Reflection: In retrospective, I could’ve used commits to document my thought process, not just code transitions.
2. Pushing Data Files in Commits
This was the big red flag. Files like CompanyMapping.json and luxuryItems.json shouldn't have been committed. They were meant for local dev testing but slipped into the repo.
🧠Note to Self: If it's not essential to the execution or automation of the PR, it probably doesn't belong in the commit.
🙏 Final Thoughts
This experience reminded me that great engineers are constantly learning—even better when it's from mentors who take the time to offer detailed insights. It also reinforced something I live by: Strive to embody the best practices of a senior engineer, even when you’re still growing into the title.
If you’re navigating similar feedback, don’t see it as criticism—see it as fuel. Every PR is a chapter in the bigger story of your growth.
Thanks for reading! Have you received a code review that changed how you think about engineering? I’d love to hear your story in the comments 👇
Top comments (0)