Enhancement request: Refactoring

[Claus Pedersen]

I have some remarks to the functionality of the new refactoring feature.

I have refactored the following code snippet:

CREATE OR REPLACE PACKAGE BODY refac IS

/*

  FUNCTION test2 RETURN number IS
  BEGIN
    RETURN 2500;
  END;
*/
  PROCEDURE test (empno IN emp.empno%TYPE) IS
    empno_cnst CONSTANT emp.empno%TYPE DEFAULT 1;
  BEGIN
    IF empno = empno_cnst THEN
      /* refactor START */
      UPDATE emp
      SET    sal = 2500
      WHERE  empno = empno_cnst;
      /* refactor END */

    END IF;
  END test;

END refac;

Note that the method test2 is inside a comment.

I refactor (using the option "Extract procedure") the three lines of code between the START and END comments (UPDATE emp ...). I give the new method the name Upd_Emp with the result:

procedure Upd_Emp(empno in emp.empno%TYPE, empno_cnst in out CONSTANT) is ...

Several things happen:

Errors:
1) the parameter empno is not used in the new method and should not be included

2) the parameter empno_cnst is declared "in out" even if it is not assigned a value

3) the parameter empno_cnst is given the type CONSTANT, it should have been emp.empno%TYPE derived from the constant definition. The most elegant solution would be to pass the value of the constant as a parameter, and then inside the procedure declare a constant with the value of the parameter and then make use of thos constant in the rest of the procedure. This ensures that the value of the 'constant' is not changed by accident when further changes are made to the new procedure.

4) The new feractored method is placed inside the comment above the code. This is a smaller inconvenience, I assume that the algorithm tries to find the nearest empty line containing the keywords Function or Procedure but forgets to perform a check for block comments.

Requests:
1) all keywords are lower case. In preferences I have chosen keyword case as Uppercase, the generated code should apply to this setting

2) I normally write my code using the '=>' notation, Upd_Emp(empno_cnst => empno_cnst), this should be a option in Preferences

3) The generated code should as good as possible respect the settings in Naming Conventions, e.g. parameter names must always be prefixed with "p_" etc.

Next, I have the following code snippet:

DECLARE
    a NUMBER := 1;
    b NUMBER := 2;
    c NUMBER := 3;
  BEGIN
    /* refactor START */
    IF a = b THEN
      c := 4;
    ELSE
      c := 5;
    END;
    /* refactor END */
  END;

The refactor menu entry "Swap assignment" is not documented, but is mostly self-explanatory. I refactor the code between the START and END tag. This results in the code

4 := c;
    ELSE
      5 := c;

likewise when "c" is assigned a constant value the same thing happens. Such meaningless swaps should be skipped by the refactoring method.

On the same code snippet as above I perform the menu entry "Replace assignment with initialization". This results in the code

c NUMBER := 3 := 4;:= 5;

If a variable is already initialised, further initialisation should be omitted.

 

[Marco Kalter]

We'll check it out.

 

[Stew Stryker]

I'll bite, what should the "Swap assignment" refactoring do besides what it does? I have trouble imagining when I'd want to say:

5 := c;

On the other hand, when I'm building a query, I like to see the comparisons in a particular order. For example:

SELECT pf.*
  FROM aeo_dcf_parent pf
  JOIN aeo_dcf_grand_children c ON pf.na_id_number = c.id_number
...

I'd like to be able to swap the comparisons in the ON clause to be:

ON c.id_number = pf.na_id_number

which I find more logical. It shouldn't be any faster, but it feels better to me.

That Swap Assignment isn't enabled when I select that clause.

 

[Marco Kalter]

The Swap assignment function swaps an assignment:

a := b; --> b := a;

It does nothing with other statements, including comparisons.

 

[Stew Stryker]

Thanks for clarifying, Marco.

 

[Stew Stryker]

In rewriting someone else's query, I'm finding myself swapping query clause comparisons a LOT. I mentioned this before and found that Swap wouldn't help me.

Could this feature be added to the enhancement request list?

Thanks,

Stew

 

[Marco Kalter]

Okay, I have added this to the list of enhancement requests.

 

[William Robertson]

Stew, I'm totally with you there. I know SQL doesn't care, but having joins written consistently forwards makes it a heck of a lot easier to see what the query is trying to do.