SQL can stink too - Code smell in stored procedures

Figure: You don't need to dig deep to find smelly code

I just found this nice piece of TSQL that I came across in a 500 line stored procedure on my current project.
There are a couple of problems with the below query.

    -- Sanitized for blog
    insert into LinksBetweenPerson (LinkItemID, PersonID1, PersonID2, DateUpdated, DateCreated, EmpUpdated, EmpCreated)
    select newid(), A.PersonID1, A.PersonID2, getdate(), getdate(), suser_sname(), suser_sname()
    from
    (
          select distinct A.PersonWorkTimeID as PersonID1, B.PersonWorkTimeID as PersonID2 
          from PersonWorkTime A                
          inner join
          (
                select A.PersonID, C.PersonWorkTimeID,  C.startTime, C.endTime 
                from PersonWorkplaceTemplate A              
                inner join PersonWorkplaceTemplate B
                on A.PersonWorkplaceTemplateID = B.PersonWorkplaceTemplateID
                inner join (
                      select B.PersonWorkTimeID, B.startTime, B.endTime,
                            case datepart(weekday,DATEADD(hh, DATEDIFF(hh, B.StartTime, B.EndTime)/2, B.StartTime))
                                  when 1 then A.Sunday
                                  when 2 then A.Monday
                                  when 3 then A.Tuesday
                                  when 4 then A.Wednesday
                                  when 5 then A.Thursday
                                  when 6 then A.Friday
                                  else A.Saturday
                            end as [Week]
                      from PersonWorkplaceTemplateLinkItem A             
                      inner join #SourceWorkTime B on (A.[week]%@totalWeeks) = B.[week]
                      where A.PersonWorkplaceTemplateID = @PersonWorkplaceTemplateID ) c
                on ((DateDiff(day, B.StartDate, C.StartTime)/7+1)%@linkedTemplateTotalWeeks + A.[Order] *2)%@linkedTemplateTotalWeeks = C.[Week]%@linkedTemplateTotalWeeks
                where A.PersonWorkplaceTemplateID = @linkedTemplateID
          ) B on A.PersonID = B.PersonID and A.StartTime < B.EndTime and A.EndTime > B.StartTime
          where A.IsTemplateCreated = 1 and A.WorkPlaceID = @WorkPlaceID      
    ) A   
    left outer join LinksBetweenPeople B
    on A.PersonID1 = B.PersonID1 and A.PersonID2 = B.PersonID2
    where B.LinkItemID is null    

 

Before you read further try to spot some problems yourself…

 

.

 

.

 

.

.

.

.

 

Wait! Did you really try to spot some problems??

 

 

There were some little issues in this query that I fixed already before I published it here

#1 Code layout was all over the place.

Someone didn't care about the layout of the file. The next developer doesn't care either and we get into the problem of the Broken Window Theory

 

#2 Commented out code

 -- insert into LinksBetweenPerson (LinkItemID, PersonID1, PersonID2)
 -- select newid(), A.PersonID1, A.PersonID2
 insert into LinksBetweenPerson (LinkItemID, PersonID1, PersonID2, DateUpdated, DateCreated, EmpUpdated, EmpCreated)
 select newid(), A.PersonID1, A.PersonID2, getdate(), getdate(), suser_sname(), suser_sname()

Why is there some commented out code?
If you really can't delete code, because you feel so attached to it and it is a work of art, then leave a comment why this could be interesting for the next reader/developer.

 

.

.

.

.

 

Alright… Here are the big ones

#3 Naming of the result set: "A", "B" and "C" is bad cause there is no way to understand the intention

             select A.PersonID, C.PersonWorkTimeID,  C.startTime, C.endTime 
                from PersonWorkplaceTemplate A              
                inner join PersonWorkplaceTemplate B
                on A.PersonWorkplaceTemplateID = B.PersonWorkplaceTemplateID
                

Please give it a nice name as:
"personsInWorkplace1" or "filteredPersons" or "personsWithLink" or "whatever_You_Come_Up_With"
but *NOT*
"A", "B" or "C"

 

#4 Reusing alias "A" and "B" in the same query multiple times, although they are completely distinct result sets

          select distinct A.PersonWorkTimeID as PersonID1, B.PersonWorkTimeID as PersonID2 
          from PersonWorkTime A                
          inner join
          (
                select A.PersonID, C.PersonWorkTimeID,  C.startTime, C.endTime 
                from PersonWorkplaceTemplate A              
                inner join PersonWorkplaceTemplate B
                on A.PersonWorkplaceTemplateID = B.PersonWorkplaceTemplateID
                inner join (
                      select B.PersonWorkTimeID, B.startTime, B.endTime,
                            case datepart(weekday,DATEADD(hh, DATEDIFF(hh, B.StartTime, B.EndTime)/2, B.StartTime))
                                  when 1 then A.Sunday
                                  when 2 then A.Monday
                                  when 3 then A.Tuesday
                                  when 4 then A.Wednesday
                                  when 5 then A.Thursday
                                  when 6 then A.Friday
                                  else A.Saturday
                            end as [Week]
                      from PersonWorkplaceTemplateLinkItem A             
                      inner join #SourceWorkTime B on (A.[week]%@totalWeeks) = B.[week]
                      where A.PersonWorkplaceTemplateID = @PersonWorkplaceTemplateID ) c
                on ((DateDiff(day, B.StartDate, C.StartTime)/7+1)%@linkedTemplateTotalWeeks + A.[Order] *2)%@linkedTemplateTotalWeeks = C.[Week]%@linkedTemplateTotalWeeks
                where A.PersonWorkplaceTemplateID = @linkedTemplateID
          ) B on A.PersonID = B.PersonID and A.StartTime < B.EndTime and A.EndTime > B.StartTime
          where A.IsTemplateCreated = 1 and A.WorkPlaceID = @WorkPlaceID      

 

 

#5 The usage of DISTINCT is normally a sign of bad written queries, since the author tried to hack something together, instead of identified the planned result set. Not necessarily too bad then..

select distinct A.PersonWorkTimeID as PersonID1, B.PersonWorkTimeID as PersonID2 
          from PersonWorkTime A                
          inner join
       -- snip snip snip
         

 

Can you spot more problems?
What if I tell you to fix something in this query?
How could we avoid this?