[GitHub] jmeter issue #257: Static Analysis Fixes

2017-01-25 Thread pmouawad
Github user pmouawad commented on the issue: https://github.com/apache/jmeter/pull/257 @ham1 , Thanks a lot for your contribution. Looks globally good for me. I have just one remark, I wouldn't "optimize" the org.apache.jmeter.control package classes. This part of JMeter is

[GitHub] jmeter issue #257: Static Analysis Fixes

2017-01-25 Thread pmouawad
Github user pmouawad commented on the issue: https://github.com/apache/jmeter/pull/257 Hello, Another question I have. I see you replace StringBuilder by '+' concat. Is there a reason for this ? Reading: - http://www.pellegrino.link/2015/08/22/string-concatenation-

[GitHub] jmeter issue #257: Static Analysis Fixes

2017-01-25 Thread ham1
Github user ham1 commented on the issue: https://github.com/apache/jmeter/pull/257 Thanks for looking at the PR and commenting. I will take another look at the changes to `org.apache.jmeter.control` and either revert or justify them. I'd gladly add more tests if we move to

[GitHub] jmeter issue #257: Static Analysis Fixes

2017-01-27 Thread ham1
Github user ham1 commented on the issue: https://github.com/apache/jmeter/pull/257 After reviewing my changes to `org.apache.jmeter.control` they were all suggested by my IDE (IntelliJ) so I am confident they are correct - however I understand if you don't want to apply some of them d