[ANN] Rearranger plugin -- new version 0.6 released

Not a trivial request, but here is what I'd like to see in the rearranger :

Reformat : reorder extracted methods to match the logical flow
http://www.intellij.net/tracker/idea/viewSCR?publicId=15118
http://www.intellij.net/tracker/idea/viewSCR?publicId=4733#547083


Example :

Before : code to reformat
-

public void outer (){

foo1 ();
foo2 ();
foo3 ();
}

private void foo3 (){ ...}
private void foo1 (){ ...}
private void foo2 (){ ...}


After : reformating action (Requested)
-

public void outer (){

foo1 ();
foo2 ();
foo3 ();
}

private void foo1 (){ ...}
private void foo2 (){ ...}
private void foo3 (){ ...}



note : the same idea could be implemented as
- intention ("Move method to match logical flow")
- code inspection

Alain

Alain,

I'm willing to consider it if you're willing to help me figure out the rules and the exceptions! :)

Is the rule thus: when a private method is referenced by one other method (the 'parent method'), it can be considered an "extracted" method. Extracted methods can be reordered (1) based on order they appear in the parent method, with respect to other extracted methods; and/or (2) moved to a position immediately after the parent method.

So there could be two checkboxes on the 'Method' panel.

1) Move extracted methods (private methods called only by the parent method) immediately below the parent method, wherever it may finally be placed. If this is not checked, private methods are left where they are.

2) Sort extracted methods by the order in which they are called (or in the order in which they appear to be called) by the parent method. If this is not checked but #1 is, then the extracted methods are simply moved in their existing order underneath the parent method.

This would (naturally!) have to happen recursively, wouldn't it? E.g.

Would we ignore private methods called by more than one parent method, or called twice by the same method?

-Dave

Dave,

>Alain,
>I'm willing to consider it if you're willing to help me figure out the rules and the exceptions! :)
>


Fair enough.

Your 2 options sounds good, but I think it's not enough .

option 3 : Depth-first vs breadth-first
-

Looking at your example, where an extracted method call 2 further
extracted methods,

, I can see 2 ways to reorder the methods :

Depth-first

, or Breadth-first

option 4 : move after the last usage.
-

> Would we ignore private methods called by more than one parent
> method, or called twice by the same method?

If people checked the "sort by call-order" option, considering only the
1st call make the most sense to me today, but I may change my mind, with
usage. Otherwise, you could add this - 4th - option :


Automatic mode:
-

You could also add an option to automatically sort classes when you open
them, depending on some rules :
- only the 1st time you open it
(you'd have to store the list of processed classes in the project file)
- age of last modif
- size (nof lines, or nof methods)
- location (regexp on the package and class names)


Alain

Hi,

This is a great plugin, extremely useful for cleaning up other peoples
code:-)

On 2003/10/29 04:32, Dave Kriewall wrote:

Version 0.6 adds several new capabilities:

- Ability to select items by matching name to a regular expression
pattern.

This setting does not seem to be saved however.

Please let me know if you have any problems. (I changed a lot of
code! :)

I have added two items in "Class Member Order":
- all methods whose name does not match 'main' (alphabetized)
- all methods whose name matches 'main'
When I restart IDEA the settings have changed to:
- all methods (alphabetized)
- all methods
As a workaround I don't restart IDEA;-)

Other than that, the name matching works great! All my main methods are
now at the bottom of the class, where they belong:-)

-- Bas

Thanks for the feedback, Bas. I'll fix it right away.
-Dave

OK Bas,

give version 0.7 a try.

-Dave

On 2003/11/04 01:41, Dave Kriewall wrote:

give version 0.7 a try.

Thanks, that was really fast! A quick test shows that it works perfectly
now. Excellent.

-- Bas

Alain, (and all rearranger users who might find this useful)

I'm about ready to get to work on your request for special handling of private methods. Here's my proposal.

1) I think that the configuration for this is separate from the existing "Class Member Order" panel. What we are really saying is that private methods will be treated by a completely different set of rules. So I'm proposing adding another tab, say "Global Settings" or "Private Methods", in addition to the existing two (class member order, outer class order).

2) On this new tab, user has following top-level choices (say radio buttons):

If the 2nd is chosen, then the user can also specify ordering options:

Examples:

If there is only one private method called by a parent method, and the private method calls no other private method, then none of these rules (2 or 3) make any difference.

If two private methods are called by a parent, and the private methods do not call other private methods, then the depth-first/breadth-first rule makes no difference.
The two private methods would be ordered according to rule 3A, 3B or 3C.

For the following 6 examples (combinations of rules 2 and 3), assume the following source code:

Then the resulting order of method placement is:

Algorithm when "move private methods" option is chosen:

Note: Any private methods that are not referenced by any other method in the class would be placed according to "Class Member Order" rules. These are useless methods (unused code) but may not be in the future.

Does this make sense?

Is there any need to emit special comments? If so, would the ability to emit a comment before and after the children be sufficient?

For example,

Thanks for your thoughts on this.

-Dave

Dave :


Thanks for the energy you are putting into this. I can't wait to the
result.
You don't need much sleep, do you?


Here are a few remarks :


1 : your plan looks ok.
-

but...


2 : it's about "extracted methods", rather than "private methods"
-

The idea behind my initial request it to have the physical order, in the
class, follow the logical flow, in the calling tree. In this context,
being private for a method is just a detail. It's not a requirement.

Another way to say this is :
making a private method public shouldn't change the rearranger result.

Or at least, there should be a way to configure the plugin to behave
this way.
Motivation : sometimes, people would make a private method public, only
to be able to test it directly, rather than through their public parent.


3 : son with 2+ fathers
-

What happens to a (private) method that would be called by 2 other
methods in the same class.
This was in my previous mail.
By default, I would move it after it's first call/father.
For completeness, you could offer an option :
° move after callee after first caller
or
° move after callee after last caller

but I'm not sure it would be useful. I wouldn't use it.



4 : handling of useless private methods : comment and location
-

1.a : special comment before their section
example
// USELESS PRIVATE METHODS


1.b : location
at the end of the "methods" section,
or
at the end of the class.



5 : the case of the overloaded methods/constructors
-

(if you have time...)

Example

Personally, I prefer

This is also valid if you replace the constructors by methods "foo(...)".




5 : OT
This morning, I posted a someway related request, that would fit your
plugin nicely :

Add separate method call and declaration colour for private: coz they
were extracted.
http://www.intellij.net/tracker/idea/viewSCR?publicId=20435



Alain

Oops, typo in point 5. Here is the correct version :

]]>

5 : the case of the overloaded methods/constructors
-

(if you have time...)

Example

Personally, I prefer

This is also valid if you replace the constructors by methods "foo(...)".

]]>


Alain

Alain,

Thanks for the feedback!

2) The reason I thought it would be best to restrict it to private methods is that:
- it's more likely that it is an extracted method
- getters/setters would not be affected.

I'm pretty sure that folks don't view getters/setters as extracted methods, so moving them after the first or last caller wouldn't be appropriate.

On the other hand, I see your point about making private methods public temporarily.

Only compromise I can think of is something like adding a checkbox on the Method dialog that means "exempt from special reordering treatment". So you could make a rule that moves all getter/setters toward the front of the file and exempts them from consideration for the kind of rearrangement we're discussing. Or conversely, have a set of rules that selects methods that we should "consider for special reordering treatment". This is probably better since we may think of other types of exemptions or special reorderings in the future.

So I'll propose that the user can create a list, similar to current selection criteria, that will select methods for special reordering. (The list order will be immaterial.) User could exclude methods by method name, e.g. or could specifically exclude methods of type getter/setter.

By default, the list would be empty so that current behavior is unchanged.

User could add private methods, non-getter/setter methods, or whatever describes your notion of "extracted" methods.

You could even add constructors (peeking ahead to your request below.)

3) son with 2+ fathers -- I snuck in that option in my previous message -- sorry, wasn't very obvious:


4) emitting a comment for useless private methods would be handled by the current rules-based feature.

Any method not selected for special treatment (because, although available for special treatment, it was not called by any other method in the class) remains in the list of items to be rearranged the current way.

So you could have a comment

which is only emitted if an item matches the next rule, and the next rule is

5) Yeah, let's talk about constructors. If we open this up to any methods, not just private methods, then I think constructors would be handled just fine. Only additional way I can think to sort them would be based on number of parameters.

The way you listed it in your amended message is reversed; the "son" constructor (called by 3 parents) comes first, then the parents in some order. To make this constructor example work, it looks like we need a "reverse order" checkbox so that the son comes first, followed by parent.

If the code was:

so that the constructors were cascaded, then the "reverse order" technique would produce:

because a constructor with N parameters calls the constructor with N+1 parameters, so there's a greatGrandfather-Grandfather-Father-Son hierarchy.

OK, it's a bit more work, and a bit less sleep.. ;) but I'll do whatever it takes to make my clientele happy!

-Dave

Dave,



> 2) The reason I thought it would be best to restrict it to private
methods is that:
> - it's more likely that it is an extracted method
> - getters/setters would not be affected.


True, getters/setters are a special case, and they should not take part
in this game.
(Don't forget to use the user's code style to recognize them !)




About your suggestion for point 2 :

> ..the user can create a list,

I'm a little confused here, because I find it quite complex, although
the "problem" is pretty simple, and common enough to deserve a smart and
effortless default handling.

Just to give more weight to this request, here is another real-life
example where a private method becomes public for a good reason :


step 1 : initial code.
-

step 2 : method extraction
-

step 3 : the private method becomes public
-

Later in the client code, I'll need to use directly the second method,
so I'll make it public.

The default handling of the code above must do as if the extracted
method were still private, and place it just after its
caller/father.Otherwise it will become hard to read. And making the code
easier to read is the whole purpose of the plugin.





Some more about overloaded methods :
-

On the the code above, if the extraction had introduced an intermediary
helper method, I think the natural order should not be followed
completely. I would introduce an exception, to keep overloaded methods
close together :


Example : before
-

For the code above, I'd tend to say that overloaded methods should be
kept together :

=> rearrangement should produce :

Example : after
-

public boolean isAccessor (Editor e, File f) ..
public boolean isAccessor (PsiMethod m) ..
private PsiMethod methodAtCaret (Editor e, File f) ..


=> rather than the real callling order :

public boolean isAccessor (Editor e, File f) ..
private PsiMethod methodAtCaret (Editor e, File f) ..
public boolean isAccessor (PsiMethod m) ..





Alain

Alain,

in step 3 you have a method

which to me looks much like a getter method. But you want this to follow its parent, the other "isAccessor" method, because you regard it as an extracted method.

How do I reconcile this with your assertion that getters/setters should not take part in this game?

Do you feel that it should be treated as an extracted method because it is called by another getter (its parent)? (It might be called by other methods also.)

The real problem is knowing when a non-private method (e.g. getters/setters) is to be regarded as an extracted method. These extracted methods are moved along with their parent(s), and aren't subject to the normal rules of rearrangement. I'm not yet convinced that we can come up with definitive criteria for determining which methods are "extracted." That's why I was proposing leaving it up to the user. (When in doubt, punt. :) Let the user decide if certain methods are, in his/her opinion, extracted. I could add additional criteria to make that possible, such as

or others you may come up with.

If you can really nail down what constitutes an extracted method, then I wouldn't have to make the user define it. That would be great, but I have a sinking feeling that it won't suit everybody. The problem doesn't seem simple to me (at the moment).

-Dave

P.S. I am using

to determine if a method is getter or setter.

Dave,



-

I feel a big part of your problems would disappear, and your code would be simpler, if you had an accuccate accessor tester :

> in step 3 you have a method ..boolean isAccessor(PsiMethod m)..
> which to me looks much like a getter method.

NO, look at what the method does/the method's code :it is NOT an getter. It doesn't follow the getter pattern.


>P.S. I am using PropertyUtil.isSimplePropertyGetter(method);
> psi.util.PropertyUtil.isSimplePropertySetter(method);

public int getX() {return notX ;}
public int getX() { x += 1 ; return x ;} //

 The real problem is knowing when a non-private method .. 
  > is to be regarded as an extracted method.  These extracted methods 
  > are moved along with their parent(s), and aren't subject to the 
  > normal rules of rearrangement.  


If, after building the calling tree of the class, a non-private method is 
  - not on the top level, (has a "father" in the class)
  - has only one father,
you are pretty safe. 

If, ..., but it has 2+ fathers ??


When the above criterias are met, you could let the user decide, with 2 additional options.

   Treat non-private methods as an extracted method if
    [ ]   Never
    [v]   has exactly one caller in the class
    [ ]   has one or more callers in the class
    
I'd set the default to option, like above.

I think this would make your life easier, and there is a chance usage will proves it's good enough. If it's not, you can add more rules later.


----------------------------

Another special category of methods I'd like the Rearranger to handle separately is the 3 canonicals

   1. equals()
   2. hashCode()
   3. toString()

I think they are special enough to receive a special treatment. They are much simpler to recognize (code added below)


Alain
----------------------------


Addendum : code to recognize accessors, equals, hashCode and toString

]]>

-

protected static boolean isAccessor ( PsiMethod i_method, CodeStyleManager i_codeStyleManager )
{
return MyPsiMethodUtil.isSetter ( i_method, i_codeStyleManager )
|| MyPsiMethodUtil.isGetter ( i_method, i_codeStyleManager );
}


-

public static boolean isHashCode(PsiMethod i_method)
{
return checkMethodSignature(i_method, "public", "int", "hashCode", 0);
}

public static boolean isToString(PsiMethod i_method)
{
return checkMethodSignature(i_method, "public", "String", "toString", 0);
}

public static boolean isEquals(PsiMethod i_method)
{
final boolean basicsAreOk = checkMethodSignature(i_method, "public", "boolean", "equals", 1);
if (!basicsAreOk)
return false;


final PsiParameter[] parameters = i_method.getParameterList().getParameters();
final boolean paramIsOk = "Object".equals(parameters[0].getTypeElement().getText());
return paramIsOk;

}

-

private static boolean checkMethodSignature(PsiMethod i_method, final String i_expectedModifier, final String i_expectedReturnType, final String i_expectedName, final int i_expectedNofParameters)
{
if (i_method == null)
return false;
final boolean nameIsOk = i_expectedName.equals(i_method.getName());
if (! nameIsOk)
return false;

if (! (nofParameters(i_method) == i_expectedNofParameters))
return false;

final PsiModifierList modifierList = i_method.getModifierList();
if (! i_expectedModifier.equals(modifierList.getText()))
return false;

if (!( i_expectedReturnType.equals(i_method.getReturnTypeElement().getText())))
return false;

return true ;
}




-

public static boolean isGetter(PsiMethod i_method, CodeStyleManager i_codeStyleManager)
{
if (i_method == null)
return false ;

final boolean hasParameters = 1 <= nofParameters(i_method);
if (hasParameters )
return false;


final String name = i_method.getName();

if (name.startsWith("get")) return getterNameMatchedReturnedValue(i_method, name, "get", i_codeStyleManager);
if (name.startsWith("is" )) return getterNameMatchedReturnedValue(i_method, name, "is", i_codeStyleManager);
if (name.startsWith("has")) return getterNameMatchedReturnedValue(i_method, name, "has", i_codeStyleManager);

return false;
}

private static boolean getterNameMatchedReturnedValue(PsiMethod i_method, String i_name, String i_prefix, CodeStyleManager i_codeStyleManager)
{
String i_methodNameTrail = i_name.substring(i_prefix.length());
if (!nameIsWellFormed(i_methodNameTrail))
return false;

if (isAbstract(i_method))
return false;

final String nameFromBody = nameOfVariableReturnedInBody(i_method);
final String propertyName = propertyNameFromMethodTrail(i_methodNameTrail);


final boolean namesMatch = nameFromBody.equals(fieldName(propertyName, i_codeStyleManager));
return namesMatch;
}

private static String nameOfVariableReturnedInBody(PsiMethod i_method)
{

final PsiStatement[] statements = i_method.getBody().getStatements();

final boolean methodBodyIsEmpty = nofStatements(i_method) == 0;
if (methodBodyIsEmpty)
return "";

if ( ! U.isReturnStatement(statements[0]))
return "";
final PsiReturnStatement returnStatement = (PsiReturnStatement) statements[0];
final PsiExpression returnValue = returnStatement.getReturnValue();
//todo : improve
if (null ==returnValue)
return "";
String returnValueText = returnValue.getText();
returnValueText = returnValueText.replaceFirst("this
.", "");
return returnValueText;
}

private static String propertyNameFromMethodTrail(String i_source)
{
final String left = i_source.substring(0,1).toLowerCase();
final String right = i_source.substring(1);

return left + right;
}


-

public static boolean isSetter(PsiMethod i_method, CodeStyleManager i_codeStyleManager)
{
if ( couldBeAsetter(i_method))
return false;

String i_methodNameTrail = i_method.getName().substring("set".length());
if (!nameIsWellFormed(i_methodNameTrail))
return false;


final PsiElement psiElement = i_method.getBody().getStatements()[0].getChildren()[0];
if ( !U.isAssignment(psiElement) )
return false;

PsiAssignmentExpression assignement = (PsiAssignmentExpression) psiElement;
final boolean notAPlainAssignment = PsiJavaToken.EQ != assignement.getOperationSign().getTokenType();
if (notAPlainAssignment)
return false;

final PsiExpression rExpressionRaw = assignement.getRExpression();
if (null == rExpressionRaw)
return false;

String
lExpression = assignement.getLExpression().getText().replaceFirst("this.", ""),
parameterName = i_method.getParameterList().getParameters()[0].getName();

if (!(parameterName.equals(rExpressionRaw.getText())))
return false;

final boolean targetIsProperty = lExpression.equals(fieldName(propertyNameFromMethodTrail(i_methodNameTrail), i_codeStyleManager));
return targetIsProperty;

}



private static boolean couldBeAsetter(PsiMethod i_method)
{
if ( i_method == null ) return false;
if ( i_method.getBody() == null ) return false;
if ( 1 != nofParameters(i_method) ) return false;
if ( 1 != nofStatements(i_method) ) return false;
if ( ! i_method.getName().startsWith("set") ) return false;

return true;
}

private static boolean nameIsWellFormed(String i_methodNameTrail)
{
final boolean methodNameIsTooShort = i_methodNameTrail.length() == 0;
if ( methodNameIsTooShort )
return false;

final char firstCandidateMemberChar = i_methodNameTrail.charAt(0);
if ( Character.isLowerCase(firstCandidateMemberChar))
return false;

return true;
}

public static boolean isAssignment (final PsiElement i_element)
{
return (i_element instanceof PsiAssignmentExpression);
}

private static String propertyNameFromMethodTrail(String i_source)
{
final String left = i_source.substring(0,1).toLowerCase();
final String right = i_source.substring(1);

return left + right;
}

private static String fieldName(String i_propertyName, CodeStyleManager i_codeStyleManager)
{
return i_codeStyleManager.propertyNameToVariableName(i_propertyName,VariableKind.FIELD);
}

private static int nofParameters (PsiMethod i_method)
{
return i_method.getParameterList().getParameters().length;
}
private static int nofStatements (PsiMethod i_method)
{
return i_method.getBody().getStatements().length;
}
private static boolean isAbstract (PsiMethod i_method)
{
return null == i_method.getBody();
}
public static boolean isReturnStatement (final PsiStatement i_statement)
{
return i_statement instanceof PsiReturnStatement;
}

Hi Alain,

Thanks very much for your code sample. I remember you posted something about determining accessors/mutators properly a few weeks back (on OpenAPI forum I think) and someone replied with the solution I used. I had it in the back of my mind to sneak a look at your plugin to see what you were doing. This makes it much easier!

(Don't you wish we had source code for PSI so we could figure out if they already had a utility that would work?)

OK, so we automatically exclude G/Setters, and top level methods (those not called by another method in the program), and constructors (special handling). We should also exclude methods involved in a cycle (a() calls b(), b() calls a() -- some form of recursion happening).

User may also exclude the remaining non-private methods by the number of callers they have (as you suggested, "never, 1, 2+".)

What special handling do you want for equals(), hashcode() and toString()? Just exclusion from special treatment? You can already place them specially anywhere you want by doing a name match in the method criteria. And, for that matter, you can handle any overriding method in the same way.

Today is going to be a tabifier sort of day, but I hope to get to work on this next.

Thanks for your help & suggestions,
-Dave

Dave Kriewall wrote:

>What special handling do you want for equals(), hashcode() and toString()? Just exclusion from special treatment? You can already place them specially anywhere you want by doing a name match in the method criteria.
>

As for the accessors, I give them a special treatment in the
CamouflagePlugin, because they are a special kind of methods that belong
to the same family : canonicals methods.
I'd like them to
- stick together,
- in a specific order (ex: 1: equals, 2: hashCode, 3: toString)
- be identified/movable as a block (to unclutter the main ui : 3 lines
=> 1 line)

=>
- in the main interface, you would simply locate where to place the
group of canonical methods (1 line),
- in another location (another tab), you would choose the order inside
the group.

You could reuse that UI principle ( 1 line for the group in the main ui,
and more details in another tab) for other group/kinds of methods :
- constructors
- accessors
..?

Alain