September 18, 2018 at 9:13 am
September 18, 2018 at 9:24 am
Hard to really help when you only provide snippets of code. In the last, why are you only inserting the data for a single id determined by the value of the variable @id instead of just inserting all of the data for all id's? And actually, you want to do the insert only if the data being inserted doesn't already exist in the target table.
September 18, 2018 at 9:42 am
sgmunson - Monday, September 17, 2018 1:40 PMthelenj - Monday, September 17, 2018 1:21 PMPhil Parkin - Monday, September 17, 2018 1:15 PMIt's fine, except I'd add in a JOIN instead of using the old-style WHERE:
SELECT
T1.Id
, T1.IsActive
, T1.StartDate
, T1.EndDate
, T1.Title
, T1.Body
, CreateName = T2.NetId
, T1.CreateDate
FROM
Alert T1
JOIN [User] T2 ON T1.CreateId = T2.Id;I'd also suggest that you try to get into the habit of terminating your statements with a semicolon (or let SQL Prompt do it for you).
Does using the JOIN statement help with speed?
I find your formatting to be interesting, is this common?
Thanks for the input.You may well find that formatting is somewhat personal in terms of preferences. I happen to hate having the commas at the beginning of a line. I also prefer to be specific about the JOIN that I want to use, so I'll never use just JOIN. It will always be INNER JOIN, LEFT JOIN, or FULL OUTER JOIN for me. And I happen to set my tabs to equal 4 spaces in SSMS. I also indent my JOINs. and an additional indent for the ON clause as well. On SELECT, GROUP BY, and ORDER BY, the list of columns always gets started on the next line and indented, and I never use an equal sign to assign a column name, and I always use AS SomeColumnName for that purpose. I also insist on making data types as all lower case, and all SQL keywords capitalized. Not everyone is going to agree with my preferences, or yours, but these kinds of things tend to make code a LOT more readable and easy on the eyes.
I used to hate leading commas, as well. Now, I love them just like I love the use of columnalias = column rather than the ol' column AS columnalias and for much the same reasons. I've also found that they're also easier to edit especially if you use in-row comments at the end of the line and especially since "vertical selection editing" has been available for quite a while.
Not arguing for either way... have spent way too much time on that in the past. Just expressing that I used to hate leading commas and then fell in love with them a couple of years ago and just a touch of the reasons why.
--Jeff Moden
Change is inevitable... Change for the better is not.
September 18, 2018 at 10:59 am
Lynn Pettis - Tuesday, September 18, 2018 9:24 AMHard to really help when you only provide snippets of code. In the last, why are you only inserting the data for a single id determined by the value of the variable @id instead of just inserting all of the data for all id's? And actually, you want to do the insert only if the data being inserted doesn't already exist in the target table.
I'm sorry for the sad state of things.
The web is acting up for me today and I'm having real trouble posting here.
This is a case where I update a single row in Table A and where Table B acts as a log table of changes.
INSERT INTO
StatusLog
(Id, IsOffline, IsOfflineMessage, UpdateId, UpdateDate)
SELECT
T1.Id
, T1.IsOffline
, T1.IsOfflineMessage
, T1.UpdateId
, T1.UpdateDate
FROM
[Status] AS T1
WHERE
T1.Id = @Id;
Edited: To add WHERE clause.
September 18, 2018 at 11:15 am
thelenj - Tuesday, September 18, 2018 10:59 AMLynn Pettis - Tuesday, September 18, 2018 9:24 AMHard to really help when you only provide snippets of code. In the last, why are you only inserting the data for a single id determined by the value of the variable @id instead of just inserting all of the data for all id's? And actually, you want to do the insert only if the data being inserted doesn't already exist in the target table.I'm sorry for the sad state of things.
The web is acting up for me today and I'm having real trouble posting here.
This is a case where I update a single row in Table A and where Table B acts as a log table of changes.
UPDATE
[Status]
SET
IsOffline = @IsOffline
, IsOfflineMessage = @IsOfflineMessage
, UpdateId = @UpdateId
, UpdateDate = GETDATE()WHERE
Id = @Id;INSERT INTO
StatusLog
(Id, IsOffline, IsOfflineMessage, UpdateId, UpdateDate)
SELECT
T1.Id
, T1.IsOffline
, T1.IsOfflineMessage
, T1.UpdateId
, T1.UpdateDate
FROM
[Status] AS T1
WHERE
T1.Id = @Id;Edited: To add WHERE clause.
Actually, here is another way to accomplish that:
UPDATE
[dbo].[Status]
SET
[IsOffline] = @IsOffline
, [IsOfflineMessage] = @IsOfflineMessage
, [UpdateId] = @UpdateId
, [UpdateDate] = GETDATE()
OUTPUT
[Inserted].[id]
, [Inserted].[IsOffline]
, [Inserted].[IsOfflineId]
, [Inserted].[UpdateId]
, [Inserted].[UpdateDate]
INTO [dbo].[StatusLog]
(
[Id]
, [IsOffline]
, [IsOfflineMessage]
, [UpdateId]
, [UpdateDate]
)
WHERE
[Id] = @Id;
September 18, 2018 at 11:17 am
Chris Wooding - Tuesday, September 18, 2018 7:11 AMScottPletcher - Monday, September 17, 2018 3:24 PM100% not using a JOIN clause screams "noob".But I see many more issues.
A serious one is not including a schema name before the table names. You would only leave off schema in the (hopefully rare) instances where you wanted different people to see different tables based on their own default schema (a rather rare technique, nowadays, except perhaps for certain vendors(?)).
While certainly not a noob issue (most long-term developers don't add them to their code), the lack of the correct SET statements are also major and are added in the code below. The SETs in the proc are required for certain "advanced" indexes, among them filtered indexes. Your DBA will appreciate you adding the SETs -- because his/her index tuning won't go to waste -- and you'll appreciate it, because your code won't crash at some point after the DBA adds a filtered index. For my own ease of use, I put the SETs in alpha order.
SET XACT_ABORT ON has some serious implications, so you'll want to check with in-your-shop experts before using that one. In fact, ditto for the SET statements in general. But they are the future, so you might as well start preparing for them now.
A final, minor point, get rid of the comment lines regarding NOCOUNT and "--Insert statements for procedure here" -- only a noob would need them!
I often leave a blank line, or a '--', before the FROM statement if the list of columns is long (or even kinda long), but of course that's just a preference and not a noob vs non-noob thing.
SET ANSI_NULLS ON;
SET QUOTED_IDENTIFIER ON;
GO
CREATE PROCEDURE dbo.proc_name
@param_1 ...
AS
SET ANSI_PADDING ON;
SET ANSI_WARNINGS ON;
SET ARITHABORT ON;
SET CONCAT_NULL_YIELDS_NULL ON;
SET NOCOUNT ON;
SET NUMERIC_ROUNDABORT OFF;
SET XACT_ABORT ON;SELECT
T1.Id,
T1.IsActive,
T1.StartDate,
T1.EndDate,
T1.Title,
T1.Body,
T2.NetId AS CreateName,
T1.CreateDateFROM dbo.Alert AS T1
INNER JOIN dbo.[User] AS T2 ON T1.CreateId = T2.Id/*End of proc: "dbo.proc_name"*/
Shouldn't those SET options be specified against the database and then left out of the stored procedure so that it takes the database defaults? Otherwise there'd be the probability of some stored procedures using different settings and therefore having behaviour that could cause confusion.
I don't see how that could really work, since the connection settings override the db settings, as I understand it. SSMS has its own default settings. Apps/users can specify settings of their connection before connecting to SQL. I agree db defaults should be set, but I don't see that you can rely at all on inheriting them.
MS agrees, as specified in BOL in "CREATE PROCEDURE":
If the logic of the procedure depends on a particular setting, include a SET statement at the start of the procedure to guarantee the appropriate setting.
I think XACT_ABORT has to be set in the proc regardless, as it's not a db setting.
SQL DBA,SQL Server MVP(07, 08, 09) A socialist is someone who will give you the shirt off *someone else's* back.
September 18, 2018 at 11:19 am
Lynn Pettis - Tuesday, September 18, 2018 7:49 AMChris Wooding - Tuesday, September 18, 2018 7:11 AMScottPletcher - Monday, September 17, 2018 3:24 PM100% not using a JOIN clause screams "noob".But I see many more issues.
A serious one is not including a schema name before the table names. You would only leave off schema in the (hopefully rare) instances where you wanted different people to see different tables based on their own default schema (a rather rare technique, nowadays, except perhaps for certain vendors(?)).
While certainly not a noob issue (most long-term developers don't add them to their code), the lack of the correct SET statements are also major and are added in the code below. The SETs in the proc are required for certain "advanced" indexes, among them filtered indexes. Your DBA will appreciate you adding the SETs -- because his/her index tuning won't go to waste -- and you'll appreciate it, because your code won't crash at some point after the DBA adds a filtered index. For my own ease of use, I put the SETs in alpha order.
SET XACT_ABORT ON has some serious implications, so you'll want to check with in-your-shop experts before using that one. In fact, ditto for the SET statements in general. But they are the future, so you might as well start preparing for them now.
A final, minor point, get rid of the comment lines regarding NOCOUNT and "--Insert statements for procedure here" -- only a noob would need them!
I often leave a blank line, or a '--', before the FROM statement if the list of columns is long (or even kinda long), but of course that's just a preference and not a noob vs non-noob thing.
SET ANSI_NULLS ON;
SET QUOTED_IDENTIFIER ON;
GO
CREATE PROCEDURE dbo.proc_name
@param_1 ...
AS
SET ANSI_PADDING ON;
SET ANSI_WARNINGS ON;
SET ARITHABORT ON;
SET CONCAT_NULL_YIELDS_NULL ON;
SET NOCOUNT ON;
SET NUMERIC_ROUNDABORT OFF;
SET XACT_ABORT ON;SELECT
T1.Id,
T1.IsActive,
T1.StartDate,
T1.EndDate,
T1.Title,
T1.Body,
T2.NetId AS CreateName,
T1.CreateDateFROM dbo.Alert AS T1
INNER JOIN dbo.[User] AS T2 ON T1.CreateId = T2.Id/*End of proc: "dbo.proc_name"*/
Shouldn't those SET options be specified against the database and then left out of the stored procedure so that it takes the database defaults? Otherwise there'd be the probability of some stored procedures using different settings and therefore having behaviour that could cause confusion.
As coded the SET options are outside of the stored procedure. The stored procedure uses the setting as set at the time it is created, iirc.
The ANSI_NULLS and QUOTED_IDENTIFIER settings are saved at creation time, since they must be known in order to correctly parse the code. You can see them in the sys.sql_modules view. As I understand it, the other settings are all specified at run time.
SQL DBA,SQL Server MVP(07, 08, 09) A socialist is someone who will give you the shirt off *someone else's* back.
September 18, 2018 at 12:01 pm
Lynn Pettis - Tuesday, September 18, 2018 11:15 AMthelenj - Tuesday, September 18, 2018 10:59 AMLynn Pettis - Tuesday, September 18, 2018 9:24 AMHard to really help when you only provide snippets of code. In the last, why are you only inserting the data for a single id determined by the value of the variable @id instead of just inserting all of the data for all id's? And actually, you want to do the insert only if the data being inserted doesn't already exist in the target table.I'm sorry for the sad state of things.
The web is acting up for me today and I'm having real trouble posting here.
This is a case where I update a single row in Table A and where Table B acts as a log table of changes.
UPDATE
[Status]
SET
IsOffline = @IsOffline
, IsOfflineMessage = @IsOfflineMessage
, UpdateId = @UpdateId
, UpdateDate = GETDATE()WHERE
Id = @Id;INSERT INTO
StatusLog
(Id, IsOffline, IsOfflineMessage, UpdateId, UpdateDate)
SELECT
T1.Id
, T1.IsOffline
, T1.IsOfflineMessage
, T1.UpdateId
, T1.UpdateDate
FROM
[Status] AS T1
WHERE
T1.Id = @Id;Edited: To add WHERE clause.
Actually, here is another way to accomplish that:
UPDATE
[dbo].[Status]
SET
[IsOffline] = @IsOffline
, [IsOfflineMessage] = @IsOfflineMessage
, [UpdateId] = @UpdateId
, [UpdateDate] = GETDATE()
OUTPUT
[Inserted].[id]
, [Inserted].[IsOffline]
, [Inserted].[IsOfflineId]
, [Inserted].[UpdateId]
, [Inserted].[UpdateDate]
INTO [dbo].[StatusLog]
(
[Id]
, [IsOffline]
, [IsOfflineMessage]
, [UpdateId]
, [UpdateDate]
)
WHERE
[Id] = @Id;
OMG, what strange voodoo is this?
This must be what was referenced in an earlier post about doing it all at once.
I bet this is more performant as well.
Nice.
Why the brackets around absolutely everything?
September 18, 2018 at 1:20 pm
thelenj - Tuesday, September 18, 2018 12:01 PMLynn Pettis - Tuesday, September 18, 2018 11:15 AMthelenj - Tuesday, September 18, 2018 10:59 AMLynn Pettis - Tuesday, September 18, 2018 9:24 AMHard to really help when you only provide snippets of code. In the last, why are you only inserting the data for a single id determined by the value of the variable @id instead of just inserting all of the data for all id's? And actually, you want to do the insert only if the data being inserted doesn't already exist in the target table.I'm sorry for the sad state of things.
The web is acting up for me today and I'm having real trouble posting here.
This is a case where I update a single row in Table A and where Table B acts as a log table of changes.
UPDATE
[Status]
SET
IsOffline = @IsOffline
, IsOfflineMessage = @IsOfflineMessage
, UpdateId = @UpdateId
, UpdateDate = GETDATE()WHERE
Id = @Id;INSERT INTO
StatusLog
(Id, IsOffline, IsOfflineMessage, UpdateId, UpdateDate)
SELECT
T1.Id
, T1.IsOffline
, T1.IsOfflineMessage
, T1.UpdateId
, T1.UpdateDate
FROM
[Status] AS T1
WHERE
T1.Id = @Id;Edited: To add WHERE clause.
Actually, here is another way to accomplish that:
UPDATE
[dbo].[Status]
SET
[IsOffline] = @IsOffline
, [IsOfflineMessage] = @IsOfflineMessage
, [UpdateId] = @UpdateId
, [UpdateDate] = GETDATE()
OUTPUT
[Inserted].[id]
, [Inserted].[IsOffline]
, [Inserted].[IsOfflineId]
, [Inserted].[UpdateId]
, [Inserted].[UpdateDate]
INTO [dbo].[StatusLog]
(
[Id]
, [IsOffline]
, [IsOfflineMessage]
, [UpdateId]
, [UpdateDate]
)
WHERE
[Id] = @Id;OMG, what strange voodoo is this?
This must be what was referenced in an earlier post about doing it all at once.
I bet this is more performant as well.Nice.
Why the brackets around absolutely everything?
It is defensive programming I have learned to use where I work. Plus, I configured SQL Prompt to put them in.
September 18, 2018 at 1:23 pm
Thank you for the help, people!
Like I said, I've put most of these ideas into practice and will be using them going forward.
September 19, 2018 at 6:18 am
ScottPletcher - Tuesday, September 18, 2018 11:17 AMChris Wooding - Tuesday, September 18, 2018 7:11 AMScottPletcher - Monday, September 17, 2018 3:24 PM100% not using a JOIN clause screams "noob".But I see many more issues.
A serious one is not including a schema name before the table names. You would only leave off schema in the (hopefully rare) instances where you wanted different people to see different tables based on their own default schema (a rather rare technique, nowadays, except perhaps for certain vendors(?)).
While certainly not a noob issue (most long-term developers don't add them to their code), the lack of the correct SET statements are also major and are added in the code below. The SETs in the proc are required for certain "advanced" indexes, among them filtered indexes. Your DBA will appreciate you adding the SETs -- because his/her index tuning won't go to waste -- and you'll appreciate it, because your code won't crash at some point after the DBA adds a filtered index. For my own ease of use, I put the SETs in alpha order.
SET XACT_ABORT ON has some serious implications, so you'll want to check with in-your-shop experts before using that one. In fact, ditto for the SET statements in general. But they are the future, so you might as well start preparing for them now.
A final, minor point, get rid of the comment lines regarding NOCOUNT and "--Insert statements for procedure here" -- only a noob would need them!
I often leave a blank line, or a '--', before the FROM statement if the list of columns is long (or even kinda long), but of course that's just a preference and not a noob vs non-noob thing.
SET ANSI_NULLS ON;
SET QUOTED_IDENTIFIER ON;
GO
CREATE PROCEDURE dbo.proc_name
@param_1 ...
AS
SET ANSI_PADDING ON;
SET ANSI_WARNINGS ON;
SET ARITHABORT ON;
SET CONCAT_NULL_YIELDS_NULL ON;
SET NOCOUNT ON;
SET NUMERIC_ROUNDABORT OFF;
SET XACT_ABORT ON;SELECT
T1.Id,
T1.IsActive,
T1.StartDate,
T1.EndDate,
T1.Title,
T1.Body,
T2.NetId AS CreateName,
T1.CreateDateFROM dbo.Alert AS T1
INNER JOIN dbo.[User] AS T2 ON T1.CreateId = T2.Id/*End of proc: "dbo.proc_name"*/
Shouldn't those SET options be specified against the database and then left out of the stored procedure so that it takes the database defaults? Otherwise there'd be the probability of some stored procedures using different settings and therefore having behaviour that could cause confusion.
I don't see how that could really work, since the connection settings override the db settings, as I understand it. SSMS has its own default settings. Apps/users can specify settings of their connection before connecting to SQL. I agree db defaults should be set, but I don't see that you can rely at all on inheriting them.
MS agrees, as specified in BOL in "CREATE PROCEDURE":
If the logic of the procedure depends on a particular setting, include a SET statement at the start of the procedure to guarantee the appropriate setting.
I think XACT_ABORT has to be set in the proc regardless, as it's not a db setting.
Thanks. We've been relying on the database settings - it hadn't occurred to me that individuals could tinker with their defaults and end up with them being applied to all stored procedures they create. I can see our standards being updated shortly to include your code block.
September 19, 2018 at 7:45 am
thelenj - Monday, September 17, 2018 1:09 PMThis is pretty typical SQL Code for me:
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;-- Insert statements for procedure here
SELECT
T1.Id,
T1.IsActive,
T1.StartDate,
T1.EndDate,
T1.Title,
T1.Body,
T2.NetId AS CreateName,
T1.CreateDate
FROM
Alert AS T1,
[User] AS T2
WHERE
T1.CreateId = T2.IdI've been writing SQL code like this for 15 years; however, I've begun to wonder if there is a better (or more professional) way.
I don't interact with other developers so sometimes I wonder if I'm making mistakes that are glaringly obvious but I'm never told about them.
Does this look like NOOB code, any way to make it more 'professional'?
One tip/personal pet peeve on aliasing is to use meaningful aliases. Use something mnemonic that makes it easily clear what table the alias represents and makes it easy to interpret and debug the code -- e.g.
U for User and A for Alert -- rather than arbitrary characters/code like A, B, C or T1, T2 (apologies if these actually have contextual meaning in your example). Use a long enough string to convey meaning & intelligently distinguish tables. It probably doesn't make much difference w/ a simple select joining two tables, but improving readability, and reducing the risk and of confusing which alias is, which can be very important w/ a large complex query involving many tables. Relational databases tend to produce similarly used tables -- e.g., User, UserPreference, UserPreferenceHistory, UserAddress, etc. So a meaningful alias may require more characters the more tables you join.
I have waivered on aliasing over the years -- I don't see much value in aliasing short tables names, especially given the availability of intellisense. But there are times when it's absolutely necessary because one is joining to the same table more than once. In those cases, it's probably even more valuable to use an alias that will help distinguish the two different uses of the same table. And there is a lot to be said for consistency.
One could argue that intellisense makes my concerns about meaningful aliases less important -- one can hover over an alias and see the real table name. But that takes time & reduces efficiency & readability -- one can't necessarily just read the query and interpret it accurately w/o stopping to hover & interact w/ individual elements.
September 19, 2018 at 8:57 am
RAThor - Wednesday, September 19, 2018 7:45 AMthelenj - Monday, September 17, 2018 1:09 PMThis is pretty typical SQL Code for me:
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;-- Insert statements for procedure here
SELECT
T1.Id,
T1.IsActive,
T1.StartDate,
T1.EndDate,
T1.Title,
T1.Body,
T2.NetId AS CreateName,
T1.CreateDate
FROM
Alert AS T1,
[User] AS T2
WHERE
T1.CreateId = T2.IdI've been writing SQL code like this for 15 years; however, I've begun to wonder if there is a better (or more professional) way.
I don't interact with other developers so sometimes I wonder if I'm making mistakes that are glaringly obvious but I'm never told about them.
Does this look like NOOB code, any way to make it more 'professional'?One tip/personal pet peeve on aliasing is to use meaningful aliases. Use something mnemonic that makes it easily clear what table the alias represents and makes it easy to interpret and debug the code -- e.g.
U for User and A for Alert -- rather than arbitrary characters/code like A, B, C or T1, T2 (apologies if these actually have contextual meaning in your example). Use a long enough string to convey meaning & intelligently distinguish tables. It probably doesn't make much difference w/ a simple select joining two tables, but improving readability, and reducing the risk and of confusing which alias is, which can be very important w/ a large complex query involving many tables. Relational databases tend to produce similarly used tables -- e.g., User, UserPreference, UserPreferenceHistory, UserAddress, etc. So a meaningful alias may require more characters the more tables you join.I have waivered on aliasing over the years -- I don't see much value in aliasing short tables names, especially given the availability of intellisense. But there are times when it's absolutely necessary because one is joining to the same table more than once. In those cases, it's probably even more valuable to use an alias that will help distinguish the two different uses of the same table. And there is a lot to be said for consistency.
One could argue that intellisense makes my concerns about meaningful aliases less important -- one can hover over an alias and see the real table name. But that takes time & reduces efficiency & readability -- one can't necessarily just read the query and interpret it accurately w/o stopping to hover & interact w/ individual elements.
I would say always use table aliases, even with single table queries. I can't count the times that a single table query was changed to include multiple tables. If you are in the habit of using aliases it is easier.
September 19, 2018 at 11:28 am
Phil Parkin - Monday, September 17, 2018 1:15 PMI loathe commas at the start, because then you always have to read the next line to see if you're done with the current column definition. That's a royal pita when you're dealing with a query where many/most columns are defined using multiple lines ... and even worse when it's just the one column you're working on, and you didn't notice it, and you coded in the middle of the existing column def.
Since I'm older than dirt, I actually used a program with punch cards! Google it or talk to your grandfather if you don't know what that means 🙂 the reason we put the, at the start of the punch card was so we could rearrange them and reuse them. When I worked at AIRMICS, we were engaged in research to make code readable for for the Department of Defense and the Army. The leading comma actually confuses the reader and makes the code harder to read or maintain.
The leading comma is just a left over from the old days, much like the watch pocket. We used to have on blue jeans long after nobody had pocket watches.
Please post DDL and follow ANSI/ISO standards when asking for help.
September 19, 2018 at 11:37 am
thelenj - Monday, September 17, 2018 1:21 PMPhil Parkin - Monday, September 17, 2018 1:15 PMI loathe commas at the start, because then you always have to read the next line to see if you're done with the current column definition. That's a royal pita when you're dealing with a query where many/most columns are defined using multiple lines ... and even worse when it's just the one column you're working on, and you didn't notice it, and you coded in the middle of the existing column def.
Since I'm older than dirt, I actually used a program with punch cards! Google it or talk to your grandfather if you don't know what that means 🙂 the reason we put the, at the start of the punch card was so we could rearrange them and reuse them. When I worked at AIRMICS, we were engaged in research to make code readable for for the Department of Defense and the Army. The leading comma actually confuses the reader and makes the code harder to read or maintain.
The leading comma is just a left over from the old days, much like the watch pocket. We used to have on blue jeans long after nobody had pocket watches.
Maybe it confused you (the reader), card readers didn't care. Yes, I know exactly what punch cards are, used them. I also used a card sorter, that is why there was unused columns on the cards for sequence numbers in case a careless programmer or operator decided to do a floor sort of the deck to get it back in proper sequence.
Viewing 15 posts - 16 through 30 (of 33 total)
You must be logged in to reply to this topic. Login to reply