September 7, 2016 at 6:06 am
Good Afternoon All, This is my first post.
I created the below code and if works fine. However my DBA said "It does the job but is clunky" how could this be re-written so it's not "clunky"?
USE TESTDB
GO
DECLARE @checkDate DATE
SET @checkDate = DATEADD(day, DATEDIFF(day, 1, GETDATE()), 0)
SELECT d.WORK_TYPE AS 'Work Type',
CONVERT(varchar, a.ACT_DATE, 103) AS 'Date Opened',
CONVERT(varchar, a.ACT_DATE, 108) AS 'Time Opened',
b.IT_IDEN AS 'Item ID',
CONVERT(varchar, @checkDate, 103) AS 'Run Date',
a.CUSTID AS 'Customer Number',
CONVERT(varchar, ACT_CLOSE, 103) AS 'Close Date',
DATEDIFF(day, a.ACT_DATE, CAST(checkDate AS DATE)) AS 'Days Elapsed',
a.PERSON AS 'Started By',
c.HANDOFF AS 'Assigned To',
a.TYPE_STAT AS 'Status Code'
FROM main.SOURCE_DATA a
JOIN main.LINKED_DATA b
ON a.ID=b.ID
JOIN main.LOOK_UP c
ON a.ID=c.ID
JOIN main.LOOK_UP_TYPE d
ON a.ID_TYPE=d.ID_TYPE
WHERE CAST(a.ACT_DATE AS DATE) >= @checkDate OR a.ACT_CLOSE IS NULL
ORDER BY 8 DESC,1
September 7, 2016 at 6:20 am
colin.dunn (9/7/2016)
Good Afternoon All, This is my first post.I created the below code and if works fine. However my DBA said "It does the job but is clunky" how could this be re-written so it's not "clunky"?
USE TESTDBGO
DECLARE @checkDate DATE
SET @checkDate = DATEADD(day, DATEDIFF(day, 1, GETDATE()), 0)
SELECT d.WORK_TYPE AS 'Work Type',
CONVERT(varchar, a.ACT_DATE, 103) AS 'Date Opened',
CONVERT(varchar, a.ACT_DATE, 108) AS 'Time Opened',
b.IT_IDEN AS 'Item ID',
CONVERT(varchar, @checkDate, 103) AS 'Run Date',
a.CUSTID AS 'Customer Number',
CONVERT(varchar, ACT_CLOSE, 103) AS 'Close Date',
DATEDIFF(day, a.ACT_DATE, CAST(checkDate AS DATE)) AS 'Days Elapsed',
a.PERSON AS 'Started By',
c.HANDOFF AS 'Assigned To',
a.TYPE_STAT AS 'Status Code'
FROM main.SOURCE_DATA a
JOIN main.LINKED_DATA b
ON a.ID=b.ID
JOIN main.LOOK_UP c
ON a.ID=c.ID
JOIN main.LOOK_UP_TYPE d
ON a.ID_TYPE=d.ID_TYPE
WHERE CAST(a.ACT_DATE AS DATE) >= @checkDate OR a.ACT_CLOSE IS NULL
ORDER BY 8 DESC,1
I have some comments. I'm a bit pushed for time, so please excuse brevity:
Do this in one:
DECLARE @checkDate DATE = DATEADD(day, DATEDIFF(day, 1, GETDATE()), 0)
Don't put spaces in column aliases - it makes everything easier and there's no need for those single quotes
SELECT WorkType = d.WORK_TYPE,
varchar should be given a length, otherwise it defaults (and the default may be different, depending on context). Eg, VARCHAR(30), or whatever makes sense.
Give your table aliases meaningful names. Instead of 'b' for LINKED_DATA, use 'ld', for example.
I assume a.ACT_DATE is a datetime? If so, simplify your WHERE clause:
WHERE a.ACT_DATE >= @checkDate OR a.ACT_CLOSE IS NULL
Use semicolon terminators throughout.
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 7, 2016 at 6:28 am
You should also use column alias in your ORDER BY clause instead of position.
ORDER BY [Work Type] DESC, [Days Elapsed];
EDIT:
Also here are some additional comments
SELECT --Use the actual length (it won't vary)
CONVERT(char(10), a.ACT_DATE, 103) AS 'Date Opened',
CONVERT(char(8), a.ACT_DATE, 108) AS 'Time Opened',
--Don't use cast here, it's already a date data type and it wouldn't make any difference
DATEDIFF(day, a.ACT_DATE, @checkDate) AS 'Days Elapsed'
P.S. Your DBA should explain how to make it not "clunky".
September 7, 2016 at 6:31 am
Thanks for the info guys, I've been learning SQL for a while now and I want to get my scripts as effective as possible.
September 7, 2016 at 6:36 am
colin.dunn (9/7/2016)
Thanks for the info guys, I've been learning SQL for a while now and I want to get my scripts as effective as possible.
No problem. Keep posting your questions here and you'll learn fast!
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 7, 2016 at 6:40 am
Did he say it looks clunky or it runs clunky?
I think your DBA is being really picky unless you have company standards for formatting, but.
SELECT d.WORK_TYPE AS [Work Type],
CONVERT(varchar, a.ACT_DATE, 103) AS [Date Opened],
CONVERT(varchar, a.ACT_DATE, 108) AS [Time Opened],
b.IT_IDEN AS [Item ID],
CONVERT(varchar, @checkDate, 103) AS [Run Date],
a.CUSTID AS [Customer Number],
CONVERT(varchar, ACT_CLOSE, 103) AS [Close Date],
DATEDIFF(day, a.ACT_DATE, CAST(checkDate AS DATE)) AS [Days Elapsed],
a.PERSON AS [Started By],
c.HANDOFF AS [Assigned To],
a.TYPE_STAT AS [Status Code]
FROM main.SOURCE_DATA a
JOIN main.LINKED_DATA bON a.ID=b.ID
JOIN main.LOOK_UP cON a.ID=c.ID
JOIN main.LOOK_UP_TYPE d ON a.ID_TYPE=d.ID_TYPE
WHERE a.ACT_DATE >= CONVERT(DATE,DATEADD(day, DATEDIFF(day, 1, GETDATE()), 0))
OR a.ACT_CLOSE IS NULL
ORDER BY [Days Elapsed] DESC, [Work Type]
Got rid of your variable declaration and just pit the conversion code in the WHERE clause
Put the join ON clauses on the same line since they were so short.
** Replaced the column numbers int the ORDER BY clause with the actual column names. **
Put all column name in square brackets just because that is more usually associated with columns than the 'format' which is usually associated with constants. You should probably also take all the spaces out of your column names, so even the square brackets aren't necessary. Let the calling application pretty up the display of the column names.
Edited to add: I overlooked adding the length to the varchar datatypes but Luis is exactly right.
__________________________________________________
Against stupidity the gods themselves contend in vain. -- Friedrich Schiller
Stop, children, what's that sound? Everybody look what's going down. -- Stephen Stills
September 7, 2016 at 6:52 am
Thanks for your input Dixie
Did he say it looks clunky or it runs clunky?
He just said it looks clunky
Put the join ON clauses on the same line since they were so short.
I usually have my ON clauses on the same line as my JOIN, but when he was testing it, he changed it to the next line.
Thanks again.
September 7, 2016 at 7:04 am
colin.dunn (9/7/2016)
Put the join ON clauses on the same line since they were so short.
I usually have my ON clauses on the same line as my JOIN, but when he was testing it, he changed it to the next line.
<rant>
I don't like when people put the ON clauses in separate lines. It feels as if they weren't part of the JOIN. It seems like they still want to write code with the SQL-86 standard, where the tables and join conditions were set apart.
</rant>
That's one of the reasons for your DBA to tell you what means clunky to him/her. However, don't hesitate to come back for guidance. 😉
September 7, 2016 at 7:27 am
Tell your DBA that she get's the job done, but she's kind of clunky when it comes to offering constructive advice about T-SQL.
"Do not seek to follow in the footsteps of the wise. Instead, seek what they sought." - Matsuo Basho
September 7, 2016 at 8:28 am
Eric M Russell (9/7/2016)
Tell your DBA that she get's the job done, but she's kind of clunky when it comes to offering constructive advice about T-SQL.
I should have asked for advice at the time, but you are correct, he should have offered the advice. If it was me saying to someone "it's not the best way to do it", I would give them advice on the correct method.
September 7, 2016 at 9:56 am
Luis Cazares (9/7/2016)
colin.dunn (9/7/2016)
Put the join ON clauses on the same line since they were so short.
I usually have my ON clauses on the same line as my JOIN, but when he was testing it, he changed it to the next line.
<rant>
I don't like when people put the ON clauses in separate lines. It feels as if they weren't part of the JOIN. It seems like they still want to write code with the SQL-86 standard, where the tables and join conditions were set apart.
</rant>
That's one of the reasons for your DBA to tell you what means clunky to him/her. However, don't hesitate to come back for guidance. 😉
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
J. Drew Allen
Business Intelligence Analyst
Philadelphia, PA
September 7, 2016 at 1:07 pm
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. 😉
September 7, 2016 at 1:13 pm
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.
September 7, 2016 at 1:23 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.
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 🙂
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 7, 2016 at 1:28 pm
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...
MM
select geometry::STGeomFromWKB(0x0106000000020000000103000000010000000B0000001000000000000840000000000000003DD8CCCCCCCCCC0840000000000000003DD8CCCCCCCCCC08408014AE47E17AFC3F040000000000104000CDCCCCCCCCEC3F9C999999999913408014AE47E17AFC3F9C99999999991340000000000000003D0000000000001440000000000000003D000000000000144000000000000000400400000000001040000000000000F03F100000000000084000000000000000401000000000000840000000000000003D0103000000010000000B000000000000000000143D000000000000003D009E99999999B93F000000000000003D009E99999999B93F8014AE47E17AFC3F400000000000F03F00CDCCCCCCCCEC3FA06666666666FE3F8014AE47E17AFC3FA06666666666FE3F000000000000003D1800000000000040000000000000003D18000000000000400000000000000040400000000000F03F000000000000F03F000000000000143D0000000000000040000000000000143D000000000000003D, 0);Viewing 15 posts - 1 through 15 (of 44 total)
You must be logged in to reply to this topic. Login to reply