September 7, 2016 at 1:31 pm
Luis Cazares (9/7/2016)
Jason A. Long (9/7/2016)
Speaking for all the jerk code reviewers of the world... Yes, formatting counts, yes I'll give you crap over it (when you have to read through the code from 10 different sql devs nd they're all using different formatting styles it gets painful) but I won't reject your code over it. There are only two things that I'd bust you for on this...#1... ORDER BY ordinal position rather than column name (I'd reject for that).
#2... Using column aliases with spaces. (I be inclined to reject unless there is some odd-nut reason that justifies it)
As to the ON clause formatting... I'm with Drew... Separate line and indented. 😉
And that's why there should be company standards.
I wouldn't format the code as it was originally published, but I wouldn't reject it either. However, if it wasn't formatted at all, I would have rejected it.
It's not terribly bad... Thankfully we all have SQL Prompt installed and all of our formatting templates are fairly similar... If I don't like someone formatting SQL Prompt will usually get me close enough to be able to read the code.
Without SQL Prompt... I'd either slit my wrists or find another job that doesn't require me to look at other peoples code... That said, it only gets you "close enough". I keep hoping for more granularity as to how things are formatted under various conditions (for example, I format my CASE expressions differently if there is only 1 WHEN condition).
September 7, 2016 at 1:33 pm
Phil Parkin (9/7/2016)
And that's where a shared set of SQL Prompt formatting options really works.
One thing missing from SQL Prompt, however, is the ability to change between 'my' settings (whatever works for the individual) and 'company' settings (the common format). I'd love that, because I'm one of the few lower-case-keywords developers 🙂
Now THAT would be an awesome option!
September 7, 2016 at 1:51 pm
mister.magoo (9/7/2016)
Phil Parkin (9/7/2016)
One thing missing from SQL Prompt, however, is the ability to change between 'my' settings (whatever works for the individual) and 'company' settings (the common format). I'd love that, because I'm one of the few lower-case-keywords developers 🙂Another lower-caser here...and if you can use it, SSMSBoost has formatting profiles - so you could have a personal one in there, then use SQL Prompt for company styling...
I could use it ... and I used to use it ... and I will again. But for now, there is no version for SSMS 2016, so I'm in limbo. I really miss the definable collapsible regions and fast connection switching.
The absence of evidence is not evidence of absence
- Martin Rees
The absence of consumable DDL, sample data and desired results is, however, evidence of the absence of my response
- Phil Parkin
September 8, 2016 at 5:15 am
drew.allen (9/7/2016)
I always put my ON clauses on a separate indented line, because I want to keep it consistent. I treat short and long JOIN clauses the same, if I'm going to insert a new line before the ON clause for one, I'm going to do it for both. I indent it for two reasons: it helps to indicate that it's part of the JOIN clause, and it helps to break up the JOIN clauses.Drew
That doesn't make sense Drew 😎 "Consistency" is keeping to the same format, not using a particular format. Always putting them on the same line is just as consistent as putting them on a different line.
This is a bit of a contentious issue, but I go for putting the first 'ON' condition on the same line with extra (and, or) conditions on new lines aligned with their siblings. That way information of the same type is kept together (table aliases, conditions, condition types) and the query can be scanned quickly for scope and effect. Unfortunately there's no fixed width font here, so I can't give an example.
In my experience, putting the 'ON' clause on a new line is more of an 'old school' habit going back to the days when you could only fit 80 characters on each line :hehe:
September 8, 2016 at 5:41 am
david.wright-948385 (9/8/2016)
drew.allen (9/7/2016)
I always put my ON clauses on a separate indented line, because I want to keep it consistent. I treat short and long JOIN clauses the same, if I'm going to insert a new line before the ON clause for one, I'm going to do it for both. I indent it for two reasons: it helps to indicate that it's part of the JOIN clause, and it helps to break up the JOIN clauses.Drew
That doesn't make sense Drew 😎 "Consistency" is keeping to the same format, not using a particular format. Always putting them on the same line is just as consistent as putting them on a different line.
This is a bit of a contentious issue, but I go for putting the first 'ON' condition on the same line with extra (and, or) conditions on new lines aligned with their siblings. That way information of the same type is kept together (table aliases, conditions, condition types) and the query can be scanned quickly for scope and effect. Unfortunately there's no fixed width font here, so I can't give an example.
In my experience, putting the 'ON' clause on a new line is more of an 'old school' habit going back to the days when you could only fit 80 characters on each line :hehe:
I disagree. It makes perfect sense. He *always puts ON clauses on separate indented lines* – perfectly consistent.
The absence of evidence is not evidence of absence
- Martin Rees
The absence of consumable DDL, sample data and desired results is, however, evidence of the absence of my response
- Phil Parkin
September 8, 2016 at 5:50 am
Phil Parkin (9/8/2016)
I disagree. It makes perfect sense. He *always puts ON clauses on separate indented lines* – perfectly consistent.
That's the point I'm making - it's consistent, but no more consistent than any other approach. Putting the 'ON' clause on the same line is also consistent if used throughout a system, but my contention is that it's better - for the reasons stated.
September 8, 2016 at 6:10 am
david.wright-948385 (9/8/2016)
Phil Parkin (9/8/2016)
I disagree. It makes perfect sense. He *always puts ON clauses on separate indented lines* – perfectly consistent.That's the point I'm making - it's consistent, but no more consistent than any other approach. Putting the 'ON' clause on the same line is also consistent if used throughout a system, but my contention is that it's better - for the reasons stated.
Then we'll have to agree to disagree.
In my opinion, Drew's approach is more consistent than putting short ON clauses on one line and longer ON clauses on two (because 'longer' is a qualitative measure, which is likely to differ between one developer and another).
The absence of evidence is not evidence of absence
- Martin Rees
The absence of consumable DDL, sample data and desired results is, however, evidence of the absence of my response
- Phil Parkin
September 8, 2016 at 6:59 am
Phil Parkin (9/8/2016)
In my opinion, Drew's approach is more consistent than putting short ON clauses on one line and longer ON clauses on two (because 'longer' is a qualitative measure, which is likely to differ between one developer and another).
I don't think we're far apart. My approach is that the first (and usually the only) 'ON' comparison should go on the same line. If it goes on the next line, but is indented clear of the table name and alias, then I have no problem with that. But if it is under the table reference and alias, then when scanning the code from top to bottom, different types of information interfere with each other. With lateral separation of the 'ON' clause, the table names and aliases for example will be in line with each other, with nothing between them to distract.
I can't see how Drew's approach is more consistent though. Consistent with what? Maybe we understand the word 'consistent' differently? From the dictionary, "consistent" [kuh n-sis-tuh nt] is adjective meaning "constantly adhering to the same principles, course, form, etc." Thus for example you can be consistently happy, consistently unreliable, or even consistently wrong 😛 So consistency is vital in applying standards, but in and of itself is not sufficient reasoning to justify the standards themselves, because consistency is only as good as what it's consistent with. For instance planners across the country might apply a regulation that allows dog latrines to be build in the middle of children's playgrounds. The policy is consistent because everyone applies it, but I for one would argue whether it's a good idea!
September 8, 2016 at 9:39 am
david.wright-948385 (9/8/2016)
Phil Parkin (9/8/2016)
I disagree. It makes perfect sense. He *always puts ON clauses on separate indented lines* – perfectly consistent.That's the point I'm making - it's consistent, but no more consistent than any other approach. Putting the 'ON' clause on the same line is also consistent if used throughout a system, but my contention is that it's better - for the reasons stated.
I like to keep my lines short, and it's not just because I'm old school. (Yes, I started programming on punch cards.) I have degrees in computer science and linguistics, and I've studied typography, because it overlaps both of those interests. Long lines are simply harder to read. Lines where everything is lined up are also harder to read.
Putting ON clauses on separate lines necessarily keeps the lines shorter. Indenting the ON clauses introduces variety in the alignment. Combining the two makes it much easier to read.
And you're not being consistent. You're putting additional criteria (and/or) clauses on a new line, which is inconsistent with keeping the join criteria on the same line as your join. I also put additional criteria on a new line, but that is consistent with my approach of putting the criteria on a new line.
Drew
J. Drew Allen
Business Intelligence Analyst
Philadelphia, PA
September 8, 2016 at 9:55 am
Also, my approach can easily be used with derived tables, again being consistent. You're approach could quickly become cumbersome with derived tables.
Drew
J. Drew Allen
Business Intelligence Analyst
Philadelphia, PA
September 8, 2016 at 10:07 am
drew.allen (9/8/2016)
Also, my approach can easily be used with derived tables, again being consistent. You're approach could quickly become cumbersome with derived tables.Drew
How?
September 8, 2016 at 11:40 am
For example the following from my days in fundraising. (NOTE, this is probably not the way I would approach this, but it illustrates the point.)
SELECT *
FROM Constituent c
INNER JOIN (
SELECT const_id, YEAR(gift_dt) AS gift_year, MONTH(gift_dt) AS gift_month, SUM(gift_amount) AS gift_amount)
FROM gifts g
GROUP BY g.const_id, YEAR(gift_dt), MONTH(gift_dt)
) AS g
ON c.const_id = g.const_id-- ON on a new line
-- ON on the same line as the join -- also requires the entire derived table being on one line.
SELECT *
FROM Constituent c
INNER JOIN ( SELECT const_id, YEAR(gift_dt) AS gift_year, MONTH(gift_dt) AS gift_month, SUM(gift_amount) AS gift_amount) FROM gifts g GROUP BY g.const_id, YEAR(gift_dt), MONTH(gift_dt) ) AS g ON c.const_id = g.const_id
In order for you to get the ON clause on the same line as the JOIN (being consistent), you need to put the entire derived table on a single line, because it comes between the JOIN clause and the ON clause.
Drew
J. Drew Allen
Business Intelligence Analyst
Philadelphia, PA
September 8, 2016 at 11:52 am
You run into a similar problem when you want to evaluate the ON clauses in an order different than the JOIN clauses. For instance, with the following query where the ON for the INNER JOIN needs to be evaluated before the ON for the LEFT OUTER JOIN.
-- ON on a new line
SELECT *
FROM Employees e
LEFT OUTER JOIN Phones p
INNER JOIN Phone_Types pt
ON p.phone_type_id = pt.phone_type_id
ON e.phone_id = p.phone_id
AND pt.phone_type_cd = 'Mobile'
-- ON on the same line as the JOIN
SELECT *
FROM Employees e
LEFT OUTER JOIN Phones p INNER JOIN Phone_Types pt ON p.phone_type_id = pt.phone_type_id ON e.phone_id = p.phone_id
AND pt.phone_type_cd = 'Mobile'
Drew
J. Drew Allen
Business Intelligence Analyst
Philadelphia, PA
September 13, 2016 at 4:24 pm
You make a very good point Drew. I dont think strict rules can override common sense in accounting for every situation. Expect exceptions to the rules. A mantra I see readily repeated on the forum here is "It depends" and for good reason.
On the note of not having spaces in the column name; This ultimately resides with the customer receiving the data on how they want the columns presented however. That is why we have the brackets[], you can put a select statement in there and it is treated as text (I dont like the use of single quotes for alias names). For column names when defining a table, I do agree with eliminating the spaces.
----------------------------------------------------
September 14, 2016 at 3:25 pm
Back to the original post, that code looks far better than most of what I see as a DBA!
Just one serious change, to get rid of the CAST of/function against a column in the WHERE clause:
WHERE a.ACT_DATE >= @checkDate OR a.ACT_CLOSE IS NULL
I would also make a few other minor changes:
SET @checkDate = DATEADD(day, DATEDIFF(day, 0, GETDATE()) - 1, 0)
rather than:
SET @checkDate = DATEADD(day, DATEDIFF(day, 1, GETDATE()), 0)
because I think the latter method is more convoluted.
I'd also change "JOIN" to "INNER JOIN". No difference you say? But you can't add query hints, such as "INNER HASH JOIN", without the INNER key word ... sometimes us DBAs have to consider doing that for query performance.
As to the spaces in column names, I don't mind them nearly as much in the final query, if they make the results clearer to the end receiver of the data. But they should be avoided in all intermediate steps/queries.
SQL DBA,SQL Server MVP(07, 08, 09) "It's a dog-eat-dog world, and I'm wearing Milk-Bone underwear." "Norm", on "Cheers". Also from "Cheers", from "Carla": "You need to know 3 things about Tortelli men: Tortelli men draw women like flies; Tortelli men treat women like flies; Tortelli men's brains are in their flies".
Viewing 15 posts - 16 through 30 (of 44 total)
You must be logged in to reply to this topic. Login to reply