3
\$\begingroup\$

Please help me to simplify the script below. It has a lot of variables and a lot of values are getting assigned to them. Can I make it simpler?

It has to calculate the percentage of 3 test forms: pre, post and final.

DECLARE @tablefinal TABLE 
(
    RowID1 INT IDENTITY(1,1),
    _CourseIDD int null, 
    _courseName varchar(1000) null,
    _PRE INT NULL, 
    _POST int null, 
    _Feedback int null, 
    _final int NULL, 
    _Per decimal(18,2) null
)

DECLARE @rowCount INT, 
        @currentRow INT, 
        @@@CourseID INT, 
        @@@CourseName varchar (1000)  --,  @STUDENT_ID int = 1078

DECLARE @tableCourse TABLE 
(
    RowID INT IDENTITY(1,1), 
    _COURSE_ID INT NULL, 
    _COURSE_NAME varchar (1000) NULL
)

INSERT INTO @tableCourse (_COURSE_ID , _COURSE_NAME)
SELECT  COURSE_ID, 
        COURSE_NAME
FROM DLC_COURSE C 
WHERE COURSE_ID IN 
( 
    SELECT CS.COURSESHD_COURSE_ID 
    FROM DLC_COURSE_SCHEDULE CS  
    INNER JOIN DLC_STUDENT_PAYMENT SP ON  
        CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID 
    WHERE SP.STUDENT_PAYMENT_STUDENT_ID = @STUDENT_ID 
    AND CS.COURSESHD_STATUS=1      
) 
AND C.COURSE_STATUS=1  

SELECT  @rowCount = @@RowCount, 
        @currentRow = 1

WHILE @currentRow<=@rowCount
BEGIN
    SELECT  @@@CourseID = _COURSE_ID, 
            @@@CourseName = _COURSE_NAME 
    FROM @tableCourse 
    WHERE RowID = @currentRow

    --do activity
    --select @@@CourseID as CID, @@@CourseName as CN
    SET @currentRow = @currentRow + 1

    DECLARE @@Pre INT = 0, 
            @@Course INT = 0, 
            @@Post INT = 0, 
            @@Feedback INT = 0, 
            @@Final INT = 0

    SELECT @@Pre = Count(*)
    FROM DLC_ASSESSMENT_RESULT   
    WHERE RESULT_STUDENT_ID = @STUDENT_ID 
    AND RESULT_COURSE_ID = @@@CourseID  
    AND RESULT_STATUS='Pass'  
    AND RESULT_ASSESSMENT_TYPE=2  

    SELECT @@Course = ISNULL(COUNT(C.COURSE_ID),0)
    FROM DLC_COURSE C     
    WHERE COURSE_ID IN 
    (  
        SELECT CS.COURSESHD_COURSE_ID  
        FROM DLC_COURSE_SCHEDULE CS  
        INNER JOIN DLC_STUDENT_PAYMENT SP 
            ON CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID 
        WHERE SP.STUDENT_PAYMENT_STUDENT_ID = @STUDENT_ID 
        AND CS.COURSESHD_STATUS = 1      
        AND CS.COURSESHD_ENDDATE < GETDATE() 
        AND CS.COURSESHD_COURSE_ID = @@@CourseID   
    ) 
    AND C.COURSE_STATUS=1  

    SELECT @@Post = Count(*) 
    FROM DLC_ASSESSMENT_RESULT   
    WHERE RESULT_STUDENT_ID = @STUDENT_ID 
    AND RESULT_COURSE_ID = @@@CourseID  
    AND RESULT_STATUS = 'Pass'  
    AND RESULT_ASSESSMENT_TYPE = 1  

    SELECT @@Feedback = Count(*)  
    FROM FEEDBACK_RESULTS  
    WHERE QUESTION_ID IN 
    (
        SELECT QUESTION_ID 
        FROM FEEDBACK_QUESTIONS  
        WHERE QUESTION_COURSE = @@@CourseID 
        AND STATUS_ID = 1
    )  
    AND EMP_ID = @STUDENT_ID  

    SELECT @@Final = COUNT(*) 
    FROM DLC_ASSESSMENT_RESULT   
    WHERE RESULT_STUDENT_ID = @STUDENT_ID 
    AND RESULT_COURSE_ID = @@@CourseID  
    AND RESULT_STATUS = 'Pass'  
    AND RESULT_ASSESSMENT_TYPE = 3  

    --declare @@Pre int= 0, @@Course int = 0 , @@Post int  = 0, @@Feedback int = 0  , @@Final  int  = 0

    DECLARE @per decimal (18,2)   

    --select isnull(@@Pre,0), isnull(@@Post,0) , isnull(@@Final,0)
    SET @per = ((ISNULL(@@Pre,0) + ISNULL(@@Post,0) + ISNULL(@@Final,0) ) / 3.00   * 100)

    INSERT INTO @tablefinal
    SELECT  ISNULL(@@Course,0) AS COURSE,  
            @@@CourseName, 
            ISNULL(@@Pre,0) AS PRE,  
            ISNULL(@@Post,0) AS Post,
            ISNULL(@@Feedback,0) AS Feedback,
            ISNULL(@@Final,0) AS Final, 
            @per    
  END

SELECT * 
FROM @TABLEFINAL
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Can you post your execution plan? This is a pretty complicated transaction and being we don't have access to the database it's a bit difficult to pinpoint where it could improve. \$\endgroup\$
    – Phrancis
    Commented May 22, 2014 at 1:16
  • \$\begingroup\$ is this going to be used in a report or application? I am sure that you are trying to do some of this stuff in the SQL that shouldn't be done in the SQL and should be done in the Report or Application. \$\endgroup\$
    – Malachi
    Commented May 22, 2014 at 15:13

2 Answers 2

3
\$\begingroup\$

To sum up what this script does, just for the sake of simplicity:

  1. Declare a result set table variable @tablefinal.

  2. Declare variables for a counter, and for courses, and a commented out @STUDENT_ID (likely for testing purposes).

  3. Declare another result set table variable @tableCourse to hold course IDs and course names during the transaction. This seems to be to filter out courses with no students or not on the schedule.

  4. Set the counter start = 1 and end = total number of rows.

  5. Begin to cycle through @tableCourse at row 1 using WHILE. I noticed that @student_id is not declared or set (other than the instance which is commented out) even though it's being used in multiple WHERE clauses. May be worth your while to look into why it is that way.

  6. Count records for @@pre

  7. Count records for @@courses

  8. Count records for @@post

  9. Count records for @@feedback

  10. Count records for @@final

  11. Declare @per for precentage

  12. Calculate @per with a simple arithmetic operation

  13. Shove everything into your results table @tablefinal and then select from it to see the result set.

To me this really seems like overkill, like a SQL programmer trying to show that he knows how to use variables. I think this could be much more simple, I won't rewrite the whole script but here are some ideas. If you decide to try that approach you will need to start from the ground up, you are welcome to post your new script in a new Code Review if you would like.

DECLARE @pre INT = (Count(*) FROM DLC_ASSESSMENT_RESULT WHERE RESULT_STATUS='Pass'  AND RESULT_ASSESSMENT_TYPE=2);    
-- Repeat DECLARE (Count(*)) for @post, @feedback, @final 

SELECT    
    C.COURSE_ID,     
    C.COURSE_NAME,    
    @pre AS 'Pre',    
    @post AS 'Post',    
    @feedback AS 'Feedback',    
    @final AS 'Final',    
    ((ISNULL(@Pre,0) + ISNULL(@@post,0) + ISNULL(@Final,0) ) / 3.00   * 100) AS 'Pre'

FROM DLC_COURSE C     
WHERE COURSE_ID IN   
-- Taken from the script, seems to be the only important JOIN
(     
    SELECT CS.COURSESHD_COURSE_ID     
    FROM DLC_COURSE_SCHEDULE CS     
    INNER JOIN DLC_STUDENT_PAYMENT SP ON     
        CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID     
    WHERE CS.COURSESHD_STATUS=1         
)     
AND C.COURSE_STATUS=1    
-- This will aggregate the result set by course
GROUP BY C.COURSE_ID;    
\$\endgroup\$
1
\$\begingroup\$

You are working way too hard, because you are treating SQL like a programming language rather than the query language that it was meant to be. Describe the conditions you want, and let SQL Server's query optimizer figure out how to obtain the result set in the most efficient way possible.

The whole thing should just be one query. Something like this, which probably contains some mistakes because there's no way for me to test, but you should get the idea.

WITH EndedCourses AS (
    SELECT CS.COURSESHD_COURSE_ID AS COURSE_ID
        FROM DLC_COURSE_SCHEDULE CS 
        INNER JOIN DLC_STUDENT_PAYMENT AS SP
            ON CS.COURSESHD_ID = SP.STUDENT_PAYMENT_COURSESHD_ID
        WHERE
            CS.COURSESHD_STATUS=1
            AND CS.COURSESHD_ENDDATE < GETDATE()
), Enrollment AS (
    SELECT SP.STUDENT_PAYMENT_STUDENT_ID AS STUDENT_ID
         , COURSE_ID
         , COURSE_NAME
        FROM DLC_COURSE C
        WHERE C.COURSE_STATUS = 1
), PassCount AS (
    SELECT RESULT_STUDENT_ID AS STUDENT_ID
         , RESULT_COURSE_ID AS COURSE_ID
         , COUNT(CASE RESULT_ASSESSMENT_TYPE WHEN 2 THEN 1) AS PRE
         , COUNT(CASE RESULT_ASSESSMENT_TYPE WHEN 1 THEN 1) AS POST
         , COUNT(CASE RESULT_ASSESSMENT_TYPE WHEN 3 THEN 1) AS FINAL
        FROM DLC_ASSESSMENT_RESULT
            WHERE RESULT_STATUS = 'Pass'  
        GROUP BY RESULT_STUDENT_ID, RESULT_COURSE_ID
), Feedback AS (
    SELECT FEEDBACK_RESULTS.EMP_ID AS STUDENT_ID
         , FEEDBACK_QUESTIONS.QUESTION_COURSE AS COURSE_ID
         , COUNT(*) AS FEEDBACK_COUNT
        FROM FEEDBACK_RESULTS
            INNER JOIN FEEDBACK_QUESTIONS
                ON FEEDBACK_QUESTIONS.QUESTION_ID = FEEDBACK_RESULTS.QUESTION_ID
        WHERE FEEDBACK_QUESTIONS.STATUS_ID = 1
        GROUP BY FEEDBACK_RESULTS.EMP_ID, FEEDBACK_QUESTIONS.QUESTION_COURSE
)
SELECT EndedCourses.COURSE_ID AS COURSE_ID
     , COURSE_NAME
     , PRE
     , POST
     , FEEDBACK_COUNT
     , FINAL
     , CAST((PRE + POST + FINAL) / 3.00 * 100 AS DECIMAL(18, 2)) AS PER
    FROM EndedCourses
        INNER JOIN Enrollment
            ON Enrollment.COURSE_ID = EndedCourses.COURSE_ID
        INNER JOIN PassCount
            ON PassCount.STUDENT_ID = Enrollment.STUDENT_ID
            ON PassCount.COURSE_ID = Enrollment.COURSE_ID
        INNER JOIN Feedback
            ON Feedback.STUDENT_ID = Enrollment.STUDENT_ID
            ON Feedback.COURSE_ID = Enrollment.COURSE_ID
    WHERE Enrollment.STUDENT_ID = @STUDENT_ID;
\$\endgroup\$
1
  • \$\begingroup\$ I think this should be marked as the best answer. Nice and clean, easy to understand and none of those nasty variables and WHILE loops. \$\endgroup\$
    – Phrancis
    Commented May 22, 2014 at 21:33

Your Answer

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