Please start any new threads on our new site at https://forums.sqlteam.com. We've got lots of great SQL Server experts to answer whatever question you can come up with.

 All Forums
 SQL Server 2005 Forums
 Transact-SQL (2005)
 The case for and against "pretty" code!

Author  Topic 

topdev
Starting Member

1 Post

Posted - 2010-06-15 : 01:27:45
I've been working with a great dev at my job but we have a disagreement over his stored procedure. Essentially, his code is doing tons of joins, plus the code is (at least IMO) really hard to read. There is a lot of redundant code that is used (in the form of joins) in order to determine what certain IDs are. Basically, he's stacked the query with one set of joins on top of another. Also, the way that the joins and on conditions are specified are not "standard". Typically you have code like this:

SELECT T1.Data1
FROM Table1 T1
INNER JOIN Table2 T2 ON T1.Key=T2.Key
INNER JOIN Table3 T3 ON T2.Key=T3.Key

His format is quite different and honestly, after working with SQL Server for 15 years I've never seen anything quite like it. Given the query above, his approach would be like this:

SELECT T1.Data1
FROM Table1 T1
INNER JOIN Table2
INNER JOIN Table3
ON T1.Key=T2.Key
ON T3.Key=T2.Key

Now, this is just a simple query. The actual query is much knarlier. His reason for using this style is because it makes the code "prettier" and "easier to indent". My arguments against this have to do with maintainability and readability and the ability (or lack of, in the case) of breaking the code out into smaller units of work; no to mention performance. For instance when the joins and the ons are closer together you can usually highlight a section of your query and unit-test it, but you can't with him style because the on conditions are usually at the end of the query.

Anyway, have a look at this query and let me know what you think. Am I being too pickly or missing a clever nuance? Keep in mind, this is not the entire query, it's only about 30% of the join section!

LEFT OUTER JOIN [XFS_ORel] obr_email
INNER JOIN [XFS_OTypeRel] obrt_email
ON obr_email.[OTypeRel_Id] = obrt_email.[OTypeRel_Id]
AND obrt_email.[OTypeRel_Name] = 'D2PC'
INNER JOIN [XFS_Object] obj_email
INNER JOIN [XFS_SourceType] st_email
ON obj_email.[SrcType_Id] = st_email.[SrcType_Id]
AND st_email.[SrcType_Location] = 'Email'


INNER JOIN [XFS_AttrLong] a_email
INNER JOIN [XFS_AttrType] at_email
INNER JOIN [XFS_AttrCollection] ac_email
ON at_email.[AttrColl_Id] = ac_email.[AttrColl_Id]
AND ac_email.[AttrColl_Name] = 'BPC'
ON a_email.[AttrType_Id] = at_email.[AttrType_Id]
AND at_email.[AttrType_Name] = 'Name'
ON obj_email.[Obj_Id] = a_email.[Obj_Id]
AND a_email.[AttrValueNLongString] = 'FROM'

INNER JOIN [XFS_ORel] obr_sender
INNER JOIN [XFS_OTypeRel] obrt_sender
ON obr_sender.[OTypeRel_Id] = obrt_sender.[OTypeRel_Id]
AND obrt_sender.[OTypeRel_Name] = 'PC2P'
INNER JOIN [XFS_Object] obj_sender
INNER JOIN [XFS_SourceType] st_sender
ON obj_sender.[SrcType_Id] = st_sender.[SrcType_Id]
AND st_sender.[SrcType_Location] = 'Email'
ON obr_sender.[Child_Obj_Id] = obj_sender.[Obj_Id]

INNER JOIN [XFS_AttrLong] a_sender
INNER JOIN [XFS_AttrType] at_sender
INNER JOIN [XFS_AttrCollection] ac_sender
ON at_sender.[AttrColl_Id] = ac_sender.[AttrColl_Id]
AND ac_sender.[AttrColl_Name] = 'BP'
ON a_sender.[AttrType_Id] = at_sender.[AttrType_Id]
AND at_sender.[AttrType_Name] = 'Name'
ON obj_sender.[Obj_Id] = a_sender.[Obj_Id]

ON obj_email.[Obj_Id] = obr_sender.[Parent_Obj_Id]
ON obr_email.[Child_Obj_Id] = obj_email.[Obj_Id]
ON obj_doc.[Obj_Id] = obr_email.[Parent_Obj_Id]

AndrewMurphy
Master Smack Fu Yak Hacker

2916 Posts

Posted - 2010-06-15 : 03:05:41
1. extra joins won't help performance - not used = not needed = remove.
2. when it comes to code "pretty ain't everything", "readable & understandable by the next f*****" is.
3. beauty is in the eye of the beholder.
4. what happens to the coding style when it comes to left joins and conditional items? when he has to use () or case statements?
5. small is very often best in coding terms.
6. win the war, not a small battle. does his coding style matter except at extreme cases?
performance is proof. produce evidence of improvements by using alternatives.
I can only see the number of joins being an issue. the compiled execution plan should be the same regardless of how pretty it looks.
Go to Top of Page

Lumbago
Norsk Yak Master

3271 Posts

Posted - 2010-06-15 : 03:41:13
This is discussion can be almost infinite as everyone has a different opinion of what serves the better pupose. My opinion is that code should be written so that it's easier to read and understand for people that didn't write it and for when you need to review a piece of code that you haven't touched for a long time.

Personally I don't have anything positive whatsoever to say about your colleagues standard. It looks exactly like the old-fashioned "FROM table1, table2, table3 WHERE table1.ID = table2.id and table2.otherid = table3.id" and I don't like it at all.

I have made sort of a coding standard that works great for me:
SELECT 
Col1,
Col2
FROM table1 a
INNER JOIN table2 b
ON a.id = b.id
AND a.otherid = b.otherid
INNER JOIN table3 c
ON b.something = c.something
WHERE
a.Col1 = '15'
AND b.ColX = 'Lumbago'
It doesn't look fantastic but I find it very easy to read and debug.

- Lumbago

My blog (yes, I have a blog now! just not that much content yet)
-> www.thefirstsql.com
Go to Top of Page

Arnold Fribble
Yak-finder General

1961 Posts

Posted - 2010-06-15 : 04:29:56
quote:
Originally posted by topdev

his approach would be like this:

SELECT T1.Data1
FROM Table1 T1
INNER JOIN Table2
INNER JOIN Table3
ON T1.Key=T2.Key
ON T3.Key=T2.Key



I think you've just illustrated what's so tricky about using nested joins without parentheses! As stated, this query won't parse because T1 isn't in scope for the first (innermost) ON clause.
Go to Top of Page

Kristen
Test

22859 Posts

Posted - 2010-06-15 : 13:32:39
I don't have a problem with the nested JOINs - it would have helped if you had used [CODE] tags to preserve the formatting, but your original indentation is visible by pressing REPLY to your post.

Having said that I don't do it that way, and Yeah! wars have been fought over less!

...
FROM TableA
INNER JOIN
(
TableB
INNER JOIN TableB_Child1
ON B1_Col = B_Col
)
ON B_Col = A_Col

has the benefit that INNER JOIN can easily be changed to an Outer Join whilst preserving the localised hierarchy relationship of TableB and TableB_Child1

Dunno how often that happens in real life though!

FWIW I definitely prefer

SELECT T1.Data1
FROM Table1 AS T1
INNER JOIN Table2 AS T2
ON T2.Key=T1.Key
INNER JOIN Table3 AS T3
ON T3.Key=T2.Key

to

SELECT T1.Data1
FROM Table1 T1
INNER JOIN Table2 T2 ON T1.Key=T2.Key
INNER JOIN Table3 T3 ON T2.Key=T3.Key

note that I have added "AS" to reduce the risk of an accidental space separating two words, and turned round your ON so that the Target columns are on the left. Mostly JOIN'd tables are joined on their PK, so this provides easy checking, on left hand side, that all PK columns have been satisfied. Also where there are multiple key columns it stops the criticality wandering off the right side of the screen, and makes tests against static values obvious:

SELECT T1.Data1
FROM Table1 AS T1
JOIN Table2 AS T2
ON T2.PkKey1 = T1.Col1
AND T2.PkKey2 = T1.ColN
AND T2.IsActive = 1

We don't bother with INNER but we do use OUTER in a LEFT join - to make it more obvious that it is "different"

Our style has evolved to fight bugs that creep in during code maintenance, and suits us - in the sense of helping DEVs spot goofy things more often, and generate fewer bugs

For example, we do

SELECT [TheName] = SomeColumnOrExpression

rather than

SELECT SomeColumnOrExpression AS [TheName]

because expressions are variable width and push [TheName] to the right and make it less easy to spot and associate with the column.

Same with

WHERE
...
Col1 = Col2 AND
Col3 = Col4 OR
...

and

WHERE
...
Col1 = Col2
AND Col3 = Col4
OR ...
...

we do the second for the same reason (yeah, I know, that example needs some parenthesis! but it highlights the fact that it makes that issue more likely to be spotted than if it is off the right end of the screen)

We don't do

IF ( ... lots of stuff ) {
...
}

either, we line the brackets up so we can see what goes with what

IF ( ... lots of stuff )
{
...
}

similarly with BEGIN / END in SQL

But ask two programmers about coding style and you'll get at least three answers !
Go to Top of Page

Michael Valentine Jones
Yak DBA Kernel (pronounced Colonel)

7020 Posts

Posted - 2010-06-15 : 14:45:18
I do find that coding style hard to read and understand, because it is so unusual.

However, the biggest concern that I have is that the database appears to be using an EAV (Entity, Attribute, Value) model. The disadvantages of that are so numerous that I won't even bother getting into it here, but let's just leave it at saying that it's a worst of the worst practice.


LEFT OUTER JOIN [XFS_ORel] obr_email
INNER JOIN [XFS_OTypeRel] obrt_email
ON obr_email.[OTypeRel_Id] = obrt_email.[OTypeRel_Id]
AND obrt_email.[OTypeRel_Name] = 'D2PC'
INNER JOIN [XFS_Object] obj_email
INNER JOIN [XFS_SourceType] st_email
ON obj_email.[SrcType_Id] = st_email.[SrcType_Id]
AND st_email.[SrcType_Location] = 'Email'


INNER JOIN [XFS_AttrLong] a_email
INNER JOIN [XFS_AttrType] at_email
INNER JOIN [XFS_AttrCollection] ac_email
ON at_email.[AttrColl_Id] = ac_email.[AttrColl_Id]
AND ac_email.[AttrColl_Name] = 'BPC'
ON a_email.[AttrType_Id] = at_email.[AttrType_Id]
AND at_email.[AttrType_Name] = 'Name'
ON obj_email.[Obj_Id] = a_email.[Obj_Id]
AND a_email.[AttrValueNLongString] = 'FROM'

INNER JOIN [XFS_ORel] obr_sender
INNER JOIN [XFS_OTypeRel] obrt_sender
ON obr_sender.[OTypeRel_Id] = obrt_sender.[OTypeRel_Id]
AND obrt_sender.[OTypeRel_Name] = 'PC2P'
INNER JOIN [XFS_Object] obj_sender
INNER JOIN [XFS_SourceType] st_sender
ON obj_sender.[SrcType_Id] = st_sender.[SrcType_Id]
AND st_sender.[SrcType_Location] = 'Email'
ON obr_sender.[Child_Obj_Id] = obj_sender.[Obj_Id]

INNER JOIN [XFS_AttrLong] a_sender
INNER JOIN [XFS_AttrType] at_sender
INNER JOIN [XFS_AttrCollection] ac_sender
ON at_sender.[AttrColl_Id] = ac_sender.[AttrColl_Id]
AND ac_sender.[AttrColl_Name] = 'BP'
ON a_sender.[AttrType_Id] = at_sender.[AttrType_Id]
AND at_sender.[AttrType_Name] = 'Name'
ON obj_sender.[Obj_Id] = a_sender.[Obj_Id]

ON obj_email.[Obj_Id] = obr_sender.[Parent_Obj_Id]
ON obr_email.[Child_Obj_Id] = obj_email.[Obj_Id]
ON obj_doc.[Obj_Id] = obr_email.[Parent_Obj_Id]



CODO ERGO SUM
Go to Top of Page

blindman
Master Smack Fu Yak Hacker

2365 Posts

Posted - 2010-06-15 : 14:59:39
There is absolutely nothing pretty about that code.
If beauty is in the eye of the beholder, your co-worker is extremely near-sighted.
That code is so ugly, I wouldn't go home with it if it was the last girl left at closing time.

________________________________________________
If it is not practically useful, then it is practically useless.
________________________________________________
Go to Top of Page

Michael Valentine Jones
Yak DBA Kernel (pronounced Colonel)

7020 Posts

Posted - 2010-06-15 : 16:30:01
quote:
...
That code is so ugly, I wouldn't go home with it if it was the last girl left at closing time.
...


Not even if you had 6 beers? or 8? or 12?







CODO ERGO SUM
Go to Top of Page
   

- Advertisement -