1

I'm writing a function that should add each line item quantity multiplied by its unit cost and then iterating through the entire pickticket (PT). I don't get an error when altering the function in SQL Server or when running it, but it gives me a 0 as the output each time.


Here is an example:


[PT 1]

[Line 1 - QTY: 10 Unit Cost: $5.00] total should be = $50.00

[Line 2 - QTY: 5 Unit Cost: $2.50] total should be = $12.50

The function should output - $62.50


Not really sure what I'm missing here, but would appreciate the help.

Alter Function fn_CalculateAllocatedPTPrice
  (@psPickTicket   TPickTicketNo)
  -------------------------------
  Returns TInteger
As 
Begin
  Declare
    @iReturn           TInteger,
    @iTotalLineNumbers TInteger,
    @iIndex            TInteger,
    @fTotalCost        TFloat;

    set @iIndex = 1;
    set @iTotalLineNumbers = (ISNULL((select top 1 PickLineNo
                             from tblPickTicketDtl
                             where PickTicketNo = @psPickTicket
                             order by PickLineNo desc), 0)) /* This returns the highest line number */

    while(@iIndex <= @iTotalLineNumbers)
      BEGIN
        /* This should be adding up the total cost of each line item on the PT */
        set @fTotalCost = @fTotalCost + (ISNULL((select  SUM(P.RetailUnitPrice*P.UnitsOrdered)
                        from tblPickTicketDtl P
                        left outer join tblCase C on (P.PickTicketNo = C.PickTicketNo)
                        where P.PickTicketNo = @psPickTcket
                        and P.PickLineNo = @iIndex
                        and C.CaseStatus in ('A','G','K','E','L','S')), 0))
        set @iIndex = @iIndex + 1;
      END
    set @iReturn = @fTotalCost;
    _Return:
  Return(@iReturn);
End /* fn_CalculateAllocatedPTPrice */
2
  • 2
    What you are missing is that you shouldn't use a WHILE loop in SQL unless you really have to, and you shouldn't use scalar User Defined functions unless you really have to. Why does this need a loop, why can you not just use simple aggregation? What we are missing is proper sample data formatted as a table or as CREATE INSERT statements. Commented Nov 4, 2021 at 15:38
  • 1
    Yes it's fine to encapsulate a process in a function for reusability and readability, ideally this would a table-valued function with no need for any looping - indeed if there is any single place to avoid a RBAR process, it's in a function - or a trigger... or well anywhere really. Commented Nov 4, 2021 at 15:49

1 Answer 1

3

It seems simple aggregation should suffice

A few points to note:

  • WHILE loops and cursors are very rarely needed in SQL. You should stick to set-based solutions, and if you find yourself writing a loop you shuold question your code from its beginnings.
  • Scalar functions are slow and inefficient. Use an inline Table function, which you can correlate with your main query either with an APPLY or a subquery
  • Your left join becomes an inner join because of the where predicate
  • User defined types are not normally a good idea (when they are just aliasing system types)
CREATE OR ALTER FUNCTION fn_CalculateAllocatedPTPrice
  (@psPickTicket   TPickTicketNo)

RETURNS TABLE AS RETURN

SELECT fTotalCost = ISNULL((
    SELECT SUM(P.RetailUnitPrice * P.UnitsOrdered)
    from tblPickTicketDtl P
    join tblCase C on (P.PickTicketNo = C.PickTicketNo)
    where P.PickTicketNo = @psPickTcket
      and C.CaseStatus in ('A','G','K','E','L','S')
    ), 0);

GO
Sign up to request clarification or add additional context in comments.

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.