Suggest function code improvement...?

Moving from Access and trying to get acclimated to working with UDFs and
SPs in SQL Server. Wondered if anyone could share any tips or suggestions
on improving (i.e. 'efficiency') the UDF below. It works, and returns the
correct results (a number, a percentage), but it seems that there should be
a 'better' way to do this. CAST was added when I realized that I was
dividing an integer by an integer (and of course getting a result of 0).

Any ideas, tips, suggestions are welcome.

*******
ALTER         FUNCTION dbo.udf_MyFunction
 (@ClassTypeRequested
 varchar(255))
RETURNS TABLE
AS
RETURN ( SELECT      COUNT(UnitInDatabase
) AS  CountOfUnitInDatabas
e,
(SELECT      CAST(COUNT(UnitInDat
abase) AS NUMERIC)
AS  CountOfUnitInDatabas
e
FROM          dbo. vw_EDB_Current_Inven
tory
WHERE      CLASS_TYPE = @ClassTypeRequested AND
Active = 1 AND CurrentlyInStock = 1 AND DATEDIFF(d, AddedToInventory,
 CurrentDate)
> 90 AND DATEDIFF(d,
AddedToInventory, CurrentDate) <= 120) /
(SELECT      COUNT(UnitInDatabase
) AS
 CountOfUnitInDatabas
e
FROM          dbo. vw_EDB_Current_Inven
tory
WHERE      CLASS_TYPE = @ClassTypeRequested AND
Active = 1 AND CurrentlyInStock = 1) AS Expr1
FROM         dbo. vw_EDB_Current_Inven
tory
WHERE     (CLASS_TYPE =  @ClassTypeRequested)
 AND (Active = 1) AND
(CurrentlyInStock = 1) AND (DATEDIFF(d, AddedToInventory, CurrentDate) > 90)
AND
(DATEDIFF(d, AddedToInventory, CurrentDate) <= 120) )
*******

--
Message posted via http://www.webservertalk.com

Instead of having us reverse engineer your existing code, can you show us
your sample data and desired output (see my sig)?

--
Please post DDL, sample data and desired results.
See http://www.aspfaq.com/5006 for info.




"The Gekkster via webservertalk.com" <forum@webservertalk.com> wrote in message
 news:b670d83aa1184b0
 7960a69e4f65a87d0@SQ
webservertalk.com...
> Moving from Access and trying to get acclimated to working with UDFs and
> SPs in SQL Server. Wondered if anyone could share any tips or suggestions
> on improving (i.e. 'efficiency') the UDF below. It works, and returns the
> correct results (a number, a percentage), but it seems that there should
be
> a 'better' way to do this. CAST was added when I realized that I was
> dividing an integer by an integer (and of course getting a result of 0).
>
> Any ideas, tips, suggestions are welcome.
>
> *******
> ALTER         FUNCTION dbo.udf_MyFunction
>  (@ClassTypeRequested
 varchar(255))
> RETURNS TABLE
> AS
> RETURN ( SELECT      COUNT(UnitInDatabase
) AS  CountOfUnitInDatabas
e,
>                           (SELECT      CAST(COUNT(UnitInDat
abase) AS
 NUMERIC)
> AS  CountOfUnitInDatabas
e
>                             FROM          dbo. vw_EDB_Current_Inven
tory
>                             WHERE      CLASS_TYPE =  @ClassTypeRequested[
/color]
AND
> Active = 1 AND CurrentlyInStock = 1 AND DATEDIFF(d, AddedToInventory,
> CurrentDate) 
> AddedToInventory, CurrentDate) <= 120) /
>                           (SELECT      COUNT(UnitInDatabase
) AS
>  CountOfUnitInDatabas
e
>                             FROM          dbo. vw_EDB_Current_Inven
tory
>                             WHERE      CLASS_TYPE =  @ClassTypeRequested[
/color]
AND
> Active = 1 AND CurrentlyInStock = 1) AS Expr1
> FROM         dbo. vw_EDB_Current_Inven
tory
> WHERE     (CLASS_TYPE =  @ClassTypeRequested)
 AND (Active = 1) AND
> (CurrentlyInStock = 1) AND (DATEDIFF(d, AddedToInventory, CurrentDate) >
90)
> AND
>                       (DATEDIFF(d, AddedToInventory, CurrentDate) <=
120) )
> *******
>
> --
> Message posted via http://www.webservertalk.com

Hi Aaron,

The posted function, run against this view:

*******
SELECT     dbo.tblUnits.UnitInDatabase, dbo.tblRetailers.Active,
dbo.tblUnits.CurrentlyInStock, dbo.SQUISH_V2.CLASS_TYPE,
dbo.tblUnits.AddedToInventory,
dbo.tblUnits. RemovedFromInventory
, dbo.tblRetailers.Name, GETDATE() AS
CurrentDate,
dbo.tblRetailers.ZoneID, dbo.tblUnits.W_Price,
dbo.tblUnits.R_Price
FROM         dbo.tblRetailers INNER JOIN
dbo.tblUnits ON dbo.tblRetailers.RetailerID =
dbo.tblUnits.RetailerID INNER JOIN
dbo.SQUISH_V2 ON dbo.tblUnits.Style_ID =
dbo.SQUISHV2.EDB_STYLE_ID
*******

Produces the following result:

 CountOfUnitInDatabas
e     544
Expr2     0.1574551645

The purpose of the function is to (i) quantify the number of units within
the specified CLASS_TYPE that fall within the date (aging) range of '>90
AND <=120'; and (ii) to 'compare' this number against the total of the
designated CLASS_TYPE (expressed as a percentage).

Does this help? Thanks.

--
Message posted via http://www.webservertalk.com

First, I don't think you should use GETDATE() within the view.
(See http://www.aspfaq.com/2439 for an illustration.)

Second, what are the datatypes in the table?  Do any of them allow NULLs
(e.g. UnitInDatabase)?

Third, do you really want the numbers to change throughout the day?  By
using GETDATE() instead of a date at midnight, your 90-day window will move
and will potentially cause a query run at 9:00 AM to yield different results
than a query run at 6:00 PM.

Fourth, I recommend dropping the redundant tbl prefixes, and making data
element naming more consistent (would anyone guess that Style_ID and
EDB_STYLE_ID mean the same thing?).

Finally, it is not clear if you want to see the total count in the table or
only those who were added 90-120 days ago.  I took a guess.  If it does not
yield the result you want, then PLEASE go back to http://www.aspfaq.com/5006
and review how to post your table structure (CREATE TABLE), sample data
(INSERT), and desired results.  It's tough to re-create your view when we
have absolutely no knowledge of the tables it references or the data
contained within them.


CREATE VIEW dbo. vw_EDB_Current_Inven
tory
AS
SELECT
u.UnitInDatabase,
r.Active,
u.CurrentlyInStock,
s.CLASS_TYPE,
u.AddedToInventory,
u. RemovedFromInventory
,
r.Name,
r.ZoneID,
u.W_Price,
u.R_Price
FROM
dbo.tblRetailers r
INNER JOIN tblUnits u
ON r.RetailerID = u.RetailerID
INNER JOIN SQUISH_V2 s
ON u.Style_ID = s.EDB_STYLE_ID
GO


DECLARE @dt SMALLDATETIME, @sdt SMALLDATETIME, @edt SMALLDATETIME
SET @dt = DATEADD(DAY, 0, DATEDIFF(DAY, 0, GETDATE()))
SET @sdt = @dt - 120
SET @edt = @dt - 90


SELECT
 CountOfUnitInDatabas
e =  COUNT(UnitInDatabase
),
Percentage = SUM
(
CASE WHEN AddedToInventory >= @sdt -- 120 days ago
AND AddedToInventory < @edt -- 90 days ago
THEN 1
ELSE 0 END
) / 1.0 *  COUNT(UnitInDatabase
)
FROM
dbo. vw_EDB_Current_Inven
tory
WHERE
-- CLASS_TYPE = @classTypeRequested AND
Active = 1
AND CurrentlyInStock = 1


--
Please post DDL, sample data and desired results.
See http://www.aspfaq.com/5006 for info.




"The Gekkster via webservertalk.com" <forum@webservertalk.com> wrote in message
 news:67434fa817e3453
 f87c9660ded23e679@SQ
webservertalk.com...
> Hi Aaron,
>
> The posted function, run against this view:
>
> *******
> SELECT     dbo.tblUnits.UnitInDatabase, dbo.tblRetailers.Active,
> dbo.tblUnits.CurrentlyInStock, dbo.SQUISH_V2.CLASS_TYPE,
>                       dbo.tblUnits.AddedToInventory,
> dbo.tblUnits. RemovedFromInventory
, dbo.tblRetailers.Name, GETDATE() AS
> CurrentDate,
>                       dbo.tblRetailers.ZoneID, dbo.tblUnits.W_Price,
> dbo.tblUnits.R_Price
> FROM         dbo.tblRetailers INNER JOIN
>                       dbo.tblUnits ON dbo.tblRetailers.RetailerID =
> dbo.tblUnits.RetailerID INNER JOIN
>                       dbo.SQUISH_V2 ON dbo.tblUnits.Style_ID =
> dbo.SQUISHV2.EDB_STYLE_ID
> *******
>
> Produces the following result:
>
>  CountOfUnitInDatabas
e     544
> Expr2     0.1574551645
>
> The purpose of the function is to (i) quantify the number of units within
> the specified CLASS_TYPE that fall within the date (aging) range of '>90
> AND <=120'; and (ii) to 'compare' this number against the total of the
> designated CLASS_TYPE (expressed as a percentage).
>
> Does this help? Thanks.
>
> --
> Message posted via http://www.webservertalk.com

Thanks, Aaron. This is very helpful as your comments illustrate ideas that
I can apply to similar tasks.

I appreciate the guidelines link on aspfaq.com; I'll be sure to follow this
'process' going forward. After all, it only makes things easier for all
concerned.

--
Message posted via http://www.webservertalk.com