April 27, 2015 at 1:49 pm
I have seen less and less code reviews as time goes by.
Current place does (regardless of the language):
1) Unit testing.
2) Peer review.
3) Component Integration Testing.
4) System Integration Testing.
5) Business Acceptance Testing (UAT).
6) Pre-production smoke testing.
7) Out of hours production trial.
No exceptions.
Having said that, it is in the highly regulated financial sector.
Gaz
-- Stop your grinnin' and drop your linen...they're everywhere!!!
December 14, 2018 at 3:20 am
I'm in the enviable(!)(?) position of being the lone DBA for a mid-size law firm, so I cannot get my work properly peer-reviewed. I have to explain what I have done, the syntax and the intended consequences - It makes change control interesting.
It's not so bad, there are only ~300 databases, of which ~75 can be considered production. There are benefits to being a lone DBA too, at least you mostly know what's going on and don't have to ask permission to change things like code style etc.
"Knowledge is of two kinds. We know a subject ourselves, or we know where we can find information upon it. When we enquire into any subject, the first thing we have to do is to know what books have treated of it. This leads us to look at catalogues, and at the backs of books in libraries."
— Samuel Johnson
I wonder, would the great Samuel Johnson have replaced that with "GIYF" now?
December 14, 2018 at 6:35 am
As a Lone Wolf developer I don't have the luxury of code reviews, either on the application front end or SQL backend. I also don't have much in the way of beta tester support either. Sigh.
As a result I have to rely heavily on the compiler and static analysis tools like SQL Prompt and ReSharper to catch code smells and the like. They help--a lot--but when it comes to logic problems that's all on me. So I instrument the code, set up as many automated unit tests (and a handful of systemic tests where I can) and hope for the best, pre-release.
Which, when developing a 600,000+ line application split evenly between .Net and T/SQL, is a bit nerve wracking.
Just wish unit testing was easier to deal with. After all, when you're the only developer writing test harnesses consumes WAY too much development time. Roughly doubles it, in fact.
December 14, 2018 at 7:42 am
Steve Jones - SSC Editor - Thursday, April 23, 2015 8:28 PMComments posted to this topic are about the item Code Reviews
Code reviews are part of my workday. We review everything we can: C#, Java, SQL, PoSH, cmd files and on and on. There are some things that defy review -- at least by one person: SSIS packages (reviewing the xml is pretty much impossible for mere mortals, reviewing changes in the designer means having the old and new versions open and eye-balling them for diffs -- while not forgetting to dive into properties and c# scripts etc, etc.). I for one, need the dev who submitted the review to walk me through their changes and hope that neither of us misses anything. Pretty much the same story for other machine-generated xml configs of any decent size.
December 14, 2018 at 8:11 am
Suggested code reviews, but never had any takers.
Getting through change control is diferent though - have to answer the most irrelevant questions (just so people can justify being there).
December 14, 2018 at 8:16 am
We review all DML and DDL before release to QA, etc.
December 14, 2018 at 9:46 am
One place I worked we did one code review and I was the reviewer. The person who’s code I reviewed was a VB6 / Access guy who was thrown into writing some C# functions. He had a reputation for being, um, well, let’s just say a bit temperamental. I knew this going in and gave him the most gentile code review ever although the shock on my face when I opened his code for the first time and was staring at a 1000-line function was probably pretty evident. After treating him with kid gloves and going back to my desk, he stormed into our boss’s office and ripped me up and down. How dare I criticize his code. Did I have any idea how long he’s been programming? On and on… My boss never had anyone do a code review after that and that was a shame. Especially after seeing the T-SQL code he wrote. He wrote one stored procedure so long and convoluted, that SSMS crashed trying to render the execution plan.
December 14, 2018 at 10:01 am
paul s-306273 - Friday, December 14, 2018 8:11 AMSuggested code reviews, but never had any takers.
Getting through change control is diferent though - have to answer the most irrelevant questions (just so people can justify being there).
With respect to change control, you're telling me! Supposedly we can be disciplined by not following procedure.
December 14, 2018 at 11:01 am
below86 - Friday, April 24, 2015 7:24 AMNo code reviews, no DBA. Yes there are times we really should review some individuals code, but who has the time? Yes we should make the time but you can't get upper management to give us the extra time needed."Code reviews? We don't need no stinkin' code reviews."
The above was at my prior job, we do code reviews at my current job. It can be a pain when you are the one having to do the reviews for others, when you are trying to get your own work done. What I'm running into is a few developers that wait almost to the last minute(not really that close). But I get the request to review either the day before it is to move to production or sometimes the same day. How are they expecting to make any corrections I find? A few times I've had to let a few 'standards' slide because of the critical nature of the change. These developers have been told not to wait so close to the deadline. We'll see if they listened.
I'm not saying I'm perfect, occasionally I'll have one that needs to be done within a day or so of me sending it to the person that reviews mine. But I usually try to give them a week before I plan to move to production..
-------------------------------------------------------------
we travel not to escape life but for life not to escape us
Don't fear failure, fear regret.
December 14, 2018 at 12:30 pm
We review code of all languages, including T-SQL. Everything is in code management and etiquette says you never merge your own pull request. There are a number of DBAs scattered around the company so it's never too hard to find a reviewer. As a company there's a drive (now supported from high up) to analyse the root cause of any incidents and to take the necessary actions to avoid repeating them; it would be a very bad look for an incident to be caused by code pushed to production without the safeguard of extra eyes being across it.
I wouldn't have answered the question the same when this thread was started 3 years ago, but there has been an outbreak of good practices in the meantime 😛
December 14, 2018 at 12:32 pm
When transitioning to a code review regime it is important to look at what works and what doesn't. The code review is to improve code but you also have to consider what can improve the code review.
A code review does take time and the time to review goes up exponentially with the amount to review. If you want a code review process to deliver break work down into small deliverables and peer review frequently. Look for opportunities to automate the things you peer review for. Not all is possible as of this moment but more can be automated now than we knew how to do 5 years ago.
December 14, 2018 at 2:21 pm
Like others we have 100% code reviews i.e. you don't merge your own pull requests. Our tasks are broken down into very small pieces so that no review is too onerous. It works very well: we have caught a number of crucial issues and are therefore saving time in the long term. It also ensures coding standards are being adhered to, and helps disseminate knowledge across the team.
December 17, 2018 at 3:58 am
Do not get me wrong, I like the idea of code reviews.
But it is not always easy to "code review". An example is SSIS, drag and drop "arghhhh". (I am guessing that biztalk has the same issue)
It holds everything in a huge XML file that imakes it almost impossible to make sense of.
The designer makes it hard to actually know what is going on underneath.
There are SQL code snippets, but other stuff is just so opaque that even looking at my own code after a few months away from it is not always as easy as i would like.
December 17, 2018 at 5:07 am
adelio.stevanato 21159 - Monday, December 17, 2018 3:58 AMBut it is not always easy to "code review". An example is SSIS, drag and drop "arghhhh". (I am guessing that biztalk has the same issue)
It holds everything in a huge XML file that imakes it almost impossible to make sense of.
The designer makes it hard to actually know what is going on underneath.
There are SQL code snippets, but other stuff is just so opaque that even looking at my own code after a few months away from it is not always as easy as i would like.
That's why it is important to provide a good description for each task (can be done in the Properties section) and have annotations where relevant.
I also add an Annotation block at the top of the Control Flow pane which includes who wrote the package, when they wrote it, what it does and a record of changes to the package over time.
It's possible to include all of this (and more) in a template package.
You can also add a text file to your project with a full description of how the package works. This will appear in the 'Miscellaneous' folder.
Regards
Lempster
December 17, 2018 at 5:15 am
And i do most of those but it is just hard to see everything about even a single SSIS item is one go.
You have properties, input/output columns, columns ignores, derived columns, c# Code modules, embedded SQL. All these things are hard to see in one go.
When you use something like vb/c# there is a lot more "code" to see.
with anything more than a few items in a package it starts to become harder and harder to document or find out everything easilly.
Viewing 15 posts - 31 through 45 (of 45 total)
You must be logged in to reply to this topic. Login to reply