Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question
up vote 5 down vote accepted

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
share|improve this answer
    
That is so simple, I am ashamed I missed it. – Randy Jan 5 at 15:17

@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
);
share|improve this answer

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) 
share|improve this answer

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.