Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for existing Check: DeclarationOrder #4

Closed
maxvetrenko opened this issue May 20, 2014 · 14 comments
Closed

Update for existing Check: DeclarationOrder #4

maxvetrenko opened this issue May 20, 2014 · 14 comments
Labels

Comments

@maxvetrenko
Copy link
Owner

Update existing Check to cover next rule: http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s3.4.2.1-overloads-never-split

@romani
Copy link

romani commented Jun 18, 2014

new option should be provided - "boolean groupOverloadMethods"

@rdiachenko
Copy link

If true, avoids to split overload methods

Change to "If true, avoid splitting overload methods"

  •    int parentNode = aAST.getParent().getType();
    
    •  final int parentType = parentNode;
      

Use parentType instead of parentNode as it was before

  •        if(mGroupOverloadMethods && (parentNode == TokenTypes.CLASS_DEF
    
    •              | parentNode == TokenTypes.ENUM_DEF)) {
      
  1. Use short-circuit operators
  2. What about interfaces?
  • Group overload methods
    private void groupOverloadMethods(DetailAST aObjectBlock)

Bad name and description. This method doesn't group anything it just checks for ...

DetailAST curToken

I don't understand this name

Integer methodNumber = methodCollector.get(methodName);

  1. Bad name
  2. Variable declaration is far away from its usage. It is used only in the inner if

if (methodCounter - methodNumber > 1) {

Avoid magic numbers

@rdiachenko
Copy link

Also update xdocs

@maxvetrenko
Copy link
Owner Author

Integer methodNumber = methodCollector.get(methodName);
Bad name

can't make up any good name. Please, help.

@maxvetrenko
Copy link
Owner Author

The rest is done, check my commit.

@rdiachenko
Copy link

Check the grouping of overload.

Should be "Checks that overload methods are grouped together.

checkGroupingOverloadMethods

Should be "checkGroupingOfOverloadMethods" or "checkOverloadMethodsGrouping" or any other ideas..

HashMap <String, Integer> methodCollector = new HashMap<String, Integer>();

Program to interfaces instead of their implementations

Integer methodNumber

  1. use primitives
  2. change name to "previousIndex"

methodCounter

Change name to "currentIndex"

methodCollector

Change name to "methodIndexMap"

@maxvetrenko
Copy link
Owner Author

use primitives

Can't use primitive, because sometimes methodIndexMap.get(methodName) returns null. If it tries to assign "null" to int, it will be NPE

The rest is done.

@rdiachenko
Copy link

An example check's configuration grouping overload methods:

Change to "An example of a check's configuration for grouping overload methods"

Checks that overload methods are grouped together

  • \* should not be separated from each other.
    

Change to "Checks that if overload methods are grouped together they should not be separated from each other"

Overload methods never split

Change to "Overload methods should not be split"

public void testOverloadMethods() throws Exception

Change to "testOverloadMethodsGrouping"

@maxvetrenko
Copy link
Owner Author

Overload methods never split

Where did you find it? I don't understand..

@rdiachenko
Copy link

Where did you find it? I don't understand..

  1. Open your commit (81bce3b)
  2. Press Ctrl+F and insert "Overload methods never split" into the search field
  3. Press Enter. The all usages of the text will be highlighted.

@romani
Copy link

romani commented Jun 30, 2014

looks good, but please make

value="TRUE"

make it as value="true" in both files(java and xml)

  1. from javadoc
  • * public void notFoo() {} // Have to be after foo(int i, String s)

What is the reason of inner tag "" ?

  1. update javadoc to have your name added to "@ author" tag. All Checks that you update should have your name as author.

@maxvetrenko
Copy link
Owner Author

  1. Done
  2. Done
  3. Done

@romani
Copy link

romani commented Jun 30, 2014

ok, good, this update is ready for PullRequest(PR) - please do.
Please make sure that UTs coverage is 100% or close.

@maxvetrenko
Copy link
Owner Author

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants