8
\$\begingroup\$

I think the following is good but wondering if it can be simplified or improved.

My condition for code execution is the following:

It is a Monday AND It is not the Monday following the last Saturday in the month.

This is the current sql:

DECLARE @now DATETIME = DATEADD(dd,DATEDIFF(dd,'19000101',SYSDATETIME()),'19000101');


IF (
        (--is it a monday?
            SELECT DATEPART(dw,@now)
        ) = 2  

        AND

        (--is it not the monday following the last Saturday of the month?
            (
            SELECT DATEDIFF(
                            DAY,
                            @now-2, 
                            DATEADD(MONTH,DATEDIFF(MONTH,0,@now-2)+1,0)-1 
                        )
            ) > 6
        )
    )
BEGIN;

  ...
  ...

END;
\$\endgroup\$
  • \$\begingroup\$ It would be nice if you could edit your post to include the rest of the query/procedure... I'm sure our T-SQL reviewers would have plenty to say ;-) \$\endgroup\$ – Mathieu Guindon Dec 21 '15 at 15:22
  • \$\begingroup\$ @Mat'sMug the bit in between the begin and end is a couple of thousand lines and not really relevant. \$\endgroup\$ – whytheq Dec 21 '15 at 15:40
7
\$\begingroup\$

Sometimes, comments aren't the clearest way to make a script readable:

IF (
        (--is it a monday?
            SELECT DATEPART(dw,@now)
        ) = 2

Consider:

declare @MONDAY int;
set @MONDAY = 2;

if (datepart(weekday,@now) = @MONDAY)

Does one need a comment to figure out what's going on? I hope not!

This part however:

DECLARE @now DATETIME = DATEADD(dd,DATEDIFF(dd,'19000101',SYSDATETIME()),'19000101');

If it wasn't of the @now identifier, I'd wonder what this convoluted assignment intends to do.

Why not just do this instead?

declare @now date = cast(getdate() as date);

I mean, why work with a datetime when clearly you're only interested in the date part from that point on?


I find there's way too much whitespace in your code, you need to strike a balance between wall-of-code and air code.

Now, changing @now from a datetime to a date will break your code. The @now-2 here:

DATEDIFF(DAY, @now-2, DATEADD(MONTH,DATEDIFF(MONTH,0,@now-2)+1,0)-1

...is illegal because...

one does not simply subtract an int from a date in t-sql

I would extract that bit of logic into its own variable, for readability's sake, and using a dateadd function there would be no issue with subtracting 2 days from that date.

\$\endgroup\$
  • \$\begingroup\$ Adding the variable is a very nice idea and makes things much more readable. I prefer DateTime(as, for example, I can do @now+1 in the code inside the Begin-End). The convoluted section was suggested to me by Itzik B-G, so I'm happy sticking with it - unfortunately I don't remember the rationale but he said we should use SYSDATETIME() rather than getdate(). My second condition I'm particularly interested to know whether it can be simplified or improved? \$\endgroup\$ – whytheq Dec 21 '15 at 15:45
  • \$\begingroup\$ I will be sticking with DateTime for exactly the reason you have discovered - I like to be able to subtract or add integer's. \$\endgroup\$ – whytheq Dec 21 '15 at 15:49
  • 1
    \$\begingroup\$ @whytheq except, with dateadd it's completely unambiguous what's being subtracted. Subtracting 2 from a datetime should be illegal as well... and datetime is a legacy type \$\endgroup\$ – Mathieu Guindon Dec 21 '15 at 15:51
  • \$\begingroup\$ ....how did you find that reference to my question on the same topic so quickly ?! ....spooky!! \$\endgroup\$ – whytheq Dec 21 '15 at 15:59
  • 1
    \$\begingroup\$ One does not simply not upvote this answer \$\endgroup\$ – Quill Dec 24 '15 at 1:06
0
\$\begingroup\$

Following Mat's Mug's advice I have the following

DECLARE @now DATE = DATEADD(dd,DATEDIFF(dd,'19000101',SYSDATETIME()),'19000101');


DECLARE @MONDAY INT = 2;
DECLARE @dayBeforeYESTERDAY DATE = DATEADD(dd,-2,@now);
DECLARE @LastDayOfMthfor_dayBeforeYESTERDAY DATE = DATEADD(DAY,-1,DATEADD(MONTH,DATEDIFF(MONTH,0,@dayBeforeYESTERDAY)+1,0));

IF (

        (SELECT DATEPART(dw,@now)) = @MONDAY        
        AND
        (
            --check to see if it is not following the last Saturday in month
            (SELECT DATEDIFF(DAY,@dayBeforeYESTERDAY, @LastDayOfMthfor_dayBeforeYESTERDAY))
            > 6
        )
    )
BEGIN;
...
...
END;
\$\endgroup\$
  • \$\begingroup\$ If you want additional feedback for a revised version, it would be better to ask a new "follow-up" question (linking back to this one) - self-answers are allowed and even encouraged, but they should be in the form of a peer review... this answer isn't really an answer/review, I'd recommend you either edit it into a "review-shaped answer", or remove it. Cheers! \$\endgroup\$ – Mathieu Guindon Dec 23 '15 at 15:05

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.