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: LocalVariableName #23

Closed
maxvetrenko opened this issue Jun 20, 2014 · 15 comments
Closed

Update for existing Check: LocalVariableName #23

maxvetrenko opened this issue Jun 20, 2014 · 15 comments
Labels

Comments

@maxvetrenko
Copy link
Owner

Update LocalVariableName to allow one-character for temporary and looping variables. http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.2.7-local-variable-names

maxvetrenko added a commit that referenced this issue Jul 5, 2014
maxvetrenko added a commit that referenced this issue Jul 5, 2014
@rdiachenko
Copy link

Allow one character loop variable.

Change to "Allow one character variable in for loops."

mAllowOneCharForLoopVar

Change to "mAllowOneCharVarInForLoop

if(isForLoopVariable(aAST) && mAllowOneCharForLoopVar) {

Should be "if(mAllowOneCharForLoopVar && isForLoopVariable(aAST)) {"

if variable is loop variable.

Change to "if a variable is the loop's one"

if(isForLoopVariable(aAST) && mAllowOneCharForLoopVar) {

  •      String variableName = aAST.findFirstToken(TokenTypes.IDENT).getText();
    
  •      return !aSingleChar.matcher(variableName).find();
    
  •    }
    

I think your logic won't work properly if there is a loop variable with multiple chars:

for(int index = 1; index <10; index++) {
//come code
}

Please add this case into your test input and check

@rdiachenko
Copy link

Also update the check's documentation and xdocs

maxvetrenko added a commit that referenced this issue Jul 7, 2014
maxvetrenko added a commit that referenced this issue Jul 7, 2014
@maxvetrenko
Copy link
Owner Author

Change to "Allow one character variable in for loops."

Done

Change to "mAllowOneCharVarInForLoop

Done

Should be "if(mAllowOneCharForLoopVar && isForLoopVariable(aAST)) {"

Done

Change to "if a variable is the loop's one"

Done

I think your logic won't work properly if there is a loop variable with multiple chars:

I have added this case (InputSimple) and UTs did't fall.

Also update the check's documentation and xdocs

Done

maxvetrenko added a commit that referenced this issue Jul 7, 2014
maxvetrenko added a commit that referenced this issue Jul 7, 2014
@rdiachenko
Copy link

An example how to configure the check to allow

Change to "An example of how to configure the check to allow"

  •    for(Integer k = 1; k <10; k++) {
    
    •      //come code
      
    •  }
      
    •    for(double d = 1; d <10; d++) {
      
    •      //come code
      
    •  }
      
    •    for(long l = 1; l <10; l++) {
      
    •      //come code
      
    •  }
      

There are a lot of simular test inputs and they all cover the same case. Please remove redundant inputs and add the new ones you don't have:

for (int i=0,j=1; ...)

int index = 0;
for (index < 10; index++)...

for (Map.Entry<String,Integer> e : map.entrySet()) ....

etc.

Module LocalVariableName also has the following properties:

Change to "The check provides the following properties:"

maxvetrenko added a commit that referenced this issue Jul 8, 2014
maxvetrenko added a commit that referenced this issue Jul 8, 2014
maxvetrenko added a commit that referenced this issue Jul 8, 2014
@maxvetrenko
Copy link
Owner Author

Change to "An example of how to configure the check to allow"

Done

There are a lot of simular test inputs and they all cover the same case. Please remove redundant inputs and add the new ones you don't have....

Done, see changes into InputSimple file

Change to "The check provides the following properties:"

Done

@romani
Copy link

romani commented Jul 11, 2014

one character variable in for loops:

http://docs.oracle.com/javase/tutorial/java/nutsandbolts/for.html it is initialization expression - so call that you examine only initialization expression in a loop, and put this link to JavaDoc.
2)

Utils.getPattern("^[a-z]")

Why end of line symbol is missed in RegExp

  1. put yourself as author

maxvetrenko added a commit that referenced this issue Jul 11, 2014
@maxvetrenko
Copy link
Owner Author

  1. Done
  2. Done
  3. Done

@romani
Copy link

romani commented Jul 12, 2014

you have two option so you need to describe all options and then provide examples . See how it is done in here - http://checkstyle.sourceforge.net/config_annotation.html

in for loops:

"in FOR loop", fix in all places.

maxvetrenko added a commit that referenced this issue Jul 12, 2014
@maxvetrenko
Copy link
Owner Author

  1. Added example how to configure Check with custom properties commit
  2. Done

@romani
Copy link

romani commented Jul 12, 2014

  1. one character initialization expressions in FOR loop

What does this mean ? do you catch expression like "(i)"
Please write what exactly what you allow.

maxvetrenko added a commit that referenced this issue Jul 12, 2014
@maxvetrenko
Copy link
Owner Author

  1. Is it enough or I have to write more? link

@romani
Copy link

romani commented Jul 13, 2014

one character initialization expressions in FOR loop:

please translate this and read carefully .... . And write me there example of this. Read attentively what is initialization expressions!

maxvetrenko added a commit that referenced this issue Jul 14, 2014
maxvetrenko added a commit that referenced this issue Jul 15, 2014
maxvetrenko added a commit that referenced this issue Jul 15, 2014
@romani
Copy link

romani commented Jul 16, 2014

xdoc have to be fixed too for that problem, users will read HTML description first.
So HTML part of description is our main focus.

maxvetrenko added a commit that referenced this issue Jul 16, 2014
@maxvetrenko
Copy link
Owner Author

xdoc have to be fixed too for that problem

Was updated according to Javadoc in LocalVariableNameCheck

@romani
Copy link

romani commented Jul 16, 2014

ok PR.

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