CodeSOD: Just A Temporary Thing

Daniel inherited some code which depends heavily on stored procedures. The person who developed those stored procedures was incredibly fond of overengineering things. Incredibly fond.

The result is a big pile of not incredibly interesting T-SQL code. I'll share the whole block of code in a moment, but there's one specific comment that I want to highlight before we look at it:

----Always try to update your temp table..in the event there is a error on your logic----you don't update a bunch of PROD recs in error

Don't touch data in real tables, until after touching the data in a temp table, just in case you make a mistake. This is how coding works, right? If you just add extra steps it'll stop mistakes, somehow, magically? This highlights the core logical errors.

CREATE PROCEDURE [dbo].[****_*****_Savecasesentstatus] @Xml XMLAS/*Created By: ************Date: 11/26/2020Use: Get the status on sent **** cases (initial or an update). Then inserts the records into table ****_*****_CaseSendVendorAuthStatus.Date: 12/03/2020 - adding code to check for errors. Don't want to wrap code in a Transaction for risk of locking up table *****.dbo.mm360_caseBase. I will use the @@ERROR option. */DECLARE @RecCount INT;SET NOCOUNT OFF;WITH CTEAS (SELECT CaseRecordID = XTbl.value('(CaseRecordID)[1]', 'nvarchar(200)') , Payor = XTbl.value('(Payor)[1]', 'varchar(5)') FROM @Xml.nodes('/NewDataSet/Table1') AS XD(XTbl))SELECT LTRIM(RTRIM(CaseRecordID)) AS CaseRecordID, Payor, CONVERT(VARCHAR(25), '') AS CaseSendStatusINTO #RecsFromSentFileFROM CTE;IF @@Error > 0 BEGIN SELECT @@Error AS ErrorNum; RETURN;END;SELECT a.CaseRecordID, a.Payor, a.CaseSendStatus, ISNULL(b.mm360_VendorAuthStatus, 'Null Found') AS mm360_VendorAuthStatusINTO #OrigRecsWithVendAuthStatusFROM #RecsFromSentFile AS aINNER JOIN******.dbo.mm360_caseBase AS bON a.CaseRecordID COLLATE SQL_Latin1_General_CP1_CI_AS = b.mm360_recordid COLLATE SQL_Latin1_General_CP1_CI_AS;IF @@Error > 0 BEGIN SELECT @@Error AS ErrorNum; RETURN;END;----Always try to update your temp table..in the event there is a error on your logic----you don't update a bunch of PROD recs in errorUPDATE a SET a.CaseSendStatus = CASE LTRIM(RTRIM(b.mm360_VendorAuthStatus)) WHEN '' THEN 'Initial' WHEN 'Null Found' THEN 'Initial' WHEN 'Initial Error' THEN 'Initial' WHEN 'Update Error' THEN 'Update' WHEN 'Finalized' THEN 'Update' WHEN 'Pending' THEN 'Pending' WHEN 'Voided' THEN 'Voided'ENDFROM #RecsFromSentFile aINNER JOIN#OrigRecsWithVendAuthStatus bON a.CaseRecordID = b.CaseRecordID;IF @@Error > 0 BEGIN SELECT @@Error AS ErrorNum; RETURN;END;-- Update present cases for same day...If multple file runs have took place ..exclude the previous cases-- to keep the integrity of data for reportingUPDATE a SET a.Exclude = 1FROM ****_*****_278_Sent_History aINNER JOIN#RecsFromSentFile bON a.CaseRecordID = b.CaseRecordIDWHERE CONVERT(VARCHAR(10), a.EnterDate, 101) = CONVERT(VARCHAR(10), GETDATE(), 101);----Store historical reference INSERT INTO ****_****_278_Sent_History(CaseRecordID , Payor , VendorAuthStatus , EnterDate , Exclude)SELECT CaseRecordID, Payor, CaseSendStatus, GETDATE(), 0FROM #RecsFromSentFile;SELECT @@RowCount AS TotalInsertedRecs;DROP TABLE #RecsFromSentFile;DROP TABLE #OrigRecsWithVendAuthStatus;

Now, some of this overcomplexity is because the they "Don't want to wrap code in a Transaction for risk of locking up table *****.dbo.mm360_caseBase". Of course, you can control when the transaction starts, so they could always do their read operation without acquiring locks if they don't really care, and then use transactions for the actual updates. Right off the bat, we get the sense that someone understands transactions have a cost and a tradeoff, but doesn't understand what that actually is or the correct way to manage that in SQL Server.

From there, we populate temp tables- the first with parsed XML data, the second with a quin back to that mm360_caseBase. Then we munge that data with some updates, many of which could have probably been done as part of the initial queries.

We also are doing this on temp tables, again, because we don't want to "update a bunch of PROD recs in error". Which is why we then go and update a bunch of prod records in 278_Sent_History to set the Exclude flag in the very next step- so that the new records we're adding can have an Exclude of 0.

How much of this is actually necessary? Turns out, pretty much none of it. The XML file actually contains much of the data they're joining to get, the whole Exclude flag thing is already handled by the downstream code without ever checking the Exclude flag. Daniel was able to rewrite this as a single "read from the XML and insert into the target table" without having to jump through all of those hoops.

And finally, as a minor pet peeve, since the temp tables are prefixed with #, they're local temp tables, which means they're auto-dropped as soon as they go out of scope. So the DROP TABLE statements at the end aren't needed.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!