What are your favorite ways of dealing with software code reviews?
Posted by rag1987@reddit | ExperiencedDevs | View on Reddit | 23 comments
we are a team of 15 members in a big tech org and building a map positioning service which have many different feeds like (login, dashboard, map, drivers, vehicles etc..) it's bit complex system and more I participate in code reviews in my engineering team, more I believe that the main goal of a code reviewer is to ensure code quality and maintainability while giving a collaborative learning environment to team.
This is why I really appreciate approaches that focus on constructive feedback, clear communication, and continuous improvement rather than just finding faults or enforcing personal preferences and gate keeping.
Some of the highlights from me:
• The Pragmatic Programmer by Andrew Hunt and David Thomas - a classic that emphasizes the importance of code reviews and offers practical advice.
• Clean Code by Robert C. Martin - provides excellent insights on writing readable and maintainable code.
• GitHub's pull request features - especially the ability to comment on specific lines and start discussions within the PR.
• Pair programming sessions - not a direct replacement for code reviews, but can significantly reduce issues before the formal review process.
• Code review checklists - helps maintain consistency and ensures important aspects aren't overlooked.
• Code Complete by Steve McConnell - offers a comprehensive guide on software construction, including valuable sections on code reviews.
• Regular team discussions on code review practices - helps in aligning the team and the process.
https://martinfowler.com/articles/branching-patterns.html
but some these days some of team members saying we (team) should start thinking about AI code reviews at least for simple PRs where complexity is less and we can train the local LLM.
What are your experiences or opinions on AI code reviews?
eggeggplantplant@reddit
I do have to say that the more experienced i get, the more the clean code principles are the opposite of what i find to be maintainable and performant. Many of its approaches are actually things i will block in PRs nowadays, especially the overly short functions
Potato-Engineer@reddit
Yeah, I draw a line waaay bigger than Clean Code does; if you aggressively aim for short functions, you'll get ravioli code.
That said, breaking down larger functions does have a purpose, in that it generally makes you encapsulate your code a little more so that you have less of "one function doing everything." When there are 15 steps in a function, of which only about 10 need to happen in a particular order, breaking stuff out into sub-functions keeps you from interspersing those other 5 steps at random places.
Pleasant-Database970@reddit
this comment highlights a fundamental misunderstanding many people have: procedural thinking in an oo-setting.
if you think about code as a sequence of steps (aka a procedure) then extracting things into separate functions/classes is painful. because you're jumping around to follow a procedure. (this violates separating levels of abstraction) the things that you extract also tend to have inputs/outputs tailored to one specific procedure and aren't reusable.
instead if you extract value objects, and use the functions to transform/extract different data points, these are context-free (decoupled from any specific procedure), can be called in any order anytime, and are reusable in several places in your application. they also become more reliable.
then, your "procedure" can exist within a single function/class (see: service object) that only describes the high-level flow of the procedure, and delegates to the classes/functions that do low-level data manipulation or other heavy lifting. (see: separating levels of abstraction)
then encapsulation works in your favor. simple classes/functions with good naming means that you don't have to dig into those classes to debug/understand your code.
one indication you're using procedural thinking is naming classes and functions based on what they are doing...and not what the output represents. for example method names start with verbs like get, calculate, build. or naming classes after actors "SomethingBuilder", "LigmaParser".
not agreeing with clean code is almost always the result of a shallow/incomplete understanding. or worse, a misunderstanding of fundamental oo principles.
Potato-Engineer@reddit
To address the question more directly:
Which set of transform functions would make you happiest?
This hypothetical is absurd, of course, but as the number of functions increases, the odds of any single function actually giving you the thing you want for a new feature goes up for a while, and then it starts going way down; you'll probably need to string together several functions if each one is tiny, and debugging them gets harder once you get to "a giant number of really tiny functions."
ventilazer@reddit
would this be output oriented? calculateSquareRoot(num: number): number
caprica71@reddit
Why don’t you set it up and tell us what you think of AI code reviews?
mechkbfan@reddit
100% this. Once people actually start using the AI, the weak spots are stupidly immediate and you realise what it's good for, and what it's not.
I've had a few discussions with friends about AI replacing devs
There seems to be people that think AI will replace developers and I just don't think they've had applied any level of metacognition to realise how unachievable that would be
The conversations, the reflections of "this function worked well here but it didn't work well here" thought that's never written, the experiences of similar but different problems and reconciling the overlaps in the problem space. And I could go on and on
Can AI replace many things? Absolutely
Things where subjectiveness with a level of forgiveness for error? Absolutely. Music, video, literature, etc. 95% of the work could be done for a song, have someone come in and master/edit it a little bit for iron out the rough edges. No problem.
Things that are Turing complete? Absolutely. You tell me that AI has created a new optimised bubble sort using unheard techniques, and I'd believe you (but also want to verify it)
Tell me that it's automated building a supply chain web app? I laugh and then wait for a call to fix everything at 2x my normal rate for a year.
Coming back to OP. Could it replace code reviews?
I'd trust it as a next level code maid
"You've used these conventions elsewhere, but this convention is different, I can make it consistent for the whole app if you'd like? Tell me which one"
There could be learning opportunities
"Did you know a similar function already exists within the native library?"
I'd also expect it to discover memory leaks
But that's about it. I really wouldn't expect anything more
LloydAtkinson@reddit
You expect a lot of automated bullshit producing machines
mechkbfan@reddit
I've put enough hours into them (GitHub Copilot & ChatGPT) to see where they are capable and not.
I know my examples aren't quite there yet with current solutions, but give it a year and I'll say it will be.
flmontpetit@reddit
I'm reminded of early 20th century futurists who extrapolated all sorts of possibilities out of things that were at the time mind blowing, but weren't really conscious of the implications of what they were tacking on. For instance, it should only be a matter of time until cars can float. It's just a matter of inventing VTOL, fitting it into a car chassis, making it economical, and finally turning your average driver into a pilot.
I don't mean to condescend, but at the moment, anything a LLM produces that feels like ingenuity and inventiveness is simply derived from human ingenuity and inventiveness. Nothing indicates that it can escape this fundamental limitation.
mechkbfan@reddit
I might have been ambigious, but I didn't expect it to escape it. What I am expecting is that the limitations haven't been explored enough in the context of AI code reviews.
I'm expecting the developers of the tools themselves provide opinionated input & weightings to solve specific problems that occur. There's nothing crazy in the examples I mentioned. It's all pattern recognition and syntax checking with an existing datasets.
Maybe there's already a solution out there that does it and I'm just not aware of it.
rag1987@reddit (OP)
you're right about their current limitations. I've had similar experiences:
The key is using AI to augment our skills, not replace and well, before GPT, the automated tools we had were limited to linting or static code analysis and yes AI cannot fix all our code review problems but it might be good have for small PRs and build an internal CR tool based on rules and parameters.
grimdark-@reddit
I can certainly see AI code reviews being beneficial in some cases, and I agree that it should not be used to completely replace anything. Having it as an "add-on" might be best, if anything.
If I were to leverage AI code reviews, I would probably use it on a case-by-case basis initially. If a specific AI tool can markedly improve our existing workflows, reduce failure rates by detecting logic errors early on, and/or boost productivity in other ways, then I would strongly consider adding it to our CI/CD pipelines. It doesn't have to be perfect at detecting logic errors, as long as its false positive rate is very low (ideally as close to 0 as possible).
ApprehensiveKick6951@reddit
That seems to be a relatively reactionary response that doesn't consider the strengths of LLMs. LLMs, like us, operate on probabilistic reasoning. Developers are not less fallible than LLMs – just more accountable. LLMs are weak in areas that are novel or expanse, and strong in areas that are well-documented and concise.
If it can automate tedious work, you should usue it to automate this work. My org uses it to write PR descriptions and provide automated code reviews based on potentially unsafe code, improvements in error handling, etc. Overall, it's a value-add.
diablo1128@reddit
You already got a lot of responses about AI Code reviews, but I wanted to add that my experience has shown that it's a very fine line between "constructive feedback, clear communication, and continuous improvement" and "enforcing personal preferences and gate keeping". I find it's subjective in many cases and comes down to how the individual SWEs want to see code. It works out on teams when the majority thinks the same way, but that's more of an echo chamber than anything.
I've been on teams where the code was very procedural C++, but some people would voice that it was hard to work in and if it was more Clean Code by Robert C. Martin OOP then it would be better. Both sides saw the other side as "enforcing personal preferences" and dug in.
So while your drive for quality code is noble a lot of this comes down to the SWEs involved. One SWEs nit code review comment is another SWEs valuable comment. I love people pointing out spelling mistakes in my code reviews and I know other SWEs that think those are just nit comments and it's fine.
I seen other SWEs that a protective of their code in the sense that it works and it's not unreasonable so it fine. Adding a comment that these 20 lines can be replaced with a Lamba is thought of as being nit picky since they feel there is nothing wrong with there solution. You may see the Lamba is objective better, but they don't and it turns in to each side seeing the other as being difficult to work with and "enforcing personal preferences".
I don't consider either side bad SWEs, but many things come down to personal preference in some way. There is no universal coding standard in the software industry for a reason. It's not black and white on what the best way to do things and many solutions are fine overall.
Just relying my experiences over 15 years. I didn't work at tech companies, so it could be different at that tier of company. I worked at private non-tech companies in non-tech cities on safety critical medical devices like dialysis machines.
Potato-Engineer@reddit
Because callbacks are funny: did you mean lambda function?
renq_@reddit
Personally, I think the best code review is synchronous. I really hate the pull-request model, where you have to wait for someone else to comment and approve.
The best code review, in my opinion, is ongoing, where you don't have to wait because you're already writing the code with one or more other people.
Of course, there is a lot in between these two extremes. For example, if you don't like pair programming, you can do something like this. You have an idea, you call someone else, you improve that idea, you write a small change, then you review it and test together, after the merge you talk to the other person about the next step, and repeat.
gfivksiausuwjtjtnv@reddit
AI only for code reviews is a pretty radical idea because the usual workflow is the opposite- AI churns out plausible-looking but imperfect code and humans who are(hopefully) cleverer figure out the bits that totally suck.
Using AI to augment reviews is different.
nullcone@reddit
I set up a LLaMA-3.1 service with the 405B model earlier this year with the aspiration to augment code reviews with AI generated suggestions. The hope was we could system prompt with the entire Python codebase, then LLaMA would miraculously tell us everything we did poorly, create our unit tests, refactor things sensibly, etc.
In practice, it simply did not work. We could really only fit one or two levels of the submodule tree in the limited 130k tokens of context. Generating suggestions with the 405B model took forever, and was expensive because it requires a full 8 H100 GPUs to run. And worst of all, the suggestions it gave were often either flat out wrong, or at best nits about style and formatting. E.g. we use ruff for linting and LLaMA would try and tell us that style was inconsistent, even after linting.
So in general I am not at all bullish that AI will replace devs at any time in the near future.
PushHaunting9916@reddit
Just posted this yesterday in another thread.
At work, we use this code review guide. It's not fully complete, still missing information for adding different levels of comments such as nitpick, styling, etc. Using the guide has lowered emotions in the teams. In the end, software is for humans, and code reviews can be confronting, with frustrations potentially leaking through. But the idea was that it's very helpful and wholesome. It leads to fewer bugs, encourages knowledge sharing, and provides a second pair of eyes.
rag1987@reddit (OP)
nice guide https://github.com/Attumm/code_review
Gunningagap77@reddit
Let's put it this way: would you eat off of a plate that was washed using AI and had no contact with humans through the entire process?
rag1987@reddit (OP)
agree and you may would like to give it a read - https://mtlynch.io/human-code-reviews-1/