6 Replies Latest reply on Jul 23, 2018 4:51 PM by Abdi Jibril

    function with large code -how to make it more efficient.

    Abdi Jibril

      Hi,

      Problem:

      As a PL/SQL newbie, I built the following function that works well.  however, I thought that there has to be a ways to make it more efficient with less code. In other words, we have almost similar code for each cursor, except the parameter we're passing it and there must be ways to have less repeating code.  

       

      Here is the specification:
      At any given time, there will be only 2 parameter that can be passed: id_demo (unique id) and one of these 3 (code_unit, code_dept, nbr_fund, subs).  code_anon and add_spa parameters has the default value and are less likely to change.  

      FUNCTION rel_any
               (
                id_demo          IN donation.id_demo%TYPE
                , code_unit      IN donation.code_unit%TYPE DEFAULT NULL
                , code_dept     IN donation.code_dept%TYPE DEFAULT NULL
                , nbr_fund       IN donation.nbr_fund%TYPE DEFAULT NULL
                , subs              IN donation.sub_cor%TYPE DEFAULT NULL
                , code_anon   IN donation.code_anon%TYPE DEFAULT constant_bb.std_anon
                , add_spa       IN donation.code_recipient%TYPE DEFAULT NULL
               )
                  RETURN donation.amt_fund%TYPE
          IS
        --VARIABLES
        total   donation.amt_fund%TYPE := 0;
      
      
        --CURSOR
      
        --1. all donations
        CURSOR cur_amt IS
      
      
        SELECT amt_fund
        FROM(
               SELECT DISTINCT
                      id.id_rel
                      ,p.nbr_fund
                      ,p.code_exp_type
                      ,p.amt_fund
                      ,p.id_gift
                      ,p.code_pmt_form
                      ,p.code_exp_type
                FROM donation p
                     JOIN master id ON id.id_rel = p.id_rel
               WHERE id.id_demo = rel_any.id_demo
               AND p.code_anon <= rel_any.code_anon
               AND p.code_recipient IN (constant_bb.std_u
                                         ,constant_bb.std_f
                                         ,constant_bb.std_m
                                         ,rel_any.add_spa
                                         ));
      --2. code_unit
        CURSOR cur_unit_amt IS
            SELECT amt_fund
            FROM(
                   SELECT DISTINCT
                          id.id_rel
                          ,p.nbr_fund
                          ,p.code_exp_type
                          ,p.amt_fund
                          ,p.id_gift
                          ,p.code_pmt_form
                          ,p.code_exp_type
                    FROM donation p
                         JOIN master id ON id.id_rel = p.id_rel
                   WHERE id.id_demo = rel_any.id_demo
                   AND p.code_unit = rel_any.code_unit
                   AND p.code_anon <= rel_any.code_anon
                   AND p.code_recipient IN (constant_bb.std_u
                                             ,constant_bb.std_f
                                             ,constant_bb.std_m
                                             ,rel_any.add_spa
                                             ));
      --3. MED
      CURSOR cur_med_amt IS
          SELECT amt_fund
          FROM(
                   SELECT DISTINCT
                          id.id_rel
                          ,p.nbr_fund
                          ,p.code_exp_type
                          ,p.amt_fund
                          ,p.id_gift
                          ,p.code_pmt_form
                          ,p.code_exp_type
                    FROM donation p
                         JOIN master id ON id.id_rel = p.id_rel
                   WHERE id.id_demo = rel_any.id_demo
                   AND p.code_clg = rel_any.code_unit
                   AND p.code_anon <= rel_any.code_anon
                   AND p.code_recipient IN (constant_bb.std_u
                                             ,constant_bb.std_f
                                             ,constant_bb.std_m
                                             ,rel_any.add_spa
                                             ));
      --4. dept
      CURSOR cur_dept_amt IS
          SELECT amt_fund
          FROM(
                   SELECT DISTINCT
                          id.id_rel
                          ,p.nbr_fund
                          ,p.code_exp_type
                          ,p.amt_fund
                          ,p.id_gift
                          ,p.code_pmt_form
                          ,p.code_exp_type
                    FROM donation p
                         JOIN master id ON id.id_rel = p.id_rel
                   WHERE id.id_demo = rel_any.id_demo
                   AND p.code_dept = rel_any.code_dept
                   AND p.code_anon <= rel_any.code_anon
                   AND p.code_recipient IN (constant_bb.std_u
                                             ,constant_bb.std_f
                                             ,constant_bb.std_m
                                             ,rel_any.add_spa
                                             ));
      --5. sub_corridor
      CURSOR cur_subcorr_amt IS
          SELECT amt_fund
          FROM(
                   SELECT DISTINCT
                          id.id_rel
                          ,p.nbr_fund
                          ,p.code_exp_type
                          ,p.amt_fund
                          ,p.id_gift
                          ,p.code_pmt_form
                          ,p.code_exp_type
                    FROM donation p
                         JOIN master id ON id.id_rel = p.id_rel
                   WHERE id.id_demo = rel_any.id_demo
                   AND p.sub_cor = rel_any.subs
                   AND p.code_anon <= rel_any.code_anon
                   AND p.code_recipient IN (constant_bb.std_u
                                             ,constant_bb.std_f
                                             ,constant_bb.std_m
                                             ,rel_any.add_spa
                                             ));
      --6. fund
      CURSOR cur_fund_amt IS
          SELECT amt_fund
          FROM(
                   SELECT DISTINCT
                          id.id_rel
                          ,p.nbr_fund
                          ,p.code_exp_type
                          ,p.amt_fund
                          ,p.id_gift
                          ,p.code_pmt_form
                          ,p.code_exp_type
                    FROM donation p
                         JOIN master id ON id.id_rel = p.id_rel
                   WHERE id.id_demo = rel_any.id_demo
                   AND p.nbr_fund = rel_any.nbr_fund
                   AND p.code_anon <= rel_any.code_anon
                   AND p.code_recipient IN (constant_bb.std_u
                                             ,constant_bb.std_f
                                             ,constant_bb.std_m
                                             ,rel_any.add_spa
                                             ));
      BEGIN
      total:= 0;
      
      
      case when rel_any.code_unit = 'MED' then
                  for i in cur_med_amt
                      loop
                           total:= total + i.amt_fund;
                      end loop;
           when rel_any.code_unit is not null then
                   for i in cur_unit_amt
                        loop
                            total:= total + i.amt_fund;
                        end loop;
           when rel_any.code_dept is not null then
                   for i in cur_dept_amt
                        loop
                            total:= total + i.amt_fund;
                        end loop;
           when rel_any.subs is not null then
                   for i in cur_subcorr_amt
                        loop
                            total:= total + i.amt_fund;
                        end loop;
           when rel_any.nbr_fund is not null then
                   for i in cur_fund_amt
                        loop
                            total:= total + i.amt_fund;
                        end loop;
          else
                  for i in cur_amt
                      loop
                           total:= total + i.amt_fund;
                      end loop;
      end case;
      
      return total;
      END rel_any;
      
      
      
        • 1. Re: function with large code -how to make it more efficient.
          jaramill

          Abdi Jibril wrote:

           

          Here is the specification:

          At any given time, there will be only 2 parameter that can be passed: id_demo (unique id) and one of these 3 (code_unit, code_dept, nbr_fund, subs).

          First you mentioned 3 of these, but you have 4 (four) parameters extra.

           

          You don't need to open a cursor to then loop through each record to get a total. Just use the aggregate function SUM and use it in a straight select statement.  No need for the explicit cursor.  Just use the implicit cursor of the select.

          Also the total function doesn't need to be initialized, now that the SUM function gets it in one shot.

           

          Here's your tuned function.

           

          CREATE OR REPLACE FUNCTION rel_any  
          (  
           id_demo   IN donation.id_demo%TYPE  
          ,code_unit IN donation.code_unit%TYPE      DEFAULT NULL  
          ,code_dept IN donation.code_dept%TYPE      DEFAULT NULL  
          ,nbr_fund  IN donation.nbr_fund%TYPE       DEFAULT NULL  
          ,subs      IN donation.sub_cor%TYPE        DEFAULT NULL  
          ,code_anon IN donation.code_anon%TYPE      DEFAULT constant_bb.std_anon  
          ,add_spa   IN donation.code_recipient%TYPE DEFAULT NULL  
          ) RETURN donation.amt_fund%TYPE AS  
          
             --VARIABLES
             total donation.amt_fund%TYPE;  
          
          BEGIN
             
             <<my_case>>  
             CASE
          
                WHEN rel_any.code_unit = 'MED' THEN
          
                   SELECT SUM(amt_fund)
                     INTO total
                     FROM (
                           SELECT DISTINCT
                                  id.id_rel
                                 ,p.nbr_fund
                                 ,p.code_exp_type
                                 ,p.amt_fund
                                 ,p.id_gift
                                 ,p.code_pmt_form
                                 ,p.code_exp_type
                             FROM donation P
                            INNER JOIN
                                  MASTER ID
                               ON id.id_rel   = p.id_rel
                            WHERE id.id_demo  = rel_any.id_demo
                              AND p.code_clg  = rel_any.code_unit
                              AND p.code_anon <= rel_any.code_anon
                              AND p.code_recipient IN (
                                                       constant_bb.std_u
                                                      ,constant_bb.std_f
                                                      ,constant_bb.std_m
                                                      ,rel_any.add_spa
                                                      )
                          );
            
                WHEN rel_any.code_unit IS NOT NULL THEN
                  
                   SELECT SUM(amt_fund)
                     INTO total
                     FROM (
                           SELECT DISTINCT
                                  id.id_rel
                                 ,p.nbr_fund
                                 ,p.code_exp_type
                                 ,p.amt_fund
                                 ,p.id_gift
                                 ,p.code_pmt_form
                                 ,p.code_exp_type
                             FROM donation P
                            INNER JOIN
                                  MASTER ID
                               ON id.id_rel    = p.id_rel
                            WHERE id.id_demo   = rel_any.id_demo
                              AND p.code_unit  = rel_any.code_unit
                              AND p.code_anon <= rel_any.code_anon
                              AND p.code_recipient IN (
                                                       constant_bb.std_u
                                                      ,constant_bb.std_f
                                                      ,constant_bb.std_m
                                                      ,rel_any.add_spa
                                                      )
                          );
          
                WHEN rel_any.code_dept IS NOT NULL THEN
                
                   SELECT SUM(amt_fund)
                     INTO total
                     FROM (
                           SELECT DISTINCT
                                  id.id_rel
                                 ,p.nbr_fund
                                 ,p.code_exp_type
                                 ,p.amt_fund
                                 ,p.id_gift
                                 ,p.code_pmt_form
                                 ,p.code_exp_type
                             FROM donation P
                            INNER JOIN
                                  MASTER ID
                               ON id.id_rel    = p.id_rel
                            WHERE id.id_demo   = rel_any.id_demo
                              AND p.code_dept  = rel_any.code_dept
                              AND p.code_anon <= rel_any.code_anon
                              AND p.code_recipient IN (
                                                       constant_bb.std_u
                                                      ,constant_bb.std_f
                                                      ,constant_bb.std_m
                                                      ,rel_any.add_spa
                                                      )
                          );
          
                WHEN rel_any.subs IS NOT NULL THEN
                
                   SELECT SUM(amt_fund)
                     INTO total
                     FROM (
                           SELECT DISTINCT
                                  id.id_rel
                                 ,p.nbr_fund
                                 ,p.code_exp_type
                                 ,p.amt_fund
                                 ,p.id_gift
                                 ,p.code_pmt_form
                                 ,p.code_exp_type
                             FROM donation P
                            INNER JOIN
                                  MASTER ID
                               ON id.id_rel  = p.id_rel
                            WHERE id.id_demo = rel_any.id_demo
                                  AND p.sub_cor = rel_any.subs
                                  AND p.code_anon <= rel_any.code_anon
                                  AND p.code_recipient IN (constant_bb.std_u
                                                          ,constant_bb.std_f
                                                          ,constant_bb.std_m
                                                          ,rel_any.add_spa));
            
                WHEN rel_any.nbr_fund IS NOT NULL THEN
                
                   SELECT SUM(amt_fund)
                     INTO total
                     FROM (
                           SELECT DISTINCT
                                  id.id_rel
                                 ,p.nbr_fund
                                 ,p.code_exp_type
                                 ,p.amt_fund
                                 ,p.id_gift
                                 ,p.code_pmt_form
                                 ,p.code_exp_type
                             FROM donation P
                            INNER JOIN
                                  MASTER ID
                               ON id.id_rel    = p.id_rel
                            WHERE id.id_demo   = rel_any.id_demo
                              AND p.nbr_fund   = rel_any.nbr_fund
                              AND p.code_anon <= rel_any.code_anon
                              AND p.code_recipient IN (
                                                       constant_bb.std_u
                                                      ,constant_bb.std_f
                                                      ,constant_bb.std_m
                                                      ,rel_any.add_spa
                                                      )
                          );
            
                ELSE  
                
                   SELECT SUM(amt_fund)
                     INTO total
                     FROM (            
                           SELECT DISTINCT
                                  id.id_rel
                                 ,p.nbr_fund
                                 ,p.code_exp_type
                                 ,p.amt_fund
                                 ,p.id_gift
                                 ,p.code_pmt_form
                                 ,p.code_exp_type
                             FROM donation P
                            INNER JOIN
                                  MASTER ID
                               ON id.id_rel    = p.id_rel
                            WHERE id.id_demo   = rel_any.id_demo
                              AND p.code_anon <= rel_any.code_anon
                              AND p.code_recipient IN (
                                                       constant_bb.std_u
                                                      ,constant_bb.std_f
                                                      ,constant_bb.std_m
                                                      ,rel_any.add_spa
                                                      )
                          );
          
             END CASE my_case;  
          
             RETURN(total);
            
          END rel_any;
          
          • 2. Re: function with large code -how to make it more efficient.
            Sven W.

            They are many things you could improve. I name several ones to consider. I won't give an example for each possible action.

            1) Instead of multiple parameters use a record.

            2) Don't loop and add things. Let the database do this work for you. Jaromil already showed a starting point for that.

            3) All your cursors are equal. You only need one. You can call this one cursor with different cursor parameters.

            4) is the distinct needed? I suspect it is a bug. It might make your query slow down, because of an additional sort operation.

            5) Remove columns that are not needed from the inline select. This has to do with point 4) the DISTINCT.

             


            Here is an example for a parameterized cursor (untested). I'm not sure if I used the correct datatypes.

             

             

            cursor c(cv_id_demo in number, cv_code_unit in varchar2, cv_code_anon in varchar2)
            is

            (SELECT SUM(amt_fund) 

            INTO total 

            FROM ( 

            SELECT DISTINCT  -- get rid of this!

                                    id.id_rel 

                                   ,p.nbr_fund 

                                   ,p.code_exp_type 

                                   ,p.amt_fund 

                                   ,p.id_gift 

                                   ,p.code_pmt_form 

                                   ,p.code_exp_type 

            FROM donation P 

            JOIN  MASTER ID  ON id.id_rel   = p.id_rel 

            WHERE id.id_demo  = cv_id_demo

            AND p.code_clg  = cv_code_unit

            AND p.code_anon <= cv_code_anon 

            AND p.code_recipient IN

                                                         constant_bb.std_u 

                                                        ,constant_bb.std_f 

                                                        ,constant_bb.std_m 

                                                        ,rel_any.add_spa 

                                                        ) 

            )); 

             

            And later you can open the cursor and supply the parameters.

             

            open c(p.id_rel , rel_any.id_demo, rel_any.code_anon);

            fetch c into ...

            1 person found this helpful
            • 3. Re: function with large code -how to make it more efficient.

              As a PL/SQL newbie, I built the following function that works well.

              Welcome to the forum.

               

              As a 'newbie' the MOST IMPORTANT thing to learn about writing code is that functions and procedures should do ONE THING and ONE THING ONLY.

              however, I thought that there has to be a ways to make it more efficient with less code. In other words, we have almost similar code for each cursor, except the parameter we're passing it and there must be ways to have less repeating code.  

              Ok - but 'almost similar code' does NOT mean IDENTICAL code. It is more important for code to be correct, simple and easy to maintain than it is to try to combine 'almost similar' things into one thing.

               

              That 'almost similar code' is being used for DIFFERENT purposes. So you should NOT combine code used for different purposes into one piece since that violates the first principal of coding I mentioned - code should do ONE thing.

               

              Here is the specification:
              At any given time, there will be only 2 parameter that can be passed: id_demo (unique id) and one of these 3 (code_unit, code_dept, nbr_fund, subs).  code_anon and add_spa parameters has the default value and are less likely to change.  

              Well - that is NOT really the 'specification' - that is just the way to chose to implement the specification.

               

              If you have three DIFFERENT things to do then you should create three DIFFERENT functions/procedures - one for each of them.

               

              Then the calling code should call the appropriate one of the three.

               

              You MUST modularize the code so that each code unit does ONE thing.

               

              1. create a new function/procedure for each function needed: code_unit, code_dept, nbr_fund, subs)

              2. modify the calling code to call the appropriate new function/procedure

               

              There could certainly be a 'master' unit that calls 2 or more of those 3 new procs as needed but that is ALL the master unit would do - act as a controller.

               

              This is a snippet of your current code:

              1. case when rel_any.code_unit = 'MED' then 
              2.             for i in cur_med_amt 
              3.                 loop 
              4.                      total:= total + i.amt_fund; 
              5.                 end loop; 
              6.      when rel_any.code_unit is not null then 
              7.              for i in cur_unit_amt 
              8.                   loop 
              9.                       total:= total + i.amt_fund; 
              10.                   end loop; 
              11.  

              The modified version would look more like this (ignore syntax errors - this is pseudo-code)

              1. case when rel_any.code_unit = 'MED' then 
              2.        compute_cur_med_amt
              3.      when rel_any.code_unit is not null then 
              4.            compute_cur_unit_amt
              5.  

              See how the case structure becomes the CONTROLLER of the process? It determines WHAT code gets executed and in WHAT ORDER but does NOT execute the code itself. It CONTROLS the process.

               

              Then the 'compute_cur_med_amt' and 'compute_cur_unit_amt' are NEW procedures. Each of them does ONE THING and does the actual work needed. If you do that you will find you don't need cursors and loops. Each of those procedures can execute ONE SIMPLE query that calculates a sum. Use a function and you can return that sum to the controller.

               

              Your CASE statement has 6 blocks in it so you would:

               

              1. create SIX new functions/procedures.

              2. each new code block would have a SIMPLE requirement that you would document in a comment at the beginning of the block.

              3. each new code block COULD BE assigned to a separate developer. They would know EXACTLY what there block needs to do and could develop and test it WITHOUT REGARD for what the other five developers are doing or what the CONTROLLER code is going to do with the results.

               

              That is an example of how you modularize your code to make it SIMPLE and easy to test and maintain.

               

              Start over and refactor your code into modular units.

              1 person found this helpful
              • 4. Re: function with large code -how to make it more efficient.
                Abdi Jibril
                First you mentioned 3 of these, but you have 4 (four) parameters extra.

                You're right I meant 4.

                • 5. Re: function with large code -how to make it more efficient.

                  You're right I meant 4.

                  So have you begun refactoring your code as I suggested?

                  • 6. Re: function with large code -how to make it more efficient.
                    Abdi Jibril

                    I agree we keep each functionality separately, by modularizing so each function does one thing.  This is the approach we're taking.