HiveBrain v1.2.0
Get Started
← Back to all entries
patternsqlMinor

Small query - removing XML tags

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
removingqueryxmltagssmall

Problem

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, '','')
    SET @Result = replace(@Result, '','')
    Set @Result = LEFT(@Result, LEN(@Result) - 1)

RETURN @Result
END


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

uCode:

```
CREATE TABLE [dbo].uCodeWITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LO

Solution

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


, 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
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 = ?

Code Snippets

SELECT uC.Description
FROM Justice.dbo.uCode uC
INNER JOIN Justice.dbo.xBondCondition xBC
        ON uC.CodeID = xBC.ConditionID
WHERE xBC.BondID = ?

Context

StackExchange Code Review Q#38444, answer score: 8

Revisions (0)

No revisions yet.