April 23, 2015 at 8:28 pm
Comments posted to this topic are about the item Code Reviews
April 23, 2015 at 8:41 pm
We do 100% code reviews on any and all things SQL. That includes me and I'm the DBA. I do NOT make production code or data changes without a code review and QA/UAT. What do we do in emergencies where code needs to be fixed or data needs to be changed? Exactly the same thing except the whole team is put on alert for such emergency urgencies. Depending on the nature of the change, we can correctly pound an emergency fix through the system in 10 to 30 minutes.
The key is that because we do 100% peer reviews and the code runs the gauntlet setup by our wonderful QA team, we've only had 4 such emergencies in the last 4 years and 0 in the last 14 months.
No... we're not stupid about it, either. There are several areas where we have pre-approved, previewed, thoroughly pretested code for certain key individuals to make changes for huge batch jobs for some of our clients.
--Jeff Moden
Change is inevitable... Change for the better is not.
April 24, 2015 at 1:06 am
Unfortunately I'm in the position whereby there is no-one to check my code before deployment (The closest I can get is someone opening it in SSMS and checking to make sure it either runs without an error, or has no red underlines in it...). If there is an issue where the code does something it shouldn't, yet is still valid SQL, no-one will spot it until later down the line (In most cases an end user which is never a good thing - makes me look rather daft!), meaning I have to drop everything and sort it.
Luckily this has made me get rather OCD about testing, getting users involved and getting feedback from them before I change anything in the real world - it's rare I introduce something that causes issues due to being so paranoid about messing things up and being the only one that can sort it out.
Unfortunatly the other people that do development work don't have the same mindset and I often end up fixing issues that they have created due to a lack of testing (And trust me, I've tried to point out on many occasions that they need to do more (In some cases any) testing before code is deployed, but they never listen as I'm here to fix it when things go wrong).
I'm not trying to say that I'm infallable, write perfect code first time all the time and am some sort of guru on all things SQL related - but it would be nice if there was someone else on the team that was on an equal footing, or better yet, had a greater understanding than me when it comes to coding and could point out mistakes I'm making (Of which I'm sure there are many), give me pointers etc in order to make me better at what I do.
Luckily for me, this place exists. I don't post much but am here at least once a day when I'm at work, reading up on stuff, doing QoTD's etc (In between the firefighting!).
April 24, 2015 at 1:43 am
We have three environments for Dev/Test/QA before anything hits Live.
Another DBA reviews all SQL code by the time it is in QA and it isn't given final approval for a Live release unless the DBAs responsible for the Live servers have also reviewed and approved it. Major/high risk changes have to go before an additional review panel.
Emergencies can bypass this process but then it all still has to be done retrospectively.
As I work for an Internet-based company we just can't risk unplanned downtime - even a few minutes can impact hundreds of customers.
April 24, 2015 at 3:16 am
We have code reviews for all languages including SQL. The SQL (DDL and DML) is included largely because we don't have a DBA and a lot of very poor code was going live (non-normalised tables, stored procedures with cursors and GoTo all over the place, etc.). I'm one of the reviewers and you can see from my various postings that I'm no expert, but something is better than nothing. You see a lot of writing about tensions between developers and DBAs, but we've been begging for a DBA for years!
April 24, 2015 at 3:19 am
We do have some review in place - it's mostly a senior developer going through whatever they think fit on the project. Personally I do review all SQL changes - a quick glance if fairly routine or more in-depth if it affects main process flow say. It's completely informal and down to the situation though.
April 24, 2015 at 3:20 am
We don't use formal code reviews before releasing new code to production but there is a process of change review for the whole business that covers software and database related changes. While this does not actually look at the code we do need to show testing that has been done to prove that the updates work and have not changed anything else. Having said that we often perform informal code reviews as part of the development and testing.
As for urgent changes we have a permit system that allows testing in production and we would usually go this route before formalising the change.
Great topic.
April 24, 2015 at 4:56 am
With SQL code having to get from the Dev and UAT servers before it reaches production I'd like to think its had a fair bit of testing.
As the DBA I wish I had time for proper code reviews but I simply don't. That said, the Development Team Leaders have to sign off the code as well as the end users to say testing has been passed so I've never really had major issues.
I do however check all SQL code for use of cursors, deprecated data types such as Image or Text, Nchar rather than Char, invalid objects etc. I also look at all new table designs and question why everything numeric is INT or they have used datetime not Date or datetime2 and so on.
One thing I have also stamped on is lack of comments in code. No new stored procedure can go live without the author, date and description of what it is doing. The same goes for amended SP's - no comment saying what the change is and who by means it doesn't go live, simple as that.
April 24, 2015 at 5:20 am
I am on a contract with the Compass Group on the Webtrition project. I cannot speak to how other projects handle code reviews but on the Webtrition project all application and database code goes through code review. We use Team Foundation Server to manage source control and also to manage code reviews. Our environments include a Continuous Integration platform for initial check-ins; developers are not allowed to check in code that has not been reviewed. The check-in process has a compilation test at this first step. Other platforms include QA and UA before going to production.
As a whole this is the best system I have seen in over 48 years of being involved with software at some point or another.
April 24, 2015 at 6:11 am
Luckily this has made me get rather OCD about testing, getting users involved and getting feedback from them before I change anything in the real world - it's rare I introduce something that causes issues due to being so paranoid about messing things up and being the only one that can sort it out.
Like Heals and others up yonder I use MMI code review (Me, Myself and I) because that's all I have. Theoretically my boss could do code review, but he recently admitted I'm way better at coding than he for either SQL Server or applications code.
I will also admit that sometimes code review is building things with sufficient de-coupling that I can later go back, look any given part over and say, "I can do that better" without blowing up or re-writing the entire thing.
Nevertheless, I test everything against a copy of production I get by restoring the regular backups to my test machine (thereby testing our backups at the same time) and making sure I have a rollback plan when deploying. I have point-in-time restore capability for production (which I learned attending some SQLSaturday sessions).
Not a perfect state of affairs, but at our scale it works well and sometimes ya' just gotta do what ya' gotta do with what ya' got.
____________
Just my $0.02 from over here in the cheap seats of the peanut gallery - please adjust for inflation and/or your local currency.
April 24, 2015 at 6:46 am
Our environment has a DEV/QA/UAT/PROD setup, and like a lot of the previous individuals that have spoken, we have put in place processes to have code reviews, provide time for DBAs feedback, and tried to increase collaboration between the .Net and SQL sides of development.
All of these things sound good and are what we need, but the downside of any process in place is how/if it can be circumvented. If you have individuals at higher levels that ignore the process when it suits them, it can be very difficult to instill confidence. Additional, changing the culture to buy in to the new process is difficult if there is ambiguity as to why it OK to not follow the process in one situation yet in others it is rigorously followed.
So, my comment is more not about what a process consists of, but instead, does anyone care or respect the process. A great process, if no one follows it, is just the same as no process.
April 24, 2015 at 6:52 am
Jeff Moden (4/23/2015)
The key is that because we do 100% peer reviews and the code runs the gauntlet setup by our wonderful QA team, we've only had 4 such emergencies in the last 4 years and 0 in the last 14 months.
Can you say what happened back in January 2014 :-D?
April 24, 2015 at 6:56 am
We do code reviews. I review all the TSQL and database object creation. Our senior programmer reviews all the code. This is especially so for new employees/consultants.
April 24, 2015 at 6:59 am
Code review. I wish. Some of the developers want & need code reviews, but the management doesn't seem inclined because we are too busy fire-fighting amongst other the untenable deadlines.
They prefer the rock-star and cronyism. So when things do inevitable go wrong its not the "rockstar" fault because no one could foresee the issues.
April 24, 2015 at 7:24 am
No 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."
-------------------------------------------------------------
we travel not to escape life but for life not to escape us
Don't fear failure, fear regret.
Viewing 15 posts - 1 through 15 (of 45 total)
You must be logged in to reply to this topic. Login to reply