5
\$\begingroup\$

I'm somewhat new to SQL (using it in the past but only being exposed to it heavily in my current role). Unfortunately nobody at my current company has really given me any advice on formatting. How can I format this better / what should I avoid doing that I may have done below? Any feedback on re-factoring would be greatly appreciated also.

/* Uncomment when testing this SQL */
declare @period_id as int = 252 -- ### Potentially redundant ###
declare @building_id as varchar(10) = '20500'
declare @section_name as varchar(20) = 'ALL'
declare @lease_expiry_period as int = 1;


/* SQL for Section_Name parameter on report */
select   'ALL',
         1 as order_by
union all
select distinct upper(isnull(section_name, '')) as section_name,
                2 as order_by
from     property.lease_period
where    section_name <> ''
order by 2;

declare @truncateddate as date = dateadd(d,0,datediff(d,0,getdate()));

/* SQL for Report (dsDetail) */
select
    lp.period_id ,
    lp.building_id ,
    lp.suite_id ,
    lp.lease_id ,
    lp.tenant_trading_name,
    lp.suite_status ,
    lp.suite_name ,
    lp.suite_area,
    lp.lease_status ,
    lp.lease_current_start_date ,
    lp.lease_current_stop_date ,
    lp.lease_current_term ,
    lp.lease_occupancy_date ,
    lp.lease_vacated_date
from
    property.lease_period lp
where
    lp.lease_current_stop_date < @truncateddate 
    and (upper(lp.lease_status) = 'ACTIVE' or upper(lp.lease_status) = 'OVERHOLDING')
    --and lp.period_id = @period_id -- ##Potentially redundant (query now running into the future)
    and lp.building_id = @building_id
    and not exists
    (
        select  1
        from    lease_deal.lease l
        where   lp.building_id = l.building_id
                and lp.lease_id = l.lease_id
    )
    and (@section_name = 'ALL' 
        or (@section_name <> 'ALL' and upper(section_name) = upper(@section_name)))
    and lp.lease_current_stop_date between @truncateddate 
        and dateadd(MONTH, @lease_expiry_period, @truncateddate)
order by period_id desc
\$\endgroup\$
1
  • \$\begingroup\$ FYI, Unless you have a persisted calculated column for upper(lp.lease_status) and upper(lp.section_name), you may want to research the impact of using a function within your where clause. Check out the following SO article for more details: stackoverflow.com/q/799584/5685 \$\endgroup\$ – Jeff Dec 30 '11 at 15:44
11
\$\begingroup\$

What you've got is pretty readable, but there are a few points to make:

  1. Your sub-query is written with a different indentation style from the main query.
  2. I prefer to see the keywords in upper-case.
  3. Personally, I dislike spaces before commas intensely (and you aren't 100% consistent about adding them). However, I loathe even more the style where the comma is put at the start of the next line:

         item1
       , item2
       , item3
    
  4. I'd use explicit AS to introduce table aliases. At least on the DBMS I use mainly, using AS gives a degree of future-proofing against aliases that become keywords in a later version of the product. That is, writing FROM SomeTable st runs into a problem later if st becomes a keyword, but writing FROM SomeTable AS st avoids that problem. Whether this helps in other DBMS is open to debate.

For your query, I'd probably write:

SELECT lp.period_id,
       lp.building_id,
       lp.suite_id,
       lp.lease_id,
       lp.tenant_trading_name,
       lp.suite_status,
       lp.suite_name,
       lp.suite_area,
       lp.lease_status,
       lp.lease_current_start_date,
       lp.lease_current_stop_date,
       lp.lease_current_term,
       lp.lease_occupancy_date,
       lp.lease_vacated_date
  FROM property.lease_period AS lp
 WHERE lp.lease_current_stop_date < @truncateddate 
   AND (UPPER(lp.lease_status) = 'ACTIVE' OR UPPER(lp.lease_status) = 'OVERHOLDING')
   AND lp.building_id = @building_id
   AND NOT EXISTS
       (SELECT *
          FROM lease_deal.lease AS l
         WHERE lp.building_id = l.building_id
           AND lp.lease_id = l.lease_id
       )
   AND (@section_name = 'ALL' 
        OR (@section_name <> 'ALL' AND UPPER(section_name) = UPPER(@section_name)))
   AND lp.lease_current_stop_date BETWEEN @truncateddate AND
                        DATEADD(MONTH, @lease_expiry_period, @truncateddate)
 ORDER BY period_id DESC;

Sometimes, I'd indent the sub-query more than I did here, occasionally even placing it after the EXISTS:

   AND NOT EXISTS (SELECT *
                     FROM lease_deal.lease AS l
                    WHERE lp.building_id = l.building_id
                      AND lp.lease_id = l.lease_id
                  )

That has a tendency to run off the right-hand edge of the screen, though.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Just wondering what DBMS you're using (in relation to declaring 'as' explicitly) \$\endgroup\$ – Michael A Dec 23 '11 at 7:55
  • \$\begingroup\$ See my profile :) Informix. \$\endgroup\$ – Jonathan Leffler Dec 23 '11 at 15:41
  • 3
    \$\begingroup\$ You will find the style with the comma at the start of the next line very useful if you do a lot of copying/cutting and pasting column names around. I am so tired of SQL parsers bitchslapping me just because I forgot an extra comma right before the 'FROM' keyword. \$\endgroup\$ – Mike Nakis Dec 23 '11 at 15:42
  • \$\begingroup\$ Very valid point - also good when commenting lines out. Might adopt that - appreciated. Would upvote you but not enough karma yet :( \$\endgroup\$ – Michael A Dec 23 '11 at 15:45
  • 1
    \$\begingroup\$ I know that people use the leading comma because it makes it easier to move code around without fogettting to to remove unwanted trailing commas; it can also reduce the number of lines changed when you add a new column at the end of the select-list (because you don't change what was the last line; you only add a new one). Neither is, IMNSHO (but it is only opinion) sufficient excuse to justify the violence to the normal use of comma after words. It is an ugly and unwarranted abuse of the syntax. But I'm an old fuddy-duddy, and you aren't on my team, so you can do as you see fit. \$\endgroup\$ – Jonathan Leffler Dec 23 '11 at 15:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.