January 27, 2017 at 5:57 pm
I can't speak to the "managed code" side of the house but we review ALL database code. New code is reviewed head to toe for accuracy, possible performance problems, code style (which is identified in our standards), sufficient/appropriate inline comments, the appropriate code header (also clearly defined in our standards), and the start of "revision history". All code is checked into SVN. Revisions in code must not only be marked in the "revision history" in the header but revision numbers are added to the code where the changes took place to assist in troubleshooting as with any comments. We also do a diff with the previous version to make sure of that and to catch any other changes that the developer may have made and forgotten to comment.
When I first implemented all of this about 5 years ago, the CPUs where making lots of hot water as they ran at 40 to 50% and logical and physical reads were "off the chart" of reason. The code reviews have been used as a time to mentor and train and the Developers (capital "D" this time) have done a remarkable job. Not only have code reviews gotten a whole lot shorter but average CPU usage is generally down around 10-12% and that's up from the 4-8% because we've tripled the number of people hitting the GUIs in the last year alone. 5 Years ago, the main database contained only 65 GB. Today, we have several databases each nearing the 1TB mark (we're required to keep bloody everything). 5 years ago, we had 10 minute long "outages" on the floor several times a day especially if a large report or data import needed to be run. Today, even mild slowdowns aren't tolerated and, for the most part, we have none. We've eliminated nearly 200TB of logical reads that we used to suffer in a 10 hour period during business hours (and, no, we didn't move them to nighttime hours... it was all from the GUI and a combination of some really nasty ORM code and some stored procedures)
And rework due to error/not meeting requirements/not handling exceptions correctly has dropped tremendously. Instead of having to touch code two, three, or more times, we generally build it right the first time which means that we have more time to build more code right the first time.
We (well, not me... I was against it) have taken some shortcuts during two high profile "gotta have it now" projects in the last 5 years. Both resulted in a whole lot of rework due to accuracy problems, missed requirements, performance problems, and resource usage all of which required a shedload of rework and all the retesting. Additionally, both projects missed their scheduled dates due to the amount of rework and retesting that needed to be done.
Since "firefighting" seems to be the modus operandi for a lot of development teams, let me quote what the famous oil well firefighter, Mr. Paul Neal "Red" Adair supposedly said... "If you think it's expensive to hire a professional to do the job, wait until you hire an amateur." https://www.brainyquote.com/quotes/authors/r/red_adair.html To paraphrase that, I'll say "If you think it's expensive to do 100% peer reviews, wait until you don't". 😉
Heh... also always remember that "If you want it real bad, that's usually the way you'll get it." 😉
--Jeff Moden
Change is inevitable... Change for the better is not.
January 28, 2017 at 9:12 pm
Due to Bitbucket, GIT, and JIRA, I would say I review over every line of code that submitted. I don't find it necessarily hard for me, but that really all depends on the scale of code and submissions to change. I'm working on a small team and therefore, it's easier for me to review code changes.
If there was a much larger team, then obviously I do not feel I'm beneficial to the business to be reviewing code 8 hours a day. I would likely break and go insane. Adjustments will need to be made. When I worked on 250-man development teams, this was distributed to the seniors of the smaller strike groups we were apart of. I would likely adapt similar as it seems to be the norm and it allows code reviews to remain in the smaller groups who can better support themselves during the sprints.
February 7, 2017 at 6:11 am
After an embarrassing disaster one organisation introduced proper quality procedures with proper documentation for all stages. This included peer reviews of all code (which had to follow guidelines) and a virtual end to the fire fighting mentality. Following a restructuring and change of MD this was cut back and the bug count increased. The company no longer exists but I had moved on before then. I can't help thinking cutting back on quality was the beginning of the end!
May 6, 2017 at 12:16 pm
Gary Varga - Friday, January 27, 2017 2:49 AMIt is deceptively simple: every line of code is reviewed to at least a minimal level and there is no way of circumventing that."What?" I hear some cry out. "How can you allocate the people to achieve that?"
As many are likely to have guessed the minimal level I am referring to is the static code analysis which is done over absolutely all of the code and forms part of the Continuous Integration (CI)/Continuous Delivery (CD) pipeline that is the only route to live. You can also get statistics regarding code coverage by your automated tests. That all said there is value in manual peer reviews. This can keep track of best practices, sensible commenting, use of design practices, meaningful identifiers and adherence to the architecture and design of the piece of work. The combination of the two means that the manual peer review process can be targetted to the most sensitive areas of the system and the developers who need the most guidance (reviews are not there to attack anyone but to ensure high quality both now and in the future i.e. to improve individuals not to knock their coding).
Sounds good. But it relies of all participating in a code review is that the function is to get high quality code, not to attack people or knock their coding. I've seen (a long time ago - it doesn't happen when i'm managing the development team) so-called code reviews turning into really unpleasant nastiness when some prima donna "reviewer" claims that his idea is the only sensible way to do anything although it's blatatly obvious that doing it his way would be a stupid mistake introducing pointless complexity. Then there are people who think they will lose status if they attend a code review and don't find fault with something, probably because some lunatic decided a code review should be conducted by a group of 6 or 8 people gathered round table and overseen by a manager and all looking at and commenting on the same code at the same time, instead of being done by each reviewer individually. And people who don't want their code reviewed because they've suffered from code reviews chaired by managers who don't know good code from rubbish and carried out by reviewers who know they will be in that manager's bad books if they don't find some faults, so the developers have been taught very efficiently that the function of a code review is get their colleagues to humiliate them.
So if you can get code reviews done the way you describe, they are just great. If they are done wrong, which appears to be the usual way they are done, they can be a morale-destroying team-wrecking disaster.
Tom
May 6, 2017 at 3:17 pm
The success or failure of a code review process depends on the motivation behind it, much like Tom describes. If it's used to ensure good, performant, maintainable code gets into production and the junk is kept out, it's good. If it's used as a teaching or mentoring opportunity, it's good. If it's used to beat people over the head and make them feel worthless, it's worse than worthless because it won't help anyone or anything get better. Like so many things, it depends.
May 6, 2017 at 3:51 pm
TomThomson - Saturday, May 6, 2017 12:16 PMGary Varga - Friday, January 27, 2017 2:49 AMIt is deceptively simple: every line of code is reviewed to at least a minimal level and there is no way of circumventing that."What?" I hear some cry out. "How can you allocate the people to achieve that?"
As many are likely to have guessed the minimal level I am referring to is the static code analysis which is done over absolutely all of the code and forms part of the Continuous Integration (CI)/Continuous Delivery (CD) pipeline that is the only route to live. You can also get statistics regarding code coverage by your automated tests. That all said there is value in manual peer reviews. This can keep track of best practices, sensible commenting, use of design practices, meaningful identifiers and adherence to the architecture and design of the piece of work. The combination of the two means that the manual peer review process can be targetted to the most sensitive areas of the system and the developers who need the most guidance (reviews are not there to attack anyone but to ensure high quality both now and in the future i.e. to improve individuals not to knock their coding).
Sounds good. But it relies of all participating in a code review is that the function is to get high quality code, not to attack people or knock their coding. I've seen (a long time ago - it doesn't happen when i'm managing the development team) so-called code reviews turning into really unpleasant nastiness when some prima donna "reviewer" claims that his idea is the only sensible way to do anything although it's blatatly obvious that doing it his way would be a stupid mistake introducing pointless complexity. Then there are people who think they will lose status if they attend a code review and don't find fault with something, probably because some lunatic decided a code review should be conducted by a group of 6 or 8 people gathered round table and overseen by a manager and all looking at and commenting on the same code at the same time, instead of being done by each reviewer individually. And people who don't want their code reviewed because they've suffered from code reviews chaired by managers who don't know good code from rubbish and carried out by reviewers who know they will be in that manager's bad books if they don't find some faults, so the developers have been taught very efficiently that the function of a code review is get their colleagues to humiliate them.
So if you can get code reviews done the way you describe, they are just great. If they are done wrong, which appears to be the usual way they are done, they can be a morale-destroying team-wrecking disaster.
It's really unfortunate that it comes to that in a lot of companies and it's flat out wrong and a seriously stupid attitude well deserving of high velocity pork launched at point blank range.
I use peer reviews as an opportunity to mentor, coach, and to further interaction between myself and the Developers. The code from the Developers that I've worked with (and I sit with them to answer any questions during development) has gotten to the point where the peer reviews are an absolute breeze because they're not only very well written, but properly documented to boot!
The establishment of well written, easy to understand and follow standards go a long way to prevent people from becoming "point mongers" during reviews. The key is "Team Work". Everyone needs to understand that while code needs to move, it needs to move correctly. I even have the Developers review my stuff because, although I'm the DBA, the rules and standards apply to everyone and that includes me. The standards do have some irrefutable points (like the proper way to do a joined update or why it's imperative to not use an rCTE for a counting task and the format and content of the required header, as a few examples) but they are not written in such a fashion as to discourage any type of innovation. Reviewers need to learn that, with a few imperative exceptions for safety, performance, and auditability, one-way streets in IT should generally be the exception rather than the rule and that they can actually learn something new from Developers.
The other thing to remember is that not all code is new code and there is little need to review the entire content of, say, a thousand line stored procedure that only suffered the addition of, for example, a single AND condition on a single WHERE clause. The existing code get's checked into the release folder for the project or individual ticket, the code is modified by the Developer and checked in, the peer review happens and get's checked in whether any changes were required or not (annotation that the review was done is added). For such a small change, there should be two things that changed in the code... a Revision History entry must have been made and the change itself. If the code had been completely reviewed from the git or during other changes, then you only need to review the changes (and that nothing else was slipped in) and that usually takes just a minute.
The real key again, is Team Work. If either side "doesn't get it", it's not going to work. That also means the standards have to grant the correct authority to reject code and who can override rejections or reject code that's been approved. It you don't have that written in stone, people will make crap up on the fly (especially for "urgent" things", which we also have written into the standards) and it's not going to work. If management doesn't backup the standards, then you might as well use them for toilet paper because a lot of crappy code is going to happen and you'll need something to wipe with. 😉
--Jeff Moden
Change is inevitable... Change for the better is not.
May 8, 2017 at 12:30 am
Jeff Moden - Saturday, May 6, 2017 3:51 PMTomThomson - Saturday, May 6, 2017 12:16 PMGary Varga - Friday, January 27, 2017 2:49 AMIt is deceptively simple: every line of code is reviewed to at least a minimal level and there is no way of circumventing that."What?" I hear some cry out. "How can you allocate the people to achieve that?"
As many are likely to have guessed the minimal level I am referring to is the static code analysis which is done over absolutely all of the code and forms part of the Continuous Integration (CI)/Continuous Delivery (CD) pipeline that is the only route to live. You can also get statistics regarding code coverage by your automated tests. That all said there is value in manual peer reviews. This can keep track of best practices, sensible commenting, use of design practices, meaningful identifiers and adherence to the architecture and design of the piece of work. The combination of the two means that the manual peer review process can be targetted to the most sensitive areas of the system and the developers who need the most guidance (reviews are not there to attack anyone but to ensure high quality both now and in the future i.e. to improve individuals not to knock their coding).
Sounds good. But it relies of all participating in a code review is that the function is to get high quality code, not to attack people or knock their coding. I've seen (a long time ago - it doesn't happen when i'm managing the development team) so-called code reviews turning into really unpleasant nastiness when some prima donna "reviewer" claims that his idea is the only sensible way to do anything although it's blatatly obvious that doing it his way would be a stupid mistake introducing pointless complexity. Then there are people who think they will lose status if they attend a code review and don't find fault with something, probably because some lunatic decided a code review should be conducted by a group of 6 or 8 people gathered round table and overseen by a manager and all looking at and commenting on the same code at the same time, instead of being done by each reviewer individually. And people who don't want their code reviewed because they've suffered from code reviews chaired by managers who don't know good code from rubbish and carried out by reviewers who know they will be in that manager's bad books if they don't find some faults, so the developers have been taught very efficiently that the function of a code review is get their colleagues to humiliate them.
So if you can get code reviews done the way you describe, they are just great. If they are done wrong, which appears to be the usual way they are done, they can be a morale-destroying team-wrecking disaster.It's really unfortunate that it comes to that in a lot of companies and it's flat out wrong and a seriously stupid attitude well deserving of high velocity pork launched at point blank range.
I use peer reviews as an opportunity to mentor, coach, and to further interaction between myself and the Developers. The code from the Developers that I've worked with (and I sit with them to answer any questions during development) has gotten to the point where the peer reviews are an absolute breeze because they're not only very well written, but properly documented to boot!
The establishment of well written, easy to understand and follow standards go a long way to prevent people from becoming "point mongers" during reviews. The key is "Team Work". Everyone needs to understand that while code needs to move, it needs to move correctly. I even have the Developers review my stuff because, although I'm the DBA, the rules and standards apply to everyone and that includes me. The standards do have some irrefutable points (like the proper way to do a joined update or why it's imperative to not use an rCTE for a counting task and the format and content of the required header, as a few examples) but they are not written in such a fashion as to discourage any type of innovation. Reviewers need to learn that, with a few imperative exceptions for safety, performance, and auditability, one-way streets in IT should generally be the exception rather than the rule and that they can actually learn something new from Developers.
The other thing to remember is that not all code is new code and there is little need to review the entire content of, say, a thousand line stored procedure that only suffered the addition of, for example, a single AND condition on a single WHERE clause. The existing code get's checked into the release folder for the project or individual ticket, the code is modified by the Developer and checked in, the peer review happens and get's checked in whether any changes were required or not (annotation that the review was done is added). For such a small change, there should be two things that changed in the code... a Revision History entry must have been made and the change itself. If the code had been completely reviewed from the git or during other changes, then you only need to review the changes (and that nothing else was slipped in) and that usually takes just a minute.
The real key again, is Team Work. If either side "doesn't get it", it's not going to work. That also means the standards have to grant the correct authority to reject code and who can override rejections or reject code that's been approved. It you don't have that written in stone, people will make crap up on the fly (especially for "urgent" things", which we also have written into the standards) and it's not going to work. If management doesn't backup the standards, then you might as well use them for toilet paper because a lot of crappy code is going to happen and you'll need something to wipe with. 😉
Agreed with all in here. One thing I will react to in the editorial: just because it's urgent is NOT an excuse to skimp on this. I will actually go further than that: precisely BECAUSE it's urgent, it's vital to get it right the first time, so take the time to be thorough in the review and deployment procedures. Nothing worse that compounding a bad issue with a sloppy fix that introduces a whole new level of pain.
----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?
September 28, 2023 at 7:00 am
7 years on things have moved on. Human code reviews are hugely inefficient, inconsistent and prone to errors.
Collaborative design and design reviews are useful because they set you on the right course in the first place. I'm not suggesting big design up front, it is an opportunity for the team to call out upstream/downstream constraints and/or ways to simplify stuff. If you simplify stuff there is less to review anyway.
Pair programming seems to work as a review process. I haven't seen it done in many places as it is a tough sell to management. My experience is that two of you become intimately acquainted with the code base, question each other and end up with a better implementation and fewer bugs. In the longer term you also get fewer distractions where you seem to be shackled to the code base.
Mechanical reviews take place on our workstations using pre-commit hooks, one of which will be a hook to SQLFluff. We have a suite of tests that must pass, the majority of them can be run on our workstation. All the tests we run on our workstation plus a raft of others also run in the CI/CD pipeline. These have to pass before you can deploy to a shared environment.
There are quite a few tools that can be used for data testing.
The more you can automate the more time you have to think how to improve what you do. We start to think about what normal operation of our systems are and how we can detect anomalies, report and alert on those anomalies.
When you know what is outlying behaviour for your system you can start to investigate what needs to happen to mitigate those outliers.
The answer is NOT to instigate a committee or CAB (Change Advisory Board).
Viewing 8 posts - 16 through 22 (of 22 total)
You must be logged in to reply to this topic. Login to reply