Regression: wc command is broken

All "this" and "final" as been removed on commit 5363....

The bug is here (WcCommand.java) :

private ByteCountInputStream(InputStream inputStream) {
super();
-- inputStream = inputStream;
++ this.inputStream = inputStream;
}

And an others bug on line 403...

private WcStream processStream(String fileName, InputStream inputStream) throws IOException {
ByteCountInputStream bic = new ByteCountInputStream(inputStream);
InputStreamReader reader = null;
-- fileName = fileName;
++ this.fileName = fileName;

Ah ok, thats my bad then, i did some editing of those to make some of it a little clearer. I'll fix them up.

I'll make a couple of recommendations though. I know final has its purposes, but we really don't need it in front of every local variable, same with 'this'. What i generally do is pick unambiguous names when distinguishing between instance and local vars. I know its not a JNode 'rule', but IMO it probably should be...

but IMO it probably should be...

Respectfully ... I disagree with you. I detest the convention of tacking "m_" or something on the front of attributes to distinguish them from locals, etc. And if you expect some variable to not change after it has been initialized, it is (IMO) good practice to declare it as final. And conversely, gratuitously removing a 'final' that someone else took the time to put there is bad practice!

BTW, the bug tripped you up is not a problem for people who use IDEs like Eclipse. Eclipse will produce a compiler warning when you assign some variable to itself, and display the warning right there in the source code. I expect findBugs would tell you about it too.

And while we are on the topic of style, it is a good idea to conform to the JNode style rules for identifier naming. That means not creating identifier names like "help_some_message". The correct identifier style for this attribute would be "helpSomeMessage" or (if it is declared 'static' 'final') "HELP_SOME_MESSAGE".

Also, I cannot see the point of changing this:

   private final Argument argFoo = ...;
   ...
   public XxxCommand() {
       ...
       registerArguments(argFoo,...);
   }

into this:

   private final Argument;
   ...
   public XxxCommand() {
       ...
       argFoo = ...;
       registerArguments(argFoo,...);
   }

As far as I am concerned, this is making the code harder to read ... not easier. Declaring an identifier at one place and initializing it another place means that I have two places to look rather than one. (Potentially more if the 'final' has been removed as well!) A half-decent IDE has accelerators to show me a identifier's declaration with a single keystroke, but finding a separate initialisation for an identifier takes more effort.

I detest the convention of tacking "m_" or something on the front of attributes to distinguish them from locals, etc.

That wasn't quite what i was suggesting. I meant using different names. If there is a class variable inputStream, and a method param, name it 'in', or something other than 'inputStream'. Maybe it's the c programmer in me, where ambiguous names can get you in trouble. And honestly, if you detest tacking 'm_' onto a var name, how is this any different than tacking 'this.' onto the name, which is what you have to do if you use ambiguous names...

That means not creating identifier names like "help_some_message". The correct identifier style for this attribute would be "helpSomeMessage" or (if it is declared 'static' 'final') "HELP_SOME_MESSAGE".

I realize that is convention, which is exactly why i did the opposite. help_something is different from HELP_SOMETHING, and therefore is easier strip with sed. At some point i'm going to strip all of those strings out of every command, and i dont want to have to go through each file one at a time. Thats all that is about.

As far as I am concerned, this is making the code harder to read ... not easier. Declaring an identifier at one place and initializing it another place means that I have two places to look rather than one. (Potentially more if the 'final' has been removed as well!) A half-decent IDE has accelerators to show me a identifier's declaration with a single keystroke, but finding a separate initialisation for an identifier takes more effort.

I dont understand what is making it harder. Isn't it 'normal' to declare instance vars and assigne/create in the constructor? I think those commands are probably one of the very few times i have seen instance vars instantiated en masse outside of constructors. static class vars OTOH generally follow this convention, its rare to see a static {} block instantiating them.

Anyway, if it's really that bad, i'll stop. I thought i was making it cleaner/neater, at least it is from my point of view. But then again, i think asm is easier to read than java period

That wasn't quite what i was suggesting. I meant using different names. If there is a class variable inputStream, and a method param, name it 'in', or something other than 'inputStream'.

That requires me to think harder when reading the code.

And honestly, if you detest tacking 'm_' onto a var name, how is this any different than tacking 'this.' onto the name, which is what you have to do if you use ambiguous names...

No reason. I just prefer the use of "this.". It is what I'm used to, and what I see throughout the rest of the ... umm ... 2.2 million lines in the JNode + classlib codebase.

Anyway. The point is that most JNode developers are generally happy with the coding standard the way it is. Or at least sufficiently used to it that they would not be inclined to spend energy fighting over it. It is called compromising or going with the flow. And after a time, you stop noticing what you once thought was "ugly" and just think of it as "normal".

I realize that is convention, which is exactly why i did the opposite. help_something is different from HELP_SOMETHING, and therefore is easier strip with sed. At some point i'm going to strip all of those strings out of every command, and i dont want to have to go through each file one at a time. Thats all that is about.

Most Java IDEs are better than sed for identifying strings to be internationalized. And they don't depend following some name convention. Indeed, some of them may even do most of the work for you.

I don't have much sympathy for pain caused by using Vi instead of a decent Java IDE ... in case you hadn't noticed.

Isn't it 'normal' to declare instance vars and assigne/create in the constructor?

Only if the instance variables depend on the constructor arguments. In the case of an XxxCommand constructor, there can be no constructor arguments. The "contract" for a JNode command class is that it implements the Command interface and has a no-args constructor.

BTW, this is not just my preference. Section 6.2 of the Java style guide says:
Try to initialize local variables where they're declared. The only reason not to initialize a variable where it's declared is if the initial value depends on some computation occurring first.

Manually closed.