August 3, 2018 at 6:43 pm
frederico_fonseca - Friday, August 3, 2018 8:34 AMEric M Russell - Friday, August 3, 2018 8:25 AMEven better, you can enforce your T-SQL formatting standard using SQL Server's policy-based management. Don't even let the developer save their procedures and views until they're properly formatted. :Whistling:Got examples of this? Would be interested as not aware it was possible.
I'll second that request...
--Jeff Moden
August 4, 2018 at 1:47 am
frederico_fonseca - Friday, August 3, 2018 8:34 AMEric M Russell - Friday, August 3, 2018 8:25 AMEven better, you can enforce your T-SQL formatting standard using SQL Server's policy-based management. Don't even let the developer save their procedures and views until they're properly formatted. :Whistling:Got examples of this? Would be interested as not aware it was possible.
+1, I need this like yesterday..
...
August 6, 2018 at 11:01 am
Jeff Moden - Friday, August 3, 2018 6:43 PMfrederico_fonseca - Friday, August 3, 2018 8:34 AMEric M Russell - Friday, August 3, 2018 8:25 AMEven better, you can enforce your T-SQL formatting standard using SQL Server's policy-based management. Don't even let the developer save their procedures and views until they're properly formatted. :Whistling:Got examples of this? Would be interested as not aware it was possible.
I'll second that request...
Don't get all excited just yet. I'm just talking theoretical here.
"Do not seek to follow in the footsteps of the wise. Instead, seek what they sought." - Matsuo Basho
August 6, 2018 at 11:25 am
Eric M Russell - Monday, August 6, 2018 11:01 AMJeff Moden - Friday, August 3, 2018 6:43 PMfrederico_fonseca - Friday, August 3, 2018 8:34 AMEric M Russell - Friday, August 3, 2018 8:25 AMEven better, you can enforce your T-SQL formatting standard using SQL Server's policy-based management. Don't even let the developer save their procedures and views until they're properly formatted. :Whistling:Got examples of this? Would be interested as not aware it was possible.
I'll second that request...
Don't get all excited just yet. I'm just talking theoretical here.
Since this was just wishful thinking, let me suggest something different.
Don't let the developers deploy any changes to SQL code without a proper code review. And don't review the code if it's not properly formatted. The code review can be enforced before pushing changes to the repository or releasing the code. It all depends on what tool you want to use to enforce it (and the developers might already be using it).
August 7, 2018 at 12:50 am
I also use ApexSQL as it is a free tool and although not all the changes are the formatting I'd like, as someone else mentioned, consistency is best. Additionally, as someone else already mentioned, being able to formatting inherited code with a keystroke can save hours of work. I do endeavour to write the code correctly in the first instance and use the hotkey towards the end and it usually only picks up one or two things I have missed.
August 7, 2018 at 6:14 am
+1 for SQL prompt. Also T-sql Cop which picks up a whole bunch of other code rules apart from just formatting.
August 7, 2018 at 6:29 am
Luis Cazares - Monday, August 6, 2018 11:25 AMEric M Russell - Monday, August 6, 2018 11:01 AMJeff Moden - Friday, August 3, 2018 6:43 PMfrederico_fonseca - Friday, August 3, 2018 8:34 AMEric M Russell - Friday, August 3, 2018 8:25 AMEven better, you can enforce your T-SQL formatting standard using SQL Server's policy-based management. Don't even let the developer save their procedures and views until they're properly formatted. :Whistling:Got examples of this? Would be interested as not aware it was possible.
I'll second that request...
Don't get all excited just yet. I'm just talking theoretical here.
Since this was just wishful thinking, let me suggest something different.
Don't let the developers deploy any changes to SQL code without a proper code review. And don't review the code if it's not properly formatted. The code review can be enforced before pushing changes to the repository or releasing the code. It all depends on what tool you want to use to enforce it (and the developers might already be using it).
I can't +1 that enough. Rigorous code reviews with a "no, go do it again properly" attitude are essential to stopping the rot getting in. I'll have to try ApexSQL at some point, I've tried SQLPrompt but find it unbearably laggy and it's highlighting to be even more spurious than SSMS.
I'll admit to being a dinosaur who doesn't like automatic reformatting of code in general - if only because it makes working out what the changes really were in SQLCompare a nightmare. If what's there isn't a bug and doesn't need changing, I'd rather leave well alone.
August 7, 2018 at 7:34 am
andycadley - Tuesday, August 7, 2018 6:29 AMLuis Cazares - Monday, August 6, 2018 11:25 AMEric M Russell - Monday, August 6, 2018 11:01 AMJeff Moden - Friday, August 3, 2018 6:43 PMfrederico_fonseca - Friday, August 3, 2018 8:34 AMEric M Russell - Friday, August 3, 2018 8:25 AMEven better, you can enforce your T-SQL formatting standard using SQL Server's policy-based management. Don't even let the developer save their procedures and views until they're properly formatted. :Whistling:Got examples of this? Would be interested as not aware it was possible.
I'll second that request...
Don't get all excited just yet. I'm just talking theoretical here.
Since this was just wishful thinking, let me suggest something different.
Don't let the developers deploy any changes to SQL code without a proper code review. And don't review the code if it's not properly formatted. The code review can be enforced before pushing changes to the repository or releasing the code. It all depends on what tool you want to use to enforce it (and the developers might already be using it).I can't +1 that enough. Rigorous code reviews with a "no, go do it again properly" attitude are essential to stopping the rot getting in. I'll have to try ApexSQL at some point, I've tried SQLPrompt but find it unbearably laggy and it's highlighting to be even more spurious than SSMS.
I'll admit to being a dinosaur who doesn't like automatic reformatting of code in general - if only because it makes working out what the changes really were in SQLCompare a nightmare. If what's there isn't a bug and doesn't need changing, I'd rather leave well alone.
+1 MILLION to that. Not only properly formatted but properly commented with standard flower boxes, etc. At one company that I worked at, peer reviews to meet written standards, mentoring during such peer reviews, and the "Do it right" pride that Developers actually do take upon themselves when in the right kind of environment, all helped to bring defects found by QA to near zero and made the time to analyze modifications in necessarily complex stored procedures drop from an average of 2 days to less than an hour or 2. There was, obviously, a transition period that people went through and, initially, it took a little more time but, OMG, the time we saved in rework/retest was incredible. The mentoring also gave people some extra knowledge on tips'n'tricks for writing good, fast code more quickly and that increased actual through put of code not to mention performance of the code kept improving almost auto-magically.
As for automatic formatting, I agree with Andy but for additional reasons. Unless the code is an absolute train wreck (which does need to be fixed), then I find that writing the code correctly manually forces people to pay attention to the code they're writing and I've found that they find bugs in their own (or someone else's code) in the process. Yes, it takes a little extra time up front at first but people get really good at it with a little practice and the downstream benefits of code reviews, and the related mentoring, written standards, and developing both the "Do it right" and "Don't kill the messenger" culture has huge payoffs down stream.
The trouble is getting managers to realize the benefit and to stop making schedule promises that can't actually be met. If you don't have manager buy-in, then it's a real bitch to get it all to work. Ironically, getting it all to work will make the manager shine in all ways because the reduction in test time and rework will frequently make it so that unreasonable schedules actually can be met. 😉
--Jeff Moden
August 7, 2018 at 12:46 pm
If you work in an organization where a DBA has an the opportunity, time, and political clout to perform manual code reviews, then good for you. However, the good thing about policy based code checking is that it would remove the DBA from the code review process, except for the upfront implementation of the policy, so it eliminates time wasting and political charged confrontations between the developer and DBA team.
"Do not seek to follow in the footsteps of the wise. Instead, seek what they sought." - Matsuo Basho
August 7, 2018 at 1:19 pm
Eric M Russell - Tuesday, August 7, 2018 12:46 PMIf you work in an organization where a DBA has an the opportunity, time, and political clout to perform manual code reviews, then good for you. However, the good thing about policy based code checking is that it would remove the DBA from the code review process, except for the upfront implementation of the policy, so it eliminates time wasting and political charged confrontations between the developer and DBA team.
Would like to see the PBM being used, but how does that help when the code could possibly pass the PBM checks yet be poorly written code that could be caught during a formal code review?
August 7, 2018 at 9:08 pm
Eric M Russell - Tuesday, August 7, 2018 12:46 PMIf you work in an organization where a DBA has an the opportunity, time, and political clout to perform manual code reviews, then good for you. However, the good thing about policy based code checking is that it would remove the DBA from the code review process, except for the upfront implementation of the policy, so it eliminates time wasting and political charged confrontations between the developer and DBA team.
That might be true for the simplicities of formatting, required headers, illegal words, naming conventions, and maybe even the occasional bit of non-SARGgable code. I haven't found anything yet that will check the logic of the stored procedure as it pertains to the requirements nor do anything much with or about performance issues nor is there anything possible that will explain the "WHY" of each query in the form of comments. Heh... if there were such a thing, you wouldn't need humans to write the code to begin with.
--Jeff Moden
August 8, 2018 at 6:23 am
Jeff Moden - Tuesday, August 7, 2018 9:08 PMEric M Russell - Tuesday, August 7, 2018 12:46 PMIf you work in an organization where a DBA has an the opportunity, time, and political clout to perform manual code reviews, then good for you. However, the good thing about policy based code checking is that it would remove the DBA from the code review process, except for the upfront implementation of the policy, so it eliminates time wasting and political charged confrontations between the developer and DBA team.That might be true for the simplicities of formatting, required headers, illegal words, naming conventions, and maybe even the occasional bit of non-SARGgable code. I haven't found anything yet that will check the logic of the stored procedure as it pertains to the requirements nor do anything much with or about performance issues nor is there anything possible that will explain the "WHY" of each query in the form of comments. Heh... if there were such a thing, you wouldn't need humans to write the code to begin with.
I agree with both of you!
Thing is, code review of all code is frequently impossible. So, lots and lots of automation on the stuff we can automate so that we then have the opportunity to focus on the stuff we cannot automate. You're both right!
----------------------------------------------------The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood... Theodore RooseveltThe Scary DBAAuthor of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd EditionProduct Evangelist for Red Gate Software
August 8, 2018 at 7:12 am
Code reviews are great, but depending on the volume and velocity of development, it's usually not possible. The larger the organization, the more developers and applications, and the size of the DBA team doesn't grow proportionally to the number of developers. Also, in a Continuous Integration environment, the DBA may not even have a chance to see the code before it's deployed. Having the DBA manually sniff each stored procedure and provide feedback may not be perceived by management as adding value to what is otherwise an automated and efficient delivery process.
Whenever I create or modify a DDL script, I add a single line comment to the header which includes the date, JIRA #, name, and a line line description of the purpose of the procedure or why the change was made. For more detail, the JIRA # can be used for reference.
"2018/08/08 DBA-3421 Eric Russell - This is a sample comment ..."
If you can at least get all the developers to conform to a specification when commenting, then it could be parsed and validated, including checks to confirm that the date, ticket #, and staff member's name all line up. Of course, whether the comments actually describe the code modifications in a useful way is a different issue. I don't believe that SQL Server's PBM would could do this or that it would be the best place to do it, but it could be incorporated into the CI deployment workflow, if you're using a CI stack like Atlassian and Octopus.
As for validating that the T-SQL code is well written in terms of optimization, one thing that you could do is execute it within a test environment and then parse the resulting XML execution plan, looking for things like estimated costs, table scans, and sorts. Again, this could be automated and rules driven by the CI process.
"Do not seek to follow in the footsteps of the wise. Instead, seek what they sought." - Matsuo Basho
August 8, 2018 at 7:34 am
Eric M Russell - Wednesday, August 8, 2018 7:12 AMCode reviews are great, but depending on the volume and velocity of development, it's usually not possible. The larger the organization, the more developers and applications, and the size of the DBA team doesn't grow proportionally to the number of developers. Also, in a Continuous Integration environment, the DBA may not even have a chance to see the code before it's deployed. Having the DBA manually sniff each stored procedure and provide feedback may not be perceived by management as adding value to what is otherwise an automated and efficient delivery process.Whenever I create or modify a DDL script, I add a single line comment to the header which includes the date, JIRA #, name, and a line line description of the purpose of the procedure or why the change was made. For more detail, the JIRA # can be used for reference.
"2018/08/08 DBA-3421 Eric Russell - This is a sample comment ..."
If you can at least get all the developers to conform to a specification when commenting, then it could be parsed and validated, including checks to confirm that the date, ticket #, and staff member's name all line up. Of course, whether the comments actually describe the code modifications in a useful way is a different issue. I don't believe that SQL Server's PBM would could do this or that it would be the best place to do it, but it could be incorporated into the CI deployment workflow, if you're using a CI stack like Atlassian and Octopus.As for validating that the T-SQL code is well written in terms of optimization, one thing that you could do is execute it within a test environment and then parse the resulting XML execution plan, looking for things like estimated costs, table scans, and sorts. Again, this could be automated and rules driven by the CI process.
Do you know anyone that has actually accomplished all of that? Not a challenge... just curious.
I'll admit that I work in a rather unique environment. Most of the front-end Developers are better SQL programmers than a lot of people that hold the title of "Database Developer" thanks to the in-house training we've done through peer reviews, the occasional "Lunch'n'Learn" sessions, the occasional "SQL Tip" email, things that we've published on our internal WIKI, and being taught how to search for BOL related information. They also take a sense of pride when it comes to performance of code. They do their own performance testing and at least have a clue as to what to look for in Actual Execution Plans. They're also good at setting up test data (they have several scripts in that area).
AND, they know how to comment code and follow certain standards that we've documented! 😀
I told you all of that because that's why I'm able to crank through peer reviews pretty quickly nowadays. I do understand that not all shops have such a luxury but not doing anything in these areas is why a lot of shops have so much rework to do.
--Jeff Moden
August 8, 2018 at 7:37 am
Eric M Russell - Wednesday, August 8, 2018 7:12 AMCode reviews are great, but depending on the volume and velocity of development, it's usually not possible. The larger the organization, the more developers and applications, and the size of the DBA team doesn't grow proportionally to the number of developers. Also, in a Continuous Integration environment, the DBA may not even have a chance to see the code before it's deployed. Having the DBA manually sniff each stored procedure and provide feedback may not be perceived by management as adding value to what is otherwise an automated and efficient delivery process.Whenever I create or modify a DDL script, I add a single line comment to the header which includes the date, JIRA #, name, and a line line description of the purpose of the procedure or why the change was made. For more detail, the JIRA # can be used for reference.
"2018/08/08 DBA-3421 Eric Russell - This is a sample comment ..."
If you can at least get all the developers to conform to a specification when commenting, then it could be parsed and validated, including checks to confirm that the date, ticket #, and staff member's name all line up. Of course, whether the comments actually describe the code modifications in a useful way is a different issue. I don't believe that SQL Server's PBM would could do this or that it would be the best place to do it, but it could be incorporated into the CI deployment workflow, if you're using a CI stack like Atlassian and Octopus.As for validating that the T-SQL code is well written in terms of optimization, one thing that you could do is execute it within a test environment and then parse the resulting XML execution plan, looking for things like estimated costs, table scans, and sorts. Again, this could be automated and rules driven by the CI process.
I disagree with the first sentence (bolded for emphasis by me) you wrote, but I agree with most of what you said. Code reviews should always be possible and should be part of the process, not something that people do if time permits. It doesn't need to be a DBA who reviews every single piece of code, but you still need people reviewing the code. You need a policy with at least the most basic requirements of good quality code, you can use a tool to review just the changes made to the objects (SQL Compare, TortoiseGit, etc).
Code reviews are not meant to validate the logic of the procedure, just to prevent explicit problems with the code itself. That's why a code policy is very useful for code reviews.
Viewing 15 posts - 16 through 30 (of 47 total)
You must be logged in to reply to this topic. Login to reply