issue 83: Never ignore exception check

43 views
Skip to first unread message

RomanIvanov

unread,
Oct 15, 2014, 3:55:18 PM10/15/14
to sevntu-c...@googlegroups.com, Дмитрий Базунов
from Dmitriy:


Issue:

Check find CATCH_LITERAL nodes and check if catched exception is handle by some code. 
Check has two settings:
1)Regex pattern which it use to detect what Exceptions showld be checked.(Default value InterruptedException)
2)Boolean - is comment allowed. If it's true there could be a comment in a catch block instead of handling code.(Default value true). 

Check:

Tests:

Inputs:
 --

RomanIvanov

unread,
Oct 15, 2014, 4:16:27 PM10/15/14
to sevntu-c...@googlegroups.com, binna...@gmail.com
Hi Dmitriy,


1)
> public static final String DEFAULT_EXCEPTION_CLASSNAME_REGEXP = "InterruptedException";

please make Check be aware of package name of Exception, to avoid conflict on Class names.
user need to define full package name.

2) 
> private boolean mIsCommentAllowed;

the more I read about that problem,the more I understand that allow developer to write a message like "// just suppress Checkstyle" is a expected.
I can not now even think under what circumstances we should suppress InterruptedException, but it is possible for other user custom exception.
but in this case suppress comment should be predefined by author of config.
For example to suppress only cases with comment like "// confirmed no code by .* " and not suppress by general "// no code"
So we need string(regExp) option for Check

3)
> 
final String exceptionName = parameterDef
.findFirstToken(TokenTypes.TYPE)
.getFirstChild().getText();

please repull your fork , Checkstyle already support multiple catch "catch( MyException | InteruptedException | RuntimeException) {}"


4)
> private boolean isEmptySlist(final DetailAST aSlistToken)

please make it as isEmptyCatch, and make it static.
Make static all other methods if possible.

5) 
please make examples in Javadoc how to configure Check with ignore comments.

thanks,
Roman Ivanov




Дмитрий Базунов

unread,
Nov 4, 2014, 11:11:09 PM11/4/14
to sevntu-c...@googlegroups.com, binna...@gmail.com
1)DONE
2)DONE
3)DONE
4)DONE
5)DONE

I override get path method for NeverIgnoreExceptionCheckTest to work with resource-noncompilable folder.



среда, 15 октября 2014 г., 23:16:27 UTC+3 пользователь RomanIvanov написал:

Roman Ivanov

unread,
Nov 8, 2014, 10:20:03 AM11/8/14
to sevntu-c...@googlegroups.com, Дмитрий Базунов
Hi Dmitriy,



1) 
Check co ...
Example:
First catch
do {
for (String exceptionNam
switch (leftNode.getType()) {
final StringBuilder nameB
for (Integer commentLineNumber
 lineNumbers = cppCom


Please add "\n" before each of that text , to not make a continuous mess from code and documentation. write code for human.


2) 
* Also you can allow using comment to decribe why exception not handle instead
* of handling exception with code. To suppress exception by comment check use
* regexp pattern.

Also you can allow missed handling by special excuse comment, that you can set up in option suppressionCommentRegExp. 

attention: I changed name of option on purpose to be more correct.

3)
* To configure check suppress exception by comment you need to set
* IsCommentAllowed property to true and configure SuppressCommentRegexp as you
* need.

We need only one option suppressionCommentRegExp, if use does not care about comment content it set up ".*" in regexp

4) 
DEFAULT_SUPPRESS_COMMENT_REGEXP = ".+No need to handle cause.+";

".+No need to handle that.+"

5) 
public NeverIgnoreExceptionCheck()

please move all initialization to field definition. C-tor need to be removed.

6)
 do {
switch (catchTypeNode.getType()) {

I do not understand that code , please put a comment on it .
can it be moved to separate method ?

7) 
final Pattern methodNamePattern = Pattern
.compile(mExceptionClassNameRegexp);
...
final Pattern methodNamePattern = Pattern
.compile(mCommentSuppressRegexp);

Why compilation is done on each method call ? will it change during execution ?
compile is very heavy operation it should be done ones if pattern is not changing.

8) 
* Method get full exception name with package name.
* @param aDotNode
* - DetailAST node of AST which represent node with type DOT.
* @return String which content exception name with full pass.
*/
private static String getExceptionNameFromDot(final DetailAST aDotNode)

Name of method is weird and not clear, in javadoc looks like you are more accurate.
Please change name.
9) 
return checkCommentRegular(aSlistToken.getLineNo(),
rCurlyTocken.getLineNo());

проверитьКоментарий......  - ответ: да/правда ? 

что "да" ?

10) 
final StringBuilder nameBuilder = new StringBuilder(beforeDotString);
nameBuilder.append('.');
nameBuilder.append(leftNode.getNextSibling().getText());
return nameBuilder.toString();

Why not a simple string concatenation ? I do not belive tht you will get any performance from that simple String concatenation.

11)
Please generate HTML report from maven-checkstyle-plugin to recheck code on Guava library (we need testing on real code). You can ask Max Vetrenko on how to do that or I will provide you detail later.


thanks,
Roman Ivanov
​

Roman Ivanov

unread,
Nov 22, 2014, 10:59:49 AM11/22/14
to sevntu-c...@googlegroups.com, Дмитрий Базунов
alive ?​

Дмитрий Базунов

unread,
Mar 7, 2015, 8:44:12 PM3/7/15
to sevntu-c...@googlegroups.com, binna...@gmail.com
3)
* To configure check suppress exception by comment you need to set
* IsCommentAllowed property to true and configure SuppressCommentRegexp as you
* need.

We need only one option suppressionCommentRegExp, if use does not care about comment content it set up ".*" in regexp

I am not agreed with that.  User will use isCommentAllowed  property not "does not care about comment" he use it for forbid comment suppress even if comment match  SuppressCommentRegexp. If it's purpose is not clear it can be renamed to "isCommentSuppressForbiden".
 
4)Fix code and test input
5)Done
6)Add comment. It can be moved to separate method, but i don't sure about this.
7)Made Pattern compile only on regexp change.
8)Rename method to getExceptionNameWithPackage.
9)Rename method to  isCommentContainsSuppressLine.
10)Rewrite fot string concatenation.
11)Need some explanation. How to run new sevntu-check on open-source project with maven plugin.

суббота, 22 ноября 2014 г., 18:59:49 UTC+3 пользователь RomanIvanov написал:
alive ?​

Roman Ivanov

unread,
Mar 10, 2015, 12:13:09 PM3/10/15
to sevntu-c...@googlegroups.com, Дмитрий Базунов
Hi Dmitriy,

while your changes in codereview please generate report over opensource projects:

thanks,
Roman Ivanov
​

Roman Ivanov

unread,
Mar 10, 2015, 8:56:05 PM3/10/15
to sevntu-c...@googlegroups.com, Дмитрий Базунов
Hi Dmitriy,


1) Please some reference to http://checkstyle.sourceforge.net/config_blocks.html#EmptyCatchBlock as it is very similar check

2) Please provide description of check in first sentence without example

3) In javadoc you have two examples , please remove first and provide xml configuration for the second to let other see how to configure your Check to get that result

4) 
> 
 */
public static final String MSG_KEY = "ignore.exception";
/**

Add "\n"

 5) 
> public static final String DEFAULT_SUPPRESS_COMMENT_REGEXP = ".+No need to handle that.+";


6) 
> /*
* Switch is needed to divide 3 catch type situations.
* 1) Simple catch.

Please never use block comments inside a methods. Any intention to use - is already sign for bad design of method.


7) ..... to be continued ....

thanks,
Roman Ivanov
​

Roman Ivanov

unread,
Mar 11, 2015, 2:37:16 AM3/11/15
to sevntu-c...@googlegroups.com, Дмитрий Базунов
Hi Dmitriy,


7) please rebase over latest code and update copyright as in other files

8) please remove prefixes "a", "m", "r" from argument names , and variables and ...... 

9) 
> 
 * |__IDENT(IndexOutOfBoundsException)
*/
switch (catchTypeNode.getType()) {

please move that to separate method to make it like "exceptionsNameList.add(getNames())"
> 
10 ) 
> if (!(isCommentAllowed && isCatchContainsSuppressComment(sListToken)))

please move "!" to each operand, it is hard to read and understand logic with final inversion.

11) 
> final String nameBuilder

please fix the name.

12) 
> getExceptionNameFromMultiCatch

such a weird implementation .... please make it more readable and 

13) 
> isCommentContainsSuppressLine(final int aStartLine, final int aEndLine)

please look at EmptyCatchBlockCheck for implementation of that.

14) 
> I am not agreed with that.  User will use isCommentAllowed  property not "does not care about comment" he use it for forbid comment suppress even if comment match  SuppressCommentRegexp. If it's purpose is not clear it can be renamed to "isCommentSuppressForbiden".

Please make such a regexp, that will never match, or make it null by default an skip matching by it , if it is not initialized. But two option is too much for that functionality. Each option is API for Checkstyle , ones created you can not remove it as it will crash all chekcstyle configurations in the world, so do not generate them without serious need.

thanks,
Roman Ivanov
​
Reply all
Reply to author
Forward
0 new messages