Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

There was a lot going on in this Database that I couldn't change, a lot of bad practices because the data was originally being pulled from an old mainframe database and then transformed by a horrible looking VB service and inserted into several (horrible) tables in an SQL Database so the information could be used by a VB.NET website (1.1 or something like that?)

I would like to know how clean this code is and whether or not there is anything that can be done to further my knowledge in retrieving information from Legacy Data stores.

This was an Ad-Hoc, and from the looks of the first Where clause it would be a very interesting Stored Procedure to say the least.

WITH Table1 AS
(
    SELECT 
        CaseParticipantTable.DOCK_NO AS CaseNbr, ActivityTable.JUDGMENT_DATE AS JudgmentDate, ActivityTable.AMOUNT_TYPE AS AmountType, 
            ActivityTable.AMOUNT AS JudgmentAmount, CaseParticipantTable.FILING_NAME AS CaseParticipant
    FROM
        UJSJudgementQuery.dbo.CJ_SC_CASE_PARTICIPANT AS CaseParticipantTable
        INNER JOIN UJSJudgementQuery.dbo.CJ_SC_JUDGMENT_ACTIVITY AS ActivityTable ON CaseParticipantTable.DOCK_NO = ActivityTable.DOCK_NO
    WHERE CaseParticipantTable.FILING_NAME IN ('/CountyName COUNT AUDITORS OFFICE', '/CountyName COUNTY AUDITOR', 
                    '/CountyName COUNTY AUDITOR''S OFFI', '/CountyName COUNTY AUDITORS OFFI', '/CountyName COUNTY AUDITORS OFFIC', 
                    '/CountyName COUNTY AUDITORS OFFICE', '/CountyName COUNTY TREASURER', '/CountyName County Treasurer', 
                    '/CountyName COUNTY/ AUDITOR', '/CountyName COUNTY/AUDITORS OFFIC', '/CountyName COUNTY C/O AUDITORS') 
        AND AMOUNT > 0
), ActiveJudgments AS (
    SELECT Table1.*, Judgments.JUDGMENT_STATUS
    FROM
        Table1
        INNER JOIN UJSJudgementQuery.dbo.CJ_SC_JUDGMENTS AS Judgments ON Table1.CaseNbr = Judgments.DOCK_NO
    WHERE Judgments.JUDGMENT_STATUS = 'A'
), NoInactives AS (
    SELECT ActiveJudgments.*
    FROM ActiveJudgments
    WHERE ActiveJudgments.CaseNbr NOT IN (SELECT Table1.CaseNbr 
                                            FROM 
                                                Table1 
                                                INNER JOIN UJSJudgementQuery.dbo.CJ_SC_JUDGMENTS AS Judgments ON Table1.CaseNbr = Judgments.DOCK_NO
                                            WHERE Judgments.JUDGMENT_STATUS = 'I')
)
SELECT DISTINCT 
    NoInactives.CaseNbr
    , CaseParticipantTable.FILING_NAME AS Debtor
    , NoInactives.JudgmentDate
    , NoInactives.JudgmentAmount
    , NoInactives.JUDGMENT_STATUS
FROM 
    NoInactives
    INNER JOIN UJSJudgementQuery.dbo.CJ_SC_CASE_PARTICIPANT AS CaseParticipantTable ON NoInactives.CaseNbr = CaseParticipantTable.DOCK_NO
WHERE (CaseParticipantTable.PARTICIPANT_CODE = 'DBT' OR CaseParticipantTable.PARTICIPANT_CODE = 'DEF')
ORDER BY Debtor ASC

The ORDER BY at the end is just for checking my data, it is either by date or by Debtor Name


I am looking for suggestions on future query writing

share|improve this question

3 Answers 3

up vote 6 down vote accepted

Obviously table1 is not a very good name, so I'll leave that as that. The other thing that immediately jumps at me is the poor formatting.

It's hard to read multiple items strung across a single line. Doubly when they're aliased.

SELECT 
    CaseParticipantTable.DOCK_NO AS CaseNbr, ActivityTable.JUDGMENT_DATE AS JudgmentDate, ActivityTable.AMOUNT_TYPE AS AmountType, 
        ActivityTable.AMOUNT AS JudgmentAmount, CaseParticipantTable.FILING_NAME AS CaseParticipant

Is easier to read like this:

SELECT 
    CaseParticipantTable.DOCK_NO AS CaseNbr, 
    ActivityTable.JUDGMENT_DATE AS JudgmentDate, 
    ActivityTable.AMOUNT_TYPE AS AmountType, 
    ActivityTable.AMOUNT AS JudgmentAmount, 
    CaseParticipantTable.FILING_NAME AS CaseParticipant

Similarly, I don't like having the entire JOIN...ON statement on one line. I usually put the ON statement on the next line, indented one tab.

FROM
    UJSJudgementQuery.dbo.CJ_SC_CASE_PARTICIPANT AS CaseParticipantTable
    INNER JOIN UJSJudgementQuery.dbo.CJ_SC_JUDGMENT_ACTIVITY AS ActivityTable 
        ON CaseParticipantTable.DOCK_NO = ActivityTable.DOCK_NO

I do like how you start an indented new line after keywords, but you don't do it consistently. This WHERE suffers from that. I also recommend a new line for the items in your in List.

WHERE 
    CaseParticipantTable.FILING_NAME IN (
                                    '/CountyName COUNT AUDITORS OFFICE', 
                                    '/CountyName COUNTY AUDITOR', 
                                    '/CountyName COUNTY AUDITOR''S OFFI', 
                                    '/CountyName COUNTY AUDITORS OFFI', 
                                    '/CountyName COUNTY AUDITORS OFFIC', 
                                    '/CountyName COUNTY AUDITORS OFFICE', 
                                    '/CountyName COUNTY TREASURER', 
                                    '/CountyName County Treasurer', 
                                    '/CountyName COUNTY/ AUDITOR', 
                                    '/CountyName COUNTY/AUDITORS OFFIC', 
                                    '/CountyName COUNTY C/O AUDITORS') 
    AND AMOUNT > 0

You may have your reasons for it, but there's a lot of duplication in your IN statement. Consider whether or not a LIKE...OR...IN type statement would meet your needs. It will perform worse, so test it against your original to see how bad the performance hit will be.

WHERE 
    CaseParticipantTable.FILING_NAME LIKE '/CountyName%COUNT%AUDITOR%'
    OR CaseParticipantTable.FILING_NAME IN(
                                    '/CountyName COUNTY TREASURER', 
                                    '/CountyName County Treasurer') 

This is an adhoc query now, but many many adhoc queries make their way into production use. Never should you Select * in a production query. It often forces a table scan where a more efficient query might be found otherwise.

share|improve this answer
2  
SELECT * is in general bad, but it's not as problematic when it's SELECT CTE.*. We should still write it out however, so if someone ever tinkers one they have to explicitly change the other to be sure they know what they're doing. So it should be listed out for readability and maintenance, but here it's not actually a performance problem. –  nhgrif Aug 12 at 16:44

When I have multiple CTEs, I prefer this indentation style:

WITH
    CTE1 AS (
        /* query */
    ),

    CTE2 AS (
        /* query */
    )

    CTE3 AS (
        /* query */
    )

SELECT *
FROM CTE3

It helps make a clear visual separation between the CTEs that should help enforce the logical separation between them.

share|improve this answer
    
I like your indentation style, but not the newlines in between the CTE's, I say this because the CTE's are technically one query you can't Query the Select without any portion of the CTE chain. Thank you for your Input, I utilized the indentation when I saved the SQL Query to text in the case that I need it in the future. –  Malachi Aug 13 at 13:33

Formatting

Apparently, @ckuhn203 beat me to all the good points while I was reformatting the query to make it easier to read. So, I figured I would just post the whole script with my formatting method. Here goes:

WITH Table1 AS
(
    SELECT 
        CaseParticipantTable.DOCK_NO AS CaseNbr, 
        ActivityTable.JUDGMENT_DATE AS JudgmentDate, 
        ActivityTable.AMOUNT_TYPE AS AmountType, 
        ActivityTable.AMOUNT AS JudgmentAmount, 
        CaseParticipantTable.FILING_NAME AS CaseParticipant
    FROM
        UJSJudgementQuery.dbo.CJ_SC_CASE_PARTICIPANT AS CaseParticipantTable
    INNER JOIN 
        UJSJudgementQuery.dbo.CJ_SC_JUDGMENT_ACTIVITY AS ActivityTable 
        ON CaseParticipantTable.DOCK_NO = ActivityTable.DOCK_NO
    WHERE CaseParticipantTable.FILING_NAME IN (
        '/CountyName COUNT AUDITORS OFFICE', 
        -- '/CountyName COUNTY AUDITOR%'         -- in LIKE clause
        -- '/CountyName COUNTY AUDITOR''S OFFI', -- in LIKE clause
        -- '/CountyName COUNTY AUDITORS OFFI',   -- in LIKE clause
        -- '/CountyName COUNTY AUDITORS OFFIC',  -- in LIKE clause
        -- '/CountyName COUNTY AUDITORS OFFICE', -- in LIKE clause
        '/CountyName COUNTY TREASURER', 
        '/CountyName County Treasurer', 
        '/CountyName COUNTY/ AUDITOR', 
        '/CountyName COUNTY/AUDITORS OFFIC', 
        '/CountyName COUNTY C/O AUDITORS') 
    OR CaseParticipantTable.FILING_NAME LIKE '/CountyName COUNTY AUDITOR%'
    AND AMOUNT > 0
), ActiveJudgments AS (
    SELECT 
        Table1.*, 
        Judgments.JUDGMENT_STATUS
    FROM
        Table1
    INNER JOIN 
        UJSJudgementQuery.dbo.CJ_SC_JUDGMENTS AS Judgments 
        ON Table1.CaseNbr = Judgments.DOCK_NO
    WHERE Judgments.JUDGMENT_STATUS = 'A'
), NoInactives AS (
    SELECT ActiveJudgments.*
    FROM ActiveJudgments
    WHERE ActiveJudgments.CaseNbr NOT IN (
        SELECT Table1.CaseNbr 
        FROM Table1 
    INNER JOIN 
        UJSJudgementQuery.dbo.CJ_SC_JUDGMENTS AS Judgments 
        ON Table1.CaseNbr = Judgments.DOCK_NO
    WHERE Judgments.JUDGMENT_STATUS = 'I')
)
SELECT DISTINCT 
    NoInactives.CaseNbr
    , CaseParticipantTable.FILING_NAME AS Debtor
    , NoInactives.JudgmentDate
    , NoInactives.JudgmentAmount
    , NoInactives.JUDGMENT_STATUS
FROM 
    NoInactives
INNER JOIN 
    UJSJudgementQuery.dbo.CJ_SC_CASE_PARTICIPANT AS CaseParticipantTable 
    ON NoInactives.CaseNbr = CaseParticipantTable.DOCK_NO
WHERE (CaseParticipantTable.PARTICIPANT_CODE = 'DBT' 
OR CaseParticipantTable.PARTICIPANT_CODE = 'DEF')
ORDER BY Debtor ASC
share|improve this answer
    
I have been trying to steer away from LIKE statements in the where clause, I think that in this case it will cause a performance decrease as opposed to those 5 cases being in the IN statement, but I couldn't say that for sure. most of these tables are missing Primary Keys and are not Indexed the Database came from a time before, and I am afraid that I don't understand enough to make the CaseNbr(DOCK_NO) a Primary Key without it Hurting the whole system. –  Malachi Aug 13 at 13:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.