8
\$\begingroup\$

I have some code that I wrote almost a year ago exactly (1/9/2013) and I would like to know if I wrote it well or if it can be improved. I don't have any fun input or output, as these are not set results coming out of this column in the table.

-- =============================================
-- Author: Malachi (Name Changed to Protect the possibly Guilty)
-- Create date: 1/9/2013
-- Description: Will give a list of the Bond Conditions
-- =============================================
CREATE FUNCTION [dbo].[fnBondConditionList] 
(
    @BondID int
)
RETURNS Varchar(MAX)
AS
BEGIN
    DECLARE @Result Varchar(MAX)

    SELECT @Result = (Select Justice.dbo.uCode.Description as BondConditions
        FROM Justice.dbo.uCode INNER JOIN 
            Justice.dbo.xBondCondition ON Justice.dbo.uCode.CodeID = Justice.dbo.xBondCondition.ConditionID
        WHERE Justice.dbo.xBondCondition.BondID = @BondID
        FOR XML PATH (''))

    SET @Result = replace(@Result, '<BondConditions>','')
    SET @Result = replace(@Result, '</BondConditions>','<br />')
    Set @Result = LEFT(@Result, LEN(@Result) - 1)

RETURN @Result
END

I cannot change the tables. Here are the table layouts:

uCode:

CREATE TABLE [dbo].[uCode](
    [CodeID] [dbo].[CodeID] NOT NULL,
    [CacheTableID] [dbo].[CacheTableID] NOT NULL,
    [RevisionID] [dbo].[RevisionID] NOT NULL,
    [Code] [dbo].[Word] NOT NULL,
    [RootNodeID] [dbo].[NodeID] NOT NULL,
    [EffectiveDate] [datetime] NULL,
    [ObsoleteDate] [dbo].[ObsoleteDate] NULL,
    [UserIDCreate] [dbo].[UserIDCreate] NOT NULL,
    [TimestampCreate] [dbo].[TimestampCreate] NOT NULL,
    [UserIDChange] [dbo].[UserIDChange] NULL,
    [TimestampChange] [dbo].[TimestampChange] NULL,
    [FilterState] [dbo].[StateCode] NULL,
    [FilterSiteID] [dbo].[GUID] NULL,
    [Description] [dbo].[DescriptionLong] NULL,
CONSTRAINT [PK_uCode] PRIMARY KEY NONCLUSTERED 
(
    [CodeID] ASC
)WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY],
CONSTRAINT [IXuCode1] UNIQUE CLUSTERED 
(
    [CacheTableID] ASC,
    [CodeID] ASC
)WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY]
) ON [PRIMARY]

xBondCondition:

CREATE TABLE [dbo].[xBondCondition](
    [BondID] [dbo].[BondID] NOT NULL,
    [ConditionID] [dbo].[CodeID] NOT NULL,
CONSTRAINT [PK_xBonduCode] PRIMARY KEY CLUSTERED 
(
    [BondID] ASC,
    [ConditionID] ASC
)WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY]
) ON [PRIMARY]

This code was created to stuff everything from one column into a single column of a single record, which is then fed to a stored procedure that fills a datagrid.

Should I be looping this somehow (I get a bad taste in my mouth just typing that)?

I update this because I find myself using the same FOR XML PATH ('') Template for something very similar.

I cannot use CONCAT because I am using SQL SERVER 2008 (wouldn't that be nice).

With all that going into the Stored Procedure, the SPROC Renders something similar to this:Image of Resulting Gridview

Where the rightmost column is the column of the SPROC that was produced by a function almost exactly the same as above.

\$\endgroup\$
  • \$\begingroup\$ I think you might need to have an argument with somebody about separating the presentation from the data. The code smells because the requirements smell. \$\endgroup\$ – RubberDuck Mar 17 '15 at 23:46
8
\$\begingroup\$

If you do not want the XML Tags, then do not add them!

This code frustrates me because:

  • you have not given any indication as to what this code should be doing.
  • you are embedding presentation-layer logic in the database layer
  • you convert all the data to an XML document, and then strip all the XML tags from the result because this appears to be a 'cheap' way to get all the data as a single result instead of multiple rows.
  • you do not document this
  • you use less-common features of the database (XML manipulation) without good reason, which locks you in to a single provider for the database ;-)
  • there is no documented reason for why you strip the last character from the @result... this will strip the closing > from the last <br />, and will thus leave some invalid X?HTML? I presume? A bug?

As for some nit-picks:

  • You should not be using SELECT @Result = (...), but should, instead be using SET @Result = (...) See the documentation for SELECT.
  • you capitalize some of your query key-words, but not all. I recommend using whatever case you do not use for your columns. In your case, your db.schema.table.columns are all lower/CamelCase, so your keywords should all be full CAPITALS. Thus, you should have SELECT everywhere. You have one SELECT, and one Select. You have one LEFT, and two replace. Your consistency of convention should be improved.
  • you do not use table-aliases for your query objects. Not only do table-aliases make the SQL more readable, but, if you need to change a table name for some reason (e.g. to get data from a view, or different schema), then you have to change multiple places, and could lead to bugs....

As far as I am concerned, this function should be removed entirely, and a simple presentation-layer system should be added that simply appends a <br /> after each bond description from the query:

SELECT uC.Description
FROM Justice.dbo.uCode uC
INNER JOIN Justice.dbo.xBondCondition xBC
        ON uC.CodeID = xBC.ConditionID
WHERE xBC.BondID = ?
\$\endgroup\$
  • \$\begingroup\$ good answer, I am taking Data that is Created by an application and displaying it on an additional information page of a website. I agree with the SET and as far as the removing the last character, I think that there was something being returned at the end of the string other than the trailing <br /> \$\endgroup\$ – Malachi Jan 6 '14 at 4:06
  • \$\begingroup\$ I think that I did some Dirty Things to create what I wanted, in other words I agree with you completely. when I get back around to this project again, I will think of about this and see if I can change it. it might tell me why I did this in the first place as well. \$\endgroup\$ – Malachi Feb 15 '14 at 19:36

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.