1 2 Previous Next 29 Replies Latest reply on Aug 29, 2013 6:18 AM by Billy~Verreynne

    Code Critique Request

    sitheris

      I have been developing PL/SQL for a few years but still consider myself a novice in some regards.  I am looking for an honest critique of some code I've written from some PL/SQL experts.  I have posted a pastebin link below, of my code. 

       

      I feel that my code is off to a good start, but I'm always looking for ways I can improve it and make it more efficient, maintainable, follow best practices, etc.

       

      Some background of the program - it's an inbound interface for the EBS HR module to change time information in the EBS tables for the incoming records.  For each row processed, a row is printed to the output with a message describing what was done.  More detailed information is printed to the log for each record.  Per the requirements, some messages only get printed to the log, while others are printed to both the log and the output.  Some of the business logic is quite complex, so I'm just wondering if there's anything I can do to simplify the messy parts.

       

      While I am looking for feedback on all parts of the program if possible, some specific areas I'm interested in are:

      • Program structure and flow
      • The procedure PROCESS_TIME_INFO has very complex logic to determine which EBS API to call.  Is there a way to make this simpler?
      • Use of Constants, Global Vars, and Cursors

       

      I really appreciate any feedback that can be given as I try to improve my code and PL/SQL skills.

       

      Thanks!

       

      http://pastebin.com/iee455Lj

        • 1. Re: Code Critique Request
          Sven W.

          Can you post parts of your code here? Especially the ones where you think is trouble. I don't follow any obscure links.

          • 2. Re: Code Critique Request
            sitheris

            I would hardly call pastebin an obscure link, but here's the full code:

             

             

            CREATE OR REPLACE PACKAGE BODY xx_hr_jobs_pos_time_info AS
             
              -- Global Constants for Output Destination
              C_LOG                CONSTANT VARCHAR2(1) := 'L';       -- log only
              C_OUTPUT             CONSTANT VARCHAR2(1) := 'O';       -- output only
              C_OUTPUT_LOG         CONSTANT VARCHAR2(2) := 'OL';      -- output and log
             
              -- Statistics Variables
              g_record_count       NUMBER := 0;
              g_records_skipped    NUMBER := 0;
              g_time_created       NUMBER := 0;
              g_time_updated       NUMBER := 0;
              g_time_end_dated     NUMBER := 0;
              g_time_errors        NUMBER := 0;
             
              -- Exceptions for Rotation Plan and Earning Policy
              rotation_plan_error   EXCEPTION;
              earning_policy_error  EXCEPTION;
              init_time_info_error  EXCEPTION;
             
              CURSOR gc_pos IS
                SELECT pd.ROWID row_id,
                       pd.assign_id,
                       pd.effectivedate,
                       pd.employeenumber,
                       pd.employeelastname,
                       pd.employeefirstname,
                       pd.payroll,
                       pd.flsa,
                       pd.numberhoursperweek,
                       pd.organizationname,
                       paa.assignment_id,
                       paa.assignment_number,
                       paa.effective_start_date,
                       paa.effective_end_date,
                       ppb.name pay_basis_name,
                       ppg.segment5
                  FROM xx_peopleadmin_position_data pd,
                       per_all_assignments_f paa,
                       per_pay_bases ppb,
                       pay_people_groups ppg
                 WHERE pd.assign_id = paa.assignment_id (+)
                   AND pd.effectivedate
                         BETWEEN paa.effective_start_date (+) AND paa.effective_end_date (+)
                   AND paa.pay_basis_id = ppb.pay_basis_id (+)
                   AND paa.people_group_id = ppg.people_group_id (+)
                   AND pd.record_status           = 'S'
                   AND pd.updateincumbent         = 'Yes'
                   AND NVL(pd.time_info_chg,'Y') != 'N'
                 ORDER BY pd.employeelastname;
                     
            /******************************************************************************
              Create Procedure display_log
            ******************************************************************************/
            PROCEDURE display_log
              (p_description     IN VARCHAR2)
            IS
            BEGIN
             
              fnd_file.put_line(fnd_file.LOG, p_description);
             
            EXCEPTION
              WHEN OTHERS THEN
                display_log('Error in display_log: ' || SQLERRM);
               
            END display_log;
             
             
            /******************************************************************************
              Create Procedure display_output
            ******************************************************************************/
            PROCEDURE display_output
              (p_description     IN VARCHAR2)
            IS
            BEGIN
             
              fnd_file.put_line(fnd_file.OUTPUT, p_description);
             
            EXCEPTION
              WHEN OTHERS THEN
                display_log('Error in display_output: ' || SQLERRM);
               
            END display_output;
             
             
            /******************************************************************************
              Create Procedure print_header
            ******************************************************************************/
            PROCEDURE print_header
            IS
             
              -- Variables for out parameters of GET_CONC_REQUEST_DETAIL_VALUES below
              l_conc_request_id          NUMBER         := NULL;
              l_last_ddl_time            DATE;
              l_conc_req_name            VARCHAR2(1000) := NULL;
              l_conc_description         VARCHAR2(1000) := NULL;
              l_request_start_date       DATE;
              l_request_completion_date  DATE;
              l_duration                 VARCHAR2(2000) := NULL;
              l_completion_text          VARCHAR2(2000) := NULL;
              l_oracle_user              VARCHAR2(2000) := NULL;
              l_responsibility_name      VARCHAR2(2000) := NULL;
             
            BEGIN
             
               xx_common_pkg.GET_CONC_REQUEST_DETAIL_VALUES
                                (  p_package_name              => 'XX_HR_JOBS_POS_TIME_INFO',
                                   p_conc_request_id           => l_conc_request_id,
                                   p_last_ddl_time             => l_last_ddl_time,
                                   p_conc_program_name         => l_conc_req_name,
                                   p_conc_description          => l_conc_description,
                                   p_request_start_date        => l_request_start_date,
                                   p_request_completion_date   => l_request_completion_date,
                                   p_duration                  => l_duration,
                                   p_completion_text           => l_completion_text,
                                   p_oracle_user               => l_oracle_user,
                                   p_responsibility_name       => l_responsibility_name
                                );
             
              /*   Header Information    */
             
              display_output('---------------------------------------------------------------------------------------------------------------------------------');
              --  Lines removed for confidentiality
              display_output('                                                       For '||TO_CHAR(SYSDATE,'DD-MON-RRRR')                                      );
              display_output('                                         Request ID: ' || l_conc_request_id || '   Submitted By: ' || l_oracle_user);
              display_output('---------------------------------------------------------------------------------------------------------------------------------');
              display_output('');
              display_output('Date of Process : '|| TO_CHAR(SYSDATE,'DD-MON-RRRR HH:MI:SS'));
              display_output('');
             
              display_output(RPAD('Employee Name', 35) || RPAD('Assignment', 20) ||  'Message');
              display_output('---------------------------------- ------------------- --------------------------------------------------------------------------');
             
              display_log('---------------------------------------------------------------------------------------------------------------------------------');
              -- Lines removed for confidentiality
              display_log('                                                       For '||TO_CHAR(SYSDATE,'DD-MON-RRRR')                                      );
              display_log('                           Compile Date: ' || l_last_ddl_time || '   Request ID: ' ||l_conc_request_id || '   Submitted By: ' || l_oracle_user              );
              display_log('---------------------------------------------------------------------------------------------------------------------------------');
              display_log('');
              display_log('Date of Process : '|| TO_CHAR(SYSDATE,'DD-MON-RRRR HH:MI:SS'));
              display_log('');
             
            END print_header;
             
             
            /******************************************************************************
              Create Procedure print_record
            ******************************************************************************/
            PROCEDURE print_record
              (p_pos            IN  gc_pos%ROWTYPE
              ,p_msg            IN  VARCHAR2
              ,p_print_target   IN  VARCHAR2)
            IS
             
              l_record             VARCHAR2(1000);
             
            BEGIN
             
              -- Built record for output
              l_record :=    RPAD(   p_pos.employeelastname
                                  || ', '
                                  || p_pos.employeefirstname, 35)
                          || RPAD(p_pos.assignment_number, 20)  
                          || p_msg;
             
              IF p_print_target = C_LOG THEN
                display_log(p_msg);
              ELSIF p_print_target = C_OUTPUT THEN
                display_output(l_record);
              ELSIF p_print_target = C_OUTPUT_LOG THEN
                display_log(p_msg);
                display_output(l_record);
              ELSE
                display_log('PRINT_RECORD: Invalid Print Target');
              END IF;
             
            END print_record;
             
             
            /******************************************************************************
              Create Procedure print_log_header
            ******************************************************************************/
            PROCEDURE print_log_header
              (p_pos            IN  gc_pos%ROWTYPE)
            IS
            BEGIN
             
              -- Print detailed record information in the log
              display_log(' ');
              display_log('++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++');
              display_log('  Record #: ' || g_record_count || ' Processing Salary and Time for: '||p_pos.employeefirstname||' '||p_pos.employeelastname);
              display_log('++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++');
              display_log('  ');
              display_log('Cursor Record Values');
              display_log('-----------------------------------------------');
              display_log('pd.assign_id             : '|| p_pos.assign_id);  
              display_log('pd.effectivedate         : '|| TO_CHAR(p_pos.effectivedate, 'DD-MON-YYYY'));
              display_log('pd.employeenumber        : '|| p_pos.employeenumber);
              display_log('pd.employeelastname      : '|| p_pos.employeelastname);
              display_log('pd.employeefirstname     : '|| p_pos.employeefirstname);
              display_log('pd.flsa                  : '|| p_pos.flsa);
              display_log('pd.numberhoursperweek    : '|| p_pos.numberhoursperweek);
              display_log('pd.organizationname      : '|| p_pos.organizationname);
              display_log('pd.payroll               : '|| p_pos.payroll);
              display_log('paa.assignment_id        : '|| p_pos.assignment_id);
              display_log('paa.assignment_number    : '|| p_pos.assignment_number);
              display_log('paa.effective_start_date : '|| TO_CHAR(p_pos.effective_start_date, 'DD-MON-YYYY'));
              display_log('paa.effective_end_date   : '|| TO_CHAR(p_pos.effective_end_date, 'DD-MON-YYYY'));
              display_log('ppb.pay_basis_name       : '|| p_pos.pay_basis_name);
              display_log('ppg.segment5             : '|| p_pos.segment5);
              display_log('  ');
             
            END print_log_header;
             
             
            /******************************************************************************
              Create Procedure print_log_footer
            ******************************************************************************/
            PROCEDURE print_log_footer
            IS
            BEGIN
             
              display_log('  ');
              display_log('++++++++++++++++++++++++++++++++++++++++++++++ End of record +++++++++++++++++++++++++++++++++++++++++++++++++++++');
              display_log('  ');
             
            END print_log_footer;
             
             
            /******************************************************************************
              Create Procedure print_statistics
            ******************************************************************************/
            PROCEDURE print_statistics
            IS
            BEGIN
             
              IF g_record_count = 0 THEN
                display_output('     - No records were processed -     ');
                display_log   ('     - No records were processed -     ');
              END IF;  
             
              display_output(' ');
              display_output('---------------------------------------------------------------------------------------------------------------------------------');
              display_output('                                       Position Time Information Interface - Statistics                                          ');
              display_output('---------------------------------------------------------------------------------------------------------------------------------');
              display_output(' ');
              display_output('Total Number of Records Selected               : '|| TO_CHAR(g_record_count));
              display_output('Total Number of Records Skipped                : '|| TO_CHAR(g_records_skipped));
              display_output('Total Number of Records Errored                : '|| TO_CHAR(g_time_errors));
              display_output('Total Number of Time Info Records Created      : '|| TO_CHAR(g_time_created));
              display_output('Total Number of Time Info Records Updated      : '|| TO_CHAR(g_time_updated));
              display_output('Total Number of Time Info Records End-Dated    : '|| TO_CHAR(g_time_end_dated));
             
              display_output(' ');
              display_output('---------------------------------------------------------------------------------------------------------------------------------');
              display_output(' ');
              display_output('---------------------------------------------------------------------------------------------------------------------------------');
             
              display_log(' ');
              display_log('---------------------------------------------------------------------------------------------------------------------------------');
              display_log('                                          Position Time Information Interface - Statistics                                       ');
              display_log('---------------------------------------------------------------------------------------------------------------------------------');
              display_log(' ');
              display_log('Total Number of Records Selected               : '|| TO_CHAR(g_record_count));
              display_log('Total Number of Records Skipped                : '|| TO_CHAR(g_records_skipped));
              display_log('Total Number of Records Errored                : '|| TO_CHAR(g_time_errors));
              display_log('Total Number of Time Info Records Created      : '|| TO_CHAR(g_time_created));
              display_log('Total Number of Time Info Records Updated      : '|| TO_CHAR(g_time_updated));
              display_log('Total Number of Time Info Records End-Dated    : '|| TO_CHAR(g_time_end_dated));
             
              display_log(' ');
              display_log('---------------------------------------------------------------------------------------------------------------------------------');
              display_log(' ');
             
            END print_statistics;
             
             
            /******************************************************************************
              Create Function get_rotation_plan
            ******************************************************************************/
            FUNCTION get_rotation_plan
             (p_pos            IN  gc_pos%ROWTYPE)
            RETURN NUMBER
            IS
             
              l_rotation_plan    NUMBER;
             
            BEGIN
             
              IF        p_pos.payroll        = 'Bi-weekly'
                    AND p_pos.pay_basis_name = 'Hourly Rate'
              THEN
               
                l_rotation_plan := 121; -- wage
             
              ELSIF     p_pos.payroll        = 'Bi-weekly'
                    AND p_pos.pay_basis_name = 'Annual Salary'
                    AND p_pos.flsa           = 'Non Exempt'
              THEN
             
                SELECT hrp.id
                  INTO l_rotation_plan
                  FROM hxt_rotation_plans hrp
                 WHERE hrp.name LIKE TRUNC(p_pos.numberhoursperweek) || '% hour week';
                 
              END IF;
             
              IF l_rotation_plan IS NOT NULL THEN
                display_log('GET_ROTATION_PLAN: Rotation plan found: ' || l_rotation_plan);
              END IF;
             
              RETURN l_rotation_plan;
             
            EXCEPTION
              WHEN NO_DATA_FOUND THEN
                display_log('GET_ROTATION_PLAN: Rotation plan not found from hxt_rotation_plans');
                RAISE rotation_plan_error;
               
              WHEN TOO_MANY_ROWS THEN
                display_log('GET_ROTATION_PLAN: Multiple Rotation plans found in hxt_rotation_plans.  Expected 1.');
                l_rotation_plan := NULL;
                RAISE rotation_plan_error;
               
              WHEN OTHERS THEN
                display_log('GET_ROTATION_PLAN: Unhandled exception: ' || SQLERRM);
                l_rotation_plan := NULL;
                RAISE;
                   
            END get_rotation_plan;
             
             
            /******************************************************************************
              Create Function get_earning_policy
            ******************************************************************************/
            FUNCTION get_earning_policy
             (p_pos            IN  gc_pos%ROWTYPE)
            RETURN NUMBER
            IS
             
              l_earning_policy    NUMBER;
             
            BEGIN
             
              IF        p_pos.payroll        = 'Bi-weekly'
                    AND p_pos.pay_basis_name = 'Hourly Rate'
              THEN
               
                l_earning_policy := 86;             -- Wage Earning Policy
             
             
              ELSIF     p_pos.payroll        = 'Bi-weekly'
                    AND p_pos.pay_basis_name = 'Annual Salary'
                    AND p_pos.flsa           = 'Non Exempt'
              THEN
             
                IF p_pos.organizationname LIKE '%WS%' THEN
                 
                  l_earning_policy := 137295085;    -- XX Standard Earning Policy
               
                ELSE
               
                  SELECT hep.id
                    INTO l_earning_policy
                    FROM hxt_earning_policies hep
                   WHERE hep.name LIKE TRUNC(p_pos.numberhoursperweek) || '% hour week';
                     
                END IF;
               
              END IF;
             
              IF l_earning_policy IS NOT NULL THEN
                display_log('GET_EARNING_POLICY: Earning policy found: ' || l_earning_policy);
              END IF;
             
              RETURN l_earning_policy;
             
            EXCEPTION
              WHEN NO_DATA_FOUND THEN
                display_log('GET_EARNING_POLICY: Earning Policy not found from hxt_earning_policies');
                RAISE earning_policy_error;
               
              WHEN TOO_MANY_ROWS THEN
                display_log('GET_EARNING_POLICY: Multiple Earning Policies found in hxt_earning_policies.  Expected 1.');
                l_earning_policy := NULL;
                RAISE earning_policy_error;
               
              WHEN OTHERS THEN
                display_log('GET_EARNING_POLICY: Unhandled exception: ' || SQLERRM);
                l_earning_policy := NULL;
                RAISE;
                   
            END get_earning_policy;
             
             
            /******************************************************************************
              Create Procedure init_time_info
            ******************************************************************************/
            PROCEDURE init_time_info
              (p_pos                      IN  gc_pos%ROWTYPE
              ,p_hxt_id                   OUT hxt_add_assign_info_f.id%TYPE
              ,p_hxt_eff_start_date       OUT hxt_add_assign_info_f.effective_start_date%TYPE
              ,p_hxt_eff_end_date         OUT hxt_add_assign_info_f.effective_end_date%TYPE
              ,p_hep_name                 OUT hxt_earning_policies.name%TYPE
              ,p_rotation_plan            OUT hxt_add_assign_info_f.rotation_plan%TYPE
              ,p_earning_policy           OUT hxt_add_assign_info_f.earning_policy%TYPE
              ,p_shift_diff_policy        OUT hxt_add_assign_info_f.shift_differential_policy%TYPE)
            IS
            BEGIN
             
              SELECT haai.id,
                     TRUNC(haai.effective_start_date),
                     TRUNC(haai.effective_end_date),
                     hep.name,
                     haai.rotation_plan,
                     haai.earning_policy,
                     haai.shift_differential_policy
                INTO p_hxt_id,
                     p_hxt_eff_start_date,
                     p_hxt_eff_end_date,
                     p_hep_name,
                     p_rotation_plan,
                     p_earning_policy,
                     p_shift_diff_policy
                FROM hxt_add_assign_info_f haai,
                     hxt_earning_policies hep
               WHERE haai.earning_policy = hep.id (+)
                 AND haai.assignment_id = p_pos.assign_id
                 AND haai.effective_end_date = (SELECT MAX(t1.effective_end_date)
                                                  FROM hxt_add_assign_info_f t1
                                                 WHERE t1.assignment_id = haai.assignment_id);
                                                 
              display_log('INIT_TIME_INFO: Retrieved the following values...');
              display_log('INIT_TIME_INFO: hxt_id                    : ' || p_hxt_id);
              display_log('INIT_TIME_INFO: hxt_eff_start_date        : ' || TO_CHAR(p_hxt_eff_start_date, 'DD-MON-YYYY'));
              display_log('INIT_TIME_INFO: hxt_eff_end_date          : ' || TO_CHAR(p_hxt_eff_end_date, 'DD-MON-YYYY'));
              display_log('INIT_TIME_INFO: hep_name                  : ' || p_hep_name);
              display_log('INIT_TIME_INFO: rotation_plan             : ' || p_rotation_plan);
              display_log('INIT_TIME_INFO: earning_policy            : ' || p_earning_policy);
              display_log('INIT_TIME_INFO: shift_differential_policy : ' || p_shift_diff_policy);
              display_log('');
             
            EXCEPTION
              WHEN NO_DATA_FOUND THEN
                display_log('INIT_TIME_INFO: No time information exists in hxt_add_assign_info_f.  A new record will be created.');
               
              WHEN TOO_MANY_ROWS THEN
                display_log('INIT_TIME_INFO: Too many rows encountered in hxt_add_assign_info_f. Expected 1.');
                RAISE init_time_info_error;
               
              WHEN OTHERS THEN
                display_log('INIT_TIME_INFO: Unhandled exception: ' || SQLERRM);
                RAISE;
               
            END init_time_info;
             
             
            /******************************************************************************
              Create Procedure end_date_existing_time_info
            ******************************************************************************/
            PROCEDURE end_date_existing_time_info
             (p_id                     IN    NUMBER
             ,p_effective_start_date   IN    DATE
             ,p_effective_end_date     IN    DATE
             ,p_effective_date         IN    DATE
             ,p_msg                    OUT   VARCHAR2
             )
            IS
             
              l_update_count             NUMBER;
             
            BEGIN
             
              display_log('End dating existing time info for haai.id = ' || p_id);
                                 
              UPDATE hxt.hxt_add_assign_info_f haai
                 SET haai.effective_end_date = p_effective_date - 1
               WHERE haai.id = p_id
                 AND haai.effective_start_date = p_effective_start_date
                 AND haai.effective_end_date   = p_effective_end_date;
             
              l_update_count := SQL%ROWCOUNT;
             
              IF l_update_count > 0 THEN
                p_msg            := 'Success: End-dated existing time info. End date set to ' || TO_CHAR(p_effective_date - 1,'DD-MON-YYYY');
                g_time_end_dated := g_time_end_dated + 1;  
              ELSE
                p_msg         := 'ERROR: Tried to end date existing time info, but no rows were updated';
                g_time_errors := g_time_errors + 1;      
              END IF;
             
            EXCEPTION
              WHEN OTHERS THEN
                p_msg       := 'ERROR: Time Information end dating not successful';
                g_time_errors := g_time_errors + 1;
                display_log('END_DATE_EXISTING_TIME_INFO: Unhandled exception: ' || SQLERRM);    
                   
            END end_date_existing_time_info;
             
             
            /******************************************************************************
              Create Procedure create_otlr_add_assign_info
            ******************************************************************************/
            PROCEDURE create_otlr_add_assign_info
             (p_pos                    IN    gc_pos%ROWTYPE
             ,p_rotation_plan          IN    NUMBER
             ,p_earning_policy         IN    NUMBER
             ,p_msg                    OUT   VARCHAR2
             )
            IS
             
              l_id                       NUMBER;
             
            BEGIN
             
              -- Get new id
              SELECT hxt_seqno.NEXTVAL
                INTO l_id
                FROM sys.dual;  
             
              display_log('CREATE_OTLR_ADD_ASSIGN_INFO: Calling hxt_gen_aai.create_otlr_add_assign_info');
              display_log('CREATE_OTLR_ADD_ASSIGN_INFO: p_id (new)               : ' || l_id);
              display_log('CREATE_OTLR_ADD_ASSIGN_INFO: p_effective_start_date   : ' || TO_CHAR(p_pos.effectivedate, 'DD-MON-YYYY'));
              display_log('CREATE_OTLR_ADD_ASSIGN_INFO: p_assignment_id          : ' || p_pos.assign_id);
              display_log('CREATE_OTLR_ADD_ASSIGN_INFO: p_rotation_plan          : ' || p_rotation_plan);
              display_log('CREATE_OTLR_ADD_ASSIGN_INFO: p_earning_policy         : ' || p_earning_policy);
             
              hxt_gen_aai.create_otlr_add_assign_info
                ( p_id                    => l_id
                , p_effective_start_date  => p_pos.effectivedate
                , p_assignment_id         => p_pos.assign_id
                , p_autogen_hours_yn      => 'Y'
                , p_rotation_plan         => p_rotation_plan
                , p_earning_policy        => p_earning_policy
                , p_created_by            => fnd_profile.VALUE('USER_ID')
                , p_creation_date         => SYSDATE
                , p_last_updated_by       => fnd_profile.VALUE('USER_ID')
                , p_last_update_date      => SYSDATE
                , p_last_update_login     => fnd_global.conc_login_id
                );
             
              p_msg := 'Success: Created Time Information record';
              g_time_created := g_time_created + 1;
             
            EXCEPTION
              WHEN OTHERS THEN
                p_msg := 'ERROR: Time Information create not successful';
                g_time_errors := g_time_errors + 1;
                display_log('CREATE_OTLR_ADD_ASSIGN_INFO: Unhandled exception: ' || SQLERRM);
                   
            END create_otlr_add_assign_info;
             
             
            /******************************************************************************
              Create Procedure update_otlr_add_assign_info
            ******************************************************************************/
            PROCEDURE update_otlr_add_assign_info
             (p_pos                    IN    gc_pos%ROWTYPE
             ,p_id                     IN    hxt_add_assign_info_f.id%TYPE
             ,p_eff_start_date         IN    DATE
             ,p_rotation_plan          IN    NUMBER
             ,p_earning_policy         IN    NUMBER
             ,p_msg                    OUT   VARCHAR2
             )
            IS  
            BEGIN
             
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: Calling hxt_gen_aai.update_otlr_add_assign_info');
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_id                     : ' || p_id);
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_effective_date         : ' || TO_CHAR(p_pos.effectivedate, 'DD-MON-YYYY'));
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_effective_start_date   : ' || TO_CHAR(p_eff_start_date, 'DD-MON-YYYY'));
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_assignment_id          : ' || p_pos.assign_id);
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_rotation_plan          : ' || p_rotation_plan);
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_earning_policy         : ' || p_earning_policy);
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_created_by             : ' || fnd_profile.VALUE('USER_ID'));
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_creation_date          : ' || TO_CHAR(SYSDATE, 'DD-MON-YYYY'));
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_last_updated_by        : ' || fnd_profile.VALUE('USER_ID'));
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_last_update_date       : ' || TO_CHAR(SYSDATE, 'DD-MON-YYYY'));
              display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: p_last_update_login      : ' || fnd_global.conc_login_id);
             
              hxt_gen_aai.update_otlr_add_assign_info
                ( p_id                    => p_id
                , p_datetrack_mode        => 'UPDATE_OVERRIDE'
                , p_effective_date        => p_pos.effectivedate
                , p_effective_start_date  => p_eff_start_date
                , p_assignment_id         => p_pos.assign_id
                , p_autogen_hours_yn      => 'Y'
                , p_rotation_plan         => p_rotation_plan
                , p_earning_policy        => p_earning_policy
                , p_created_by            => fnd_profile.VALUE('USER_ID')
                , p_creation_date         => SYSDATE
                , p_last_updated_by       => fnd_profile.VALUE('USER_ID')
                , p_last_update_date      => SYSDATE
                , p_last_update_login     => fnd_global.conc_login_id
                );
             
              p_msg := 'Success: Time Information updated';
              g_time_updated := g_time_updated + 1;
             
            EXCEPTION
              WHEN OTHERS THEN
                p_msg := 'ERROR: Time Information update not successful';
                g_time_errors := g_time_errors + 1;
                display_log('UPDATE_OTLR_ADD_ASSIGN_INFO: Unhandled exception: ' || SQLERRM);
               
            END update_otlr_add_assign_info;
             
             
            /******************************************************************************
              Create Procedure process_time_info
             
              This process will end-date an existing time information row if no longer
              qualifying; or, update an existing row as needed, as well as create a new
              row as needed.  
            ******************************************************************************/
            PROCEDURE process_time_info
              (p_pos             IN  gc_pos%ROWTYPE
              ,p_msg             OUT VARCHAR2
              ,p_print_target    OUT VARCHAR2)  
            IS
             
              -- Constants for Record Messages
              C_OT_MSG                   CONSTANT VARCHAR2(1000) := 'Skipped: Salaried with OT Earning Policy.';
              C_ASGN_NF                  CONSTANT VARCHAR2(1000) := 'Skipped: Oracle Assignment not found.';
              C_ASGN_END_DATED           CONSTANT VARCHAR2(1000) := 'Skipped: Oracle Assignment has been end-dated.';
              C_TIME_INFO_FUTURE_DATE    CONSTANT VARCHAR2(1000) := 'Skipped: Time Information exists with same or future date.';
              C_NO_UPDATE                CONSTANT VARCHAR2(1000) := 'Skipped: Time Information exists but update does not need to be performed.';
              C_JOB_EXEMPT               CONSTANT VARCHAR2(1000) := 'Skipped: Job is exempt; time info not needed.';
              C_ERR_ROTATION_PLAN        CONSTANT VARCHAR2(1000) := 'ERROR: Unable to determine Rotation Policy.';
              C_ERR_EARNING_POLICY       CONSTANT VARCHAR2(1000) := 'ERROR: Unable to determine Earning Policy.';
              C_ERR_INIT_TIME            CONSTANT VARCHAR2(1000) := 'ERROR: Unable to initialize Time Information.';
              C_ERR_OTHERS               CONSTANT VARCHAR2(1000) := 'ERROR: Encountered an error processing time information.';
             
              l_hxt_id                   hxt_add_assign_info_f.id%TYPE;
              l_hxt_eff_start_date       hxt_add_assign_info_f.effective_start_date%TYPE;
              l_hxt_eff_end_date         hxt_add_assign_info_f.effective_end_date%TYPE;
              l_hep_name                 hxt_earning_policies.name%TYPE;
              l_rotation_plan            NUMBER;
              l_earning_policy           NUMBER;
              l_shift_diff_policy        NUMBER;
             
              l_check_rotation_plan      NUMBER;
              l_check_earning_policy     NUMBER;  
             
              l_message                  VARCHAR2(1000);
              l_ot_flag                  VARCHAR2(1) := 'N';
             
              l_pos                      gc_pos%ROWTYPE := p_pos;
             
            BEGIN
             
              -- Make sure assignment is found
              IF l_pos.assignment_id IS NULL THEN
               
                p_msg              := C_ASGN_NF;
                p_print_target     := C_OUTPUT_LOG;
                g_records_skipped  := g_records_skipped + 1;
                RETURN; -- continue to the next cursor record
               
              END IF;
             
              -- Make sure assignment is not end-dated
              IF l_pos.effective_end_date != TO_DATE('31-DEC-4712') THEN
               
                p_msg              := C_ASGN_END_DATED;
                p_print_target     := C_OUTPUT_LOG;
                g_records_skipped  := g_records_skipped + 1;
                RETURN; -- continue to the next cursor record
             
              END IF;
             
              -- Initialize time info variables
              init_time_info
                ( p_pos                      => l_pos
                , p_hxt_id                   => l_hxt_id
                , p_hxt_eff_start_date       => l_hxt_eff_start_date
                , p_hxt_eff_end_date         => l_hxt_eff_end_date
                , p_hep_name                 => l_hep_name
                , p_rotation_plan            => l_rotation_plan
                , p_earning_policy           => l_earning_policy
                , p_shift_diff_policy        => l_shift_diff_policy);
               
              -- Determine the rotation plan and earning policy
              l_check_rotation_plan   := get_rotation_plan(l_pos);
              l_check_earning_policy  := get_earning_policy(l_pos);
             
              IF l_hxt_id IS NOT NULL THEN     -- If a row is returned, proceed with Update logic
             
                /*
                -- Check Oracle assignment end date vs datafile end date
                IF l_hxt_eff_end_date < l_pos.effectivedate THEN
                 
                  p_msg       := 'ERROR: Assignment end date is less than datafile end date. Assignment ID => ' || p_pos.assign_id;
                  g_time_errors := g_time_errors + 1;
                  RETURN; -- continue to the next cursor record
                 
                END IF;
               */
               
                -- Check if Salaried has OT Earning Policy
                IF     l_pos.segment5 =    'Salaried'
                   AND l_pos.flsa     =    'Non Exempt'                                    
                   AND l_hep_name     LIKE '%OT Leave%' THEN
                         
                  l_ot_flag       := 'Y';
                         
                END IF;
               
                -- haai.shift_differential_policy should be NULL
                -- If not, display a warning but continue processing
                IF l_shift_diff_policy IS NOT NULL THEN
               
                  l_message := '**** Warning for ' || p_pos.employeelastname || ', ' || p_pos.employeefirstname ||
                                ': Time Info Shift Differential Policy not null; review manually.';
                  display_output(l_message);
                  display_log('PROCESS_TIME_INFO: ' || l_message);
               
                END IF;
               
                -- Check to see if the time info needs to be end-dated
                IF         l_pos.payroll        = 'Monthly'
                   OR (    l_pos.payroll        = 'Bi-weekly'
                       AND l_pos.pay_basis_name IN ('Goal Payment', 'Unit Pay'))
                   OR (    l_pos.payroll        = 'Bi-weekly'
                       AND l_pos.pay_basis_name = 'Annual Salary'
                       AND l_pos.flsa           = 'Exempt')
                THEN
                 
                  -- Only print this message if the program tries to change the time info
                  IF l_ot_flag = 'Y' THEN      
                    p_msg             := C_OT_MSG;
                    p_print_target    := C_OUTPUT_LOG;
                    g_records_skipped := g_records_skipped + 1;
                    RETURN;    -- continue to the next cursor record      
                  END IF;
               
                  -- End date the exiting time information
                  end_date_existing_time_info
                    ( p_id                   => l_hxt_id
                    , p_effective_start_date => l_hxt_eff_start_date
                    , p_effective_end_date   => l_hxt_eff_end_date
                    , p_effective_date       => l_pos.effectivedate
                    , p_msg                  => p_msg);
                 
                  RETURN;  -- continue to the next cursor record
                     
                END IF;
             
                -- Check to see if the time info needs to be updated
                IF    (    l_pos.payroll        = 'Bi-weekly'
                       AND l_pos.pay_basis_name = 'Hourly Rate')
                   OR (    l_pos.payroll        = 'Bi-weekly'
                       AND l_pos.pay_basis_name = 'Annual Salary'
                       AND l_pos.flsa           = 'Non Exempt')
                THEN
             
                  -- Make sure time information doesn't already exist with same/future date
                  IF         l_hxt_eff_start_date =  TRUNC(l_pos.effectivedate)
                     OR (    l_hxt_eff_end_date   >  TRUNC(l_pos.effectivedate)
                         AND l_hxt_eff_end_date   != TO_DATE('31-DEC-4712'))
                  THEN
                 
                    p_msg             := C_TIME_INFO_FUTURE_DATE;
                    p_print_target    := C_OUTPUT_LOG;
                    g_records_skipped := g_records_skipped + 1;
                    RETURN;   -- continue to the next cursor record
                 
                  END IF;      
                 
                  -- Determine if the update needs to be performed
                  IF    l_check_rotation_plan  != l_rotation_plan
                     OR l_check_earning_policy != l_earning_policy
                     OR l_hxt_eff_end_date     <  l_pos.effectivedate THEN
                     
                    -- Only print this message if the program tries to change the time info
                    IF l_ot_flag = 'Y' THEN      
                      p_msg             := C_OT_MSG;
                      p_print_target    := C_OUTPUT_LOG;
                      g_records_skipped := g_records_skipped + 1;
                      RETURN;    -- continue to the next cursor record      
                    END IF;
                     
                    -- Call hxt_gen_aai.update_oltr_add_assign_info
                    update_otlr_add_assign_info
                      ( p_pos                  => l_pos
                      , p_id                   => l_hxt_id
                      , p_eff_start_date       => l_hxt_eff_start_date
                      , p_rotation_plan        => l_check_rotation_plan
                      , p_earning_policy       => l_check_earning_policy
                      , p_msg                  => p_msg
                      );
                   
                    RETURN;    -- continue to the next cursor record
                 
                  END IF;
                 
                END IF;
               
                -- Record did not meet any criteria above; no action was taken
                IF p_msg IS NULL THEN
                 
                  p_msg              := C_NO_UPDATE;
                  p_print_target     := C_LOG;
                  g_records_skipped  := g_records_skipped + 1;
                  RETURN;    -- continue to the next cursor record
                 
                END IF;
                                 
              ELSE      -- No row found in hxt_add_assign_info_f, so create a new record
             
                IF l_pos.flsa = 'Non Exempt' THEN                                          
               
                  -- Only print this message if the program tries to change the time info
                  IF l_ot_flag = 'Y' THEN      
                    p_msg             := C_OT_MSG;
                    p_print_target    := C_OUTPUT_LOG;
                    g_records_skipped := g_records_skipped + 1;
                    RETURN;    -- continue to the next cursor record      
                  END IF;
               
                  -- Call hxt_gen_aai.create_oltr_add_assign_info
                  create_otlr_add_assign_info
                    ( p_pos            => l_pos
                    , p_rotation_plan  => l_check_rotation_plan
                    , p_earning_policy => l_check_earning_policy
                    , p_msg            => p_msg
                    );
               
                ELSE
               
                  p_msg             := C_JOB_EXEMPT;  
                  p_print_target    := C_LOG;
                  g_records_skipped := g_records_skipped + 1;      
               
                END IF;    
               
                RETURN;     -- continue to the next cursor record
               
              END IF;
             
            EXCEPTION
              WHEN rotation_plan_error THEN
                p_msg           := C_ERR_ROTATION_PLAN;
                p_print_target  := c_output_log;
                g_time_errors   := g_time_errors + 1;
               
              WHEN earning_policy_error THEN
                p_msg           := C_ERR_EARNING_POLICY;
                p_print_target  := c_output_log;
                g_time_errors   := g_time_errors + 1;
               
              WHEN init_time_info_error THEN
                p_msg           := C_ERR_INIT_TIME;
                p_print_target  := c_output_log;
                g_time_errors   := g_time_errors + 1;
               
              WHEN OTHERS THEN
                p_msg           := C_ERR_OTHERS;
                p_print_target  := c_output_log;
                g_time_errors   := g_time_errors + 1;
                display_log('PROCESS_TIME_INFO: Unhandled exception: ' || SQLERRM);    
               
            END process_time_info;
             
             
            /******************************************************************************
             Create Procedure p_main_process
            ******************************************************************************/
            PROCEDURE main_process
              (p_errbuf            OUT     VARCHAR2
              ,p_errcode           OUT     VARCHAR2
              ,p_validate_only     IN      VARCHAR2)
            IS
             
              l_msg                VARCHAR2(255);
              l_print_target       VARCHAR2(2);    
             
            BEGIN
             
              p_errcode       := '0';
              p_errbuf        :=  NULL;
             
              print_header;
                 
              FOR l_pos IN gc_pos LOOP
             
                g_record_count := g_record_count + 1;
             
                -- Print log header
                print_log_header(l_pos);
                 
                -- Process the record and retrieve the status
                process_time_info
                  ( p_pos          => l_pos
                  , p_msg          => l_msg
                  , p_print_target => l_print_target
                  );
               
                -- Print the record and record status in the output
                print_record
                  ( p_pos          => l_pos
                  , p_msg          => l_msg
                  , p_print_target => l_print_target
                  );
               
                -- Print log footer
                print_log_footer;
               
              END LOOP;
             
              -- Print Process Statistics in Output and Log
              print_statistics;
             
              IF SUBSTR(UPPER(p_validate_only),1,1) = 'Y' THEN
                display_log('MAIN_PROCESS: Validate Mode => Changes rolled back.');
                ROLLBACK;
              ELSE
                display_log('MAIN_PROCESS: Changes committed.');
                COMMIT;
              END IF;
             
              IF g_time_errors > 0 THEN
                p_errcode       := '1';
                p_errbuf        := 'Program encountered errors. Please review.';
                display_log('MAIN_PROCESS: Program encountered errors. Please review.');
              ELSE
                p_errcode       := '0';
                p_errbuf        := 'Program completed successfully...';
                display_log('MAIN_PROCESS: Program completed successfully...');
              END IF;
               
            EXCEPTION
              WHEN OTHERS THEN
                p_errcode     := '2';
                p_errbuf      := 'MAIN_PROCESS: Error in HR Jobs Position Time Info Interface. '|| SQLERRM;
                ROLLBACK;
             
            END main_process;
             
            END xx_hr_jobs_pos_time_info;
            /
            
            
            • 3. Re: Code Critique Request
              TPD-Opitz

              I took a brief look at your code and I have a few thoughts on it:

              • spaceing especially empty lines are inconsistent. Use an auto formatter for consistent appearence of your code sins SQL-Develper is not able to prmat PL/SQL I'D recommend Eclipse-IDE with ToadEclipseExtension (which is free), or Toad (which is rather expensive).
              • You're comparing with literal strings. I'd replace them with public defined constants.
              • in line 348 and 358 you use comments to explain literal values. better create constants with usefull names.
                eg:
                -- Make sure assignment is not end-dated
                IF l_pos.effective_end_date != TO_DATE('31-DEC-4712') THEN
                p_msg := C_ASGN_END_DATED;
                p_print_target := C_OUTPUT_LOG;
                g_records_skipped := g_records_skipped + 1;
                RETURN; -- continue to the next cursor record
                END IF;
                
                would be converted to:
                -- somewhere before line 615 
                function is_assignment_end_dated
                ( p_date in date
                , po_msg varchar2
                , po_print_target varchar2) return boolean is 
                begin IF p_date != TO_DATE('31-DEC-4712') THEN
                po_msg := C_ASGN_END_DATED; 
                po_print_target := C_OUTPUT_LOG; 
                g_records_skipped := g_records_skipped + 1; 
                RETURN true; 
                ELSE 
                RETURN FALSE;
                END IF;
                END is_assignment_end_dated;
                -- current line 662
                IF is_assignment_end_dated
                ( p_date => l_pos.effective_end_date
                , po_msg => p_msg
                , po_print_target => p_print_target) then
                return;
                END IF; 
                
              • in PROCESS_TIME_INFO you separate the code by commenst. The lines following comments should better be extracted to procedures/funktion with meaniingfull names.
                eg:
              • You excessively use out parameters. you should prefix the out parameters so that it is visible on the caller side (eg: po_mgs)
              • at line 344 to 367 you set l_erning_policy by two different ways: first programatically and then by a select. I'd either try to solve that by a select only or encapsulate the select in a procedure or function.

               

              hope this was helpfull..

               

              bye

              TPD

              1 person found this helpful
              • 4. Re: Code Critique Request
                sitheris

                Awesome, thanks for your input!

                • 5. Re: Code Critique Request
                  mtefft

                  A few suggestions:

                  • Get rid of your WHEN OTHERS. You are losing (such as the line number where an error occurred) information by having them, and your program will continue merrily along despite repeated errors.
                  • You have some date conversions that do not specify a format.
                  • Any place you log error information, you should log more information to help understand and diagnose the error. For example, instead of this:

                      display_log('PRINT_RECORD: Invalid Print Target'); 

                  do this:

                      display_log('PRINT_RECORD: Invalid Print Target specified:'||p_print_target); 

                  • In process_time_info  you 'return' from multiple places. That is not a good programming practice.
                  1 person found this helpful
                  • 6. Re: Code Critique Request
                    sitheris

                    mtefft wrote:

                     

                    In process_time_info  you 'return' from multiple places. That is not a good programming practice.

                     

                    First, thanks for your input.  I'm aware it's not the most elegant solution with all the RETURNs, but I couldn't think of a better way to do it (I may be overcomplicating things).  The record being processed needs to have several checks run against it, and should be skipped/not processed further, and printed on the log/output with the associated reason if the conditions aren't met.  Do you have any suggestions on how I might get rid of using the RETURN statements in this case?

                    • 7. Re: Code Critique Request
                      Frank Kulash

                      Hi,

                       

                       

                      sitheris wrote:

                       

                      mtefft wrote:

                       

                      In process_time_info  you 'return' from multiple places. That is not a good programming practice.

                       

                      First, thanks for your input.  I'm aware it's not the most elegant solution with all the RETURNs, but I couldn't think of a better way to do it (I may be overcomplicating things).  The record being processed needs to have several checks run against it, and should be skipped/not processed further, and printed on the log/output with the associated reason if the conditions aren't met.  Do you have any suggestions on how I might get rid of using the RETURN statements in this case?

                      Use ELSIF, ELSE and/or CASE statements.  For example:

                       

                       

                      ...

                      BEGIN

                         -- Make sure assignment is found

                         IF l_pos.assignment_id IS NULL THEN

                             p_msg              := C_ASGN_NF;

                             p_print_target     := C_OUTPUT_LOG;

                             g_records_skipped  := g_records_skipped + 1;

                         ELSIF  l_pos.effective_end_date != TO_DATE ( '31-DEC-4712'

                                                                    , 'DD-MON-YYYY'

                                                                    )

                         THEN

                             p_msg              := C_ASGN_END_DATED;

                             p_print_target     := C_OUTPUT_LOG;

                             g_records_skipped  := g_records_skipped + 1;

                        ELSE

                             -- Initialize time info variables

                             init_time_info ( p_pos                      => l_pos      

                             ...

                       

                      Never call TO_DATE with only 1 argument.  Alway pass a 2nd argument, so there's no mistake about what format you mean.
                      Alternatively, use DATE literal, like this:


                      ELSIF  l_pos.effective_end_date != DATE '4712-12-31'

                      • 8. Re: Code Critique Request
                        Billy~Verreynne

                        Neat code - which is a plus.

                         

                        From a standards perspective, my opinion on using uppercase-for-reserved words are fairly well known here. It is silly. And stupid. No such standard exists in any other modern programming language used today. Nor was or is it a standard in the Ada language - which PL/SQL is an implementation/flavour of. The Ada standards are at Ada 95 Quality and Style Guide.

                         

                        If you are concerned with CORRECTLY using scope in PL/SQL, then drop the silly p_ and v_ prefixes. The language supports explicit scope - which makes inventing implicit scope via prefixes a very questionable standard. The silly prefix approach DOES NOT comprehensively address scope issues. THE LANGUAGE DOES. So it really boggles my mind that many advocate a BROKEN standard over a WORKING language feature, when it comes to scope.

                         

                        Basic example of name collisions and how to deal with them using explicit scope referencing in PL/SQL:

                         

                        SQL> create table footab( var1 varchar2(20) );
                        
                        Table created.
                        
                        SQL> insert into footab values( 'TABLE VALUE');
                        
                        1 row created.
                        
                        SQL> commit;
                        
                        Commit complete.
                        
                        SQL> 
                        SQL> create or replace package FooScope as
                          2          var1    varchar2(20) := 'global var1';
                          3  
                          4          procedure DoSomething( var1 varchar2 );
                          5  end;
                          6  /
                        
                        Package created.
                        
                        SQL> 
                        SQL> 
                        SQL> create or replace package body FooScope as
                          2          procedure DoSomething( var1 varchar2 ) is
                          3          -- there are 4 var1 name collisions in this scope:
                          4          -- 1) procedure parameter is var1
                          5          -- 2) cursor parameter is var1
                          6          -- 3) table column is var1
                          7          -- 4) package global is var1
                          8  
                          9          -- using explicit scope definition
                         10                  cursor c(var1 varchar2) is select * from footab f where f.var1 = upper(c.var1);
                         11  
                         12                  colVal  footab.var1%Type;
                         13          begin
                         14                  -- scope is local, so var1 is the parameter - straight forward and simple
                         15                  dbms_output.put_line( 'param='||var1 );
                         16  
                         17                  -- we use explicit scope (and to demonstrate cursor-for-loop scope,
                         18                  -- we use the same name for the for-loop variable as the local cursor
                         19                  -- variable c)
                         20                  for c in(
                         21                          select
                         22                                  f.var1                  as c1,  -- referring to SQL alias
                         23                                  FooScope.var1           as c2   -- referring to package
                         24                          from    footab f
                         25                          where   f.var1 = upper(DoSomething.var1)-- referring to procedure parameter
                         26                  ) loop
                         27                          dbms_output.put_line( 'c1='||c.c1 );
                         28                          dbms_output.put_line( 'c2='||c.c2 );
                         29  
                         30                          -- we refer to the cursor variable (name collision with
                         31                          -- for-loop variable) using explicit scope
                         32                          open DoSomething.c( DoSomething.var1 );
                         33                          fetch DoSomething.c into colVal;
                         34                          close DoSomething.c;
                         35                          dbms_output.put_line( 'param value='||var1 );
                         36                          dbms_output.put_line( 'fetched value='||colVal );
                         37  
                         38                  end loop;
                         39          end;
                         40  end;
                         41  /
                        
                        Package body created.
                        
                        SQL> show errors
                        No errors.
                        SQL> 
                        SQL> 
                        SQL> exec FooScope.DoSomething( 'table value' );
                        param=table value
                        c1=TABLE VALUE
                        c2=global var1
                        param value=table value
                        fetched value=TABLE VALUE
                        
                        PL/SQL procedure successfully completed.
                        
                        SQL> 
                        

                         

                        As can be seen - comprehensive and consistent method using native PL/SQL language features, to address name collisions and scope resolution. UNLIKE the idiotic prefix approach.

                        • 9. Re: Code Critique Request
                          From a standards perspective, my opinion on using uppercase-for-reserved words are fairly well known here.

                          As are mine.

                          It is silly. And stupid. No such standard exists in any other modern programming language used today. Nor was or is it a standard in the Ada language - which PL/SQL is an implementation/flavour of. The Ada standards are at Ada 95 Quality and Style Guide.

                          Not silly or stupid at all. The code posted is MUCH MORE readable than yours and makes the various blocks stand out. During code review one of the first things I want to see is the 'structure' of the code to make sure nothing is missing. When I see 'IF' I want to find the 'ELSE' or 'END IF'.

                           

                          It makes the code easier to follow. Is it necessary? Of course not. Should everyone make it a 'standard'? No. But if someone does make it a standard I certainly don't have a problem with it. There are far more important things in the code to worry about. For example, all of those hard-coded constants. For me that is a far bigger NO-NO than the case of the reserved words but you didn't even mention them.

                          If you are concerned with CORRECTLY using scope in PL/SQL, then drop the silly p_ and v_ prefixes.

                          What? That statement doesn't even make any sense. As you well know 'CORRECTLY using scope' has NOTHING to do with the actual names used or whether those names have prefixes or not.

                           

                          It is ONLY the placement of the names that will determine the scope.

                           

                          Those prefixes are a BIG aid to code reviewers and other developers in dealing with a VERY BIG issue in PL/SQL code:

                          distinguishing between ACTUAL scope and INTENDED scope.

                           

                          The ACTUAL scope is determined strictly by WHERE the name is defined and used.

                           

                          But the INTENDED scope can often not be determined. That is where those 'p_' and 'v_' prefixes are useful. It makes it much easier to identify when the WRONG parameter/variable is being used; that is, when the INTENDED scope differs from the ACTUAL scope.

                           

                          IMHO it is very useful to include such prefixes as part of a standard because it makes the INTENT of the developer known.

                          The language supports explicit scope

                          And on this I wholeheartedly agree. Your code example did not show it but even if a user declares a parameter named 'var1' and then multiple variables named 'var1' in different nested blocks those blocks can be labeled and the correct variable referenced by referencing the correct block.

                           

                          Unfortunately I have seen very little in the way of knowledge or education in the proper way to create, used and label nested blocks.

                          • 10. Re: Code Critique Request
                            Billy~Verreynne

                            rp0428 wrote:

                             

                            Not silly or stupid at all. The code posted is MUCH MORE readable than yours and makes the various blocks stand out. During code review one of the first things I want to see is the 'structure' of the code to make sure nothing is missing. When I see 'IF' I want to find the 'ELSE' or 'END IF'.

                            So then why is reserved-words-in-uppercase not a standard in ANY other language? Must be because they are all wrong, then?

                             

                            I honestly do not see any logic in that reserved-words-in-uppercase can be claimed as a standard in PL/SQL when it not only contravenes standards in all other modern programming languages (from C/C++ to Java to C#), but also contravenes Ada standards - the language that PL/SQL is an implementation of.

                             

                            What makes PL/SQL so different that de-facto standards in use today by programming language, no longer apply?? What makes it so different from Ada, that Ada standards are now no longer relevant??

                             

                            If you are concerned with CORRECTLY using scope in PL/SQL, then drop the silly p_ and v_ prefixes.

                            What? That statement doesn't even make any sense. As you well know 'CORRECTLY using scope' has NOTHING to do with the actual names used or whether those names have prefixes or not.

                            These prefixes ONLY came about because of name collisions between column names (SQL scope) and variables (PL scope).  The problem is variable name and column name matches. And then the following code does not work as intended:

                            create or replace function GetEmpDept( empNo emp.empno%type ) return number is

                              deptID emp.deptno%type;

                            begin

                              select

                                 e.deptno into deptID

                               from emp e

                               where e.empno = empNo;  -- name collision: result is that ALL rows are selected

                              return( deptID );

                            end;

                            The standard "fix" for this, in the majority of PL/SQL code, is to use a prefix for the parameter  or variable name, in order to prevent a name collision.

                             

                            This is the primary reason for the v_ and p_ prefixes in PL/SQL code. And that is what I slammed as PL/SQL allows explicit scope definition in order to address such name collisions.

                             

                             

                            Those prefixes are a BIG aid to code reviewers and other developers in dealing with a VERY BIG issue in PL/SQL code:

                            distinguishing between ACTUAL scope and INTENDED scope.

                            WHAT!? This is the first time EVER I heard of such a thing in programming standards. And I've been around.

                             

                            Intended scope? Useless.

                             

                            Why? Because the scope of a variable or constant or parameter, is not static. Code is not static. Variables becoming parameters, and constants becoming variables, happens as code changes and evolves. All the time.

                             

                            So if the scope was intended to be foo for the variable/parameter, and this is now bar.. so what? What would the intended scope tell me? Historical stuff inside actual code that no longer has ANY relevance? How does this make code better, simpler, elegant? And what about actual change log comments, where history needs to be kept? Or what about a SVN diff between old and new code to show changes at code level??

                             

                            Or if you change your v_name to p_name when a variable becomes a parameter - you need to make MULTIPLE code changes to the code when this happens, in order to substitute the old prefix with the new prefix, when changing that parameter/variable/constant name. If prefixing was not used, then you need to perform a SINGLE change.

                             

                            Standards should be about simplicity. Logical and simple steps to adhere to when designing and writing code. Not sit with a massive standards reference manual looking up what is considered reserved words and need to be in uppercase, and what prefix to use where and when.

                             

                            Simplicity is a great virtue but it requires hard work to achieve it and education to appreciate it.

                            - Dijkstra (1984) On the nature of Computing Science

                            • 11. Re: Code Critique Request
                              sitheris

                              Wow, didn't mean to incite a big debate here.  Bottom line is that upper-case reserved words and p_ & v_ prefixes are coding standards in my department, so nothing can be done about that.  I actually prefer these standards myself, as it makes the code more readable.

                               

                              But anyway, thanks to everyone who provided input, it helped a lot!

                              • 12. Re: Code Critique Request
                                Billy~Verreynne

                                I beg to differ that nothing can be done with archaic standards that are no longer relevant.

                                 

                                I remember when Hungarian notation  was common in C. Today? Microsoft's General Naming Conventions specifically state, do not use Hungarian notation.

                                 

                                Standards change as they, together with the languages, technology and development environments, evolve.

                                 

                                The mere fact that even the vi editor (one of the oldest around) in Linux, supports syntax highlighting for multiple languages and file formats, illustrates this point. And renders the reason for using uppercase for reserved words to increase legibility, null and void.

                                 

                                FWIW, I have been responsible for programming standards in various languages over the years in various development shops. I have first hand experience of just how standards, sensible 10, 20 or 30 years ago, are inane standards today. Reserved-words-in-uppercase dates back to mainframe standards of the 80's. Using variable prefixes for data type and scope, are a form of Hungarian notation that dates back to the 90's.

                                 

                                Such standards are dangerously old and irrelevant.

                                • 13. Re: Code Critique Request
                                  marcusafs

                                  Can we start a new thread discussing naming standards should be started rather than continuing it here?  Can the moderator copy the previous posts by Billy and RP0428 to that thread.

                                  • 14. Re: Code Critique Request
                                    Arild

                                    MarcusAFS wrote:

                                     

                                    Can we start a new thread discussing naming standards should be started rather than continuing it here?  Can the moderator copy the previous posts by Billy and RP0428 to that thread.

                                     

                                    Second that, and let's call it "PL/SQL Coding Standards", and write a document you can request by posting your e-mail address here on the forums

                                     

                                    https://forums.oracle.com/thread/363956?start=0&tstart=0

                                    1 2 Previous Next