2
\$\begingroup\$

I am trying to write an SQL Procedure that will calculate the Last Price paid for a certain Part.

DECLARE @ItemId int;

SET @ItemId = 1;

SELECT
SD.Price
FROM SalesOrderHeaders AS SH
INNER JOIN SalesOrderDetails AS SD
ON SH.Id = SD.HeaderId
WHERE (SD.LineItemId = @ItemId)
AND (SH.OrderDate = (SELECT
    MAX(SH.OrderDate) AS Expr1
FROM SalesOrderHeaders AS SH
INNER JOIN SalesOrderDetails AS SD
    ON SH.Id = SD.HeaderId
WHERE (SD.LineItemId = @ItemId))
)

This does produce the results I wanted but, I am doing everything twice. Would someone point me in a better direction?

\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

Considering that your subselect is basically the same as your main select, chances are that this subselect is indeed a bit of overkill. Looking at your logic, what you want to achieve is to get the Price of the SalesOrderDetail linked to the SalesOrderHeader with the most recent OrderDate, so let's use an ORDER BY DESC and select only the first answer.

I think this should do the trick:

SELECT TOP 1 SD.Price
FROM SalesOrderHeaders AS SH
INNER JOIN SalesOrderDetails AS SD
    ON SH.Id = SD.HeaderId
WHERE SD.LineItemId = @ItemId
ORDER BY SH.OrderDate DESC
\$\endgroup\$
1
  • \$\begingroup\$ That is so simple, I am ashamed I missed it. \$\endgroup\$
    – Randy
    Commented Jan 5, 2016 at 15:17
3
\$\begingroup\$

@BCdotWEB already provided a good solution, so I will mention some other ways to improve your code.


Your formatting makes your query difficult to read. Try using some indentation and advantageously placed line breaks to give it a better visual structure. You also have unnecessary round brackets around each expression in your WHERE clauses, which distract from the query logic. Here is how I would format it:

DECLARE @ItemId int;

SET @ItemId = 1;

SELECT
    SD.Price
FROM SalesOrderHeaders AS SH
    INNER JOIN SalesOrderDetails AS SD
        ON SH.Id = SD.HeaderId
WHERE SD.LineItemId = @ItemId
AND SH.OrderDate = (
    SELECT
        MAX(SH.OrderDate) AS Expr1
    FROM SalesOrderHeaders AS SH
        INNER JOIN SalesOrderDetails AS SD 
            ON SH.Id = SD.HeaderId
    WHERE SD.LineItemId = @ItemId
);

You are using the same aliases for your outer and inner queries, which could potentially cause ambiguous queries, and also make the code more difficult to understand. Furthermore, SH and SD do not make for very good aliases, as they do not say anything about what they refer to. We could rename them to hdr and dtl, which are only one extra char, but actually give you a visual aid to see what it is referring to.

Now we have a query that reads easier. Note I added the last prefix to your inner aliases, and renamed column alias Expr1 (which is meaningless) to lastOrderDate.

SELECT
    dtl.Price
FROM SalesOrderHeaders AS hdr
    INNER JOIN SalesOrderDetails AS dtl
        ON hdr.Id = dtl.HeaderId
WHERE dtl.LineItemId = @ItemId
AND hdr.OrderDate = (
    SELECT
        MAX(lastHdr.OrderDate) AS lastOrderDate
    FROM SalesOrderHeaders AS lastHdr
        INNER JOIN SalesOrderDetails AS lastDtl 
            ON lastHdr.Id = lastDtl.HeaderId
    WHERE lastDtl.LineItemId = @ItemId
);
\$\endgroup\$
3
\$\begingroup\$

pre: this question sound more fitting for "Stack overflow", than "Code Review"

how about (so often 'underused') group by / having with min/max condition?

BCdotWEBs solution is very fine if you have already one @ItemId

if you want the latest prices for many/all @ItemIds then working with group by / having can be very performant, because you only need one query and not one for each @ItemId

I am not sure if I understood your DB design, but the following should do the trick:

SELECT SD.LineItemId, SD.Price
    FROM SalesOrderHeaders AS SH
    INNER JOIN SalesOrderDetails AS SD
        ON SH.Id = SD.HeaderId
---WHERE SD.LineItemId = @ItemId
    group by 
        SD.LineItemId
    having 
        SH.OrderDate = MAX(SH.OrderDate) 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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