Less warnings, less castings, no unnecessary blocks

[cstenkes]

Less warnings, less castings, no unnecessary blocks

[highsource]

Hi,

first of all please let me say that I appreaciate your effort. Your intention clearly is to improve the code quality and I understand it.

Having said that I am inclined not to accept the pull request as I think it fixes not the right things. Below are the main things:

• A lot of "unnecessary" blocks are removed. I strongly disagree with this. Yes, technically they are not required but I'd argue that block improve code readability.
  For blocks in generating code I usually group statements which will go to one block in the generated code. This makes easier understanding what generates what.
  For blocks in generated code, they normally group operations concerning one element - like one property.
  I am persuaded both are valuable.
• Removed blocks also changed the code formatting. Now it is somewhat more difficult to reason about changes in code as blocks of code no longe match in the diff.
• Some lines now contain the // DEMATIC comment. I guess this is the name of your company, I'm not sure this belongs to the code.
• Don't see why strategy should be renamed to strategy2.
• Replacing leftFieldAccessor.toRawValue(ifShouldBeSetBlock, leftField) with leftObject.invoke(createMethodName(fieldOutline)) is not correct.

The PR seems to bring a lot of trivial changes which affect code formatting and make it harder to see substantial changes. So I'm sorry but I think it will be better not to accept this PR.

Let's try it from a different angle. Apart from "unnecessary" blocks, what are the important changes you'd like to apply?

[cstenkes]

Hi,

The reason why I look in the code at all were the followings:

• Massive amount of warnings in the generated code compiling by Eclipse 4.9 and Java 8 default settings
• Unnecessary blocks in the generated code, quite hard to understand why small code blocks inside a method. If you would like to separate code blocks an empty line is more than enough for the reader meanwhile I understand your reasons, but I have never seen such code in the last almost 15-20 years.
• Unnecessary variable declaration, then variable value assignment instructions
• DEMATIC - yes It is my company (a world wide storage automatisation giant), first I did just little changes indicated by this, later I have done more changes, and unfortunately these signs were left. I will remove it in the next commit, It is now on Github.
• @Override added, because in the last official release (0.12) these were not part of code base (It is standard from Java 1.5)
• lots of suppress warning where these are not necessary
• lots of unnecessary variable casting like byte cast to byte automatically or int cast to int etc. Some not trivial has left in the code still like byte cast to short or int
• Overall I was not satisfied the readability of the generated code anyway.
• I will need more simpleXXX plugin like simpleEqualsPlugin, because I would like not put too much dependency to my code becuse of a simple method like toString...

[cstenkes]

After googling on the blocks inside a method indicates the next things:

• you want to reduce the scope of a variable, in some exceptional case it is acceptable
• you prepare the next code refactoring - "it is a sign of a desperately needed refactor, aka a "code smell" as Martin Fowler puts it :) " -> That´s why I started to deal with this plugin code.

Look at [https://stackoverflow.com/questions/1563030/anonymous-code-blocks-in-java]

[highsource]

Servus,

The reason why I look in the code at all were the followings:

• Massive amount of warnings in the generated code compiling by Eclipse 4.9 and Java 8 default settings

Too general - this should be broken into actionable items.

• Unnecessary blocks in the generated code, quite hard to understand why small code blocks inside a method. If you would like to separate code blocks an empty line is more than enough for the reader meanwhile I understand your reasons, but I have never seen such code in the last almost 15-20 years.

I suggest to leave this point out of discussion. I have a rather strong opinion on this. Write me off as stubborn, if you will, but please let us save time on this one.

At the moment, removed blocks in plugin code hinder me to review the PR since the code became not-quite-diffable and that's a practical problem.

• Unnecessary variable declaration, then variable value assignment instructions

This is how field accessor work in XJC. Please see com.sun.tools.xjc.outline.FieldAccessor. Accessing properties directly via getters or settes is not the right way in XJC.

• DEMATIC - yes It is my company (a world wide storage automatisation giant), first I did just little changes indicated by this, later I have done more changes, and unfortunately these signs were left. I will remove it in the next commit, It is now on Github.

OK.

• @override added, because in the last official release (0.12) these were not part of code base (It is standard from Java 1.5)

Not quite sure which @override you mean, not easy to see in the PR diffs. But we can work this out.

• lots of suppress warning where these are not necessary

Ok, we can work this out.

• lots of unnecessary variable casting like byte cast to byte automatically or int cast to int etc. Some not trivial has left in the code still like byte cast to short or int

Could you please give an example?

• Overall I was not satisfied the readability of the generated code anyway.

Would be good to break this into actionable items.

• I will need more simpleXXX plugin like simpleEqualsPlugin, because I would like not put too much dependency to my code becuse of a simple method like toString...

I'd absolutely welcome SimpleToStringPlugin etc. I've just found out that implementing it correctly, also handling special cases like JAXBElement is at least a little bit hard without some runtime code. But I's surely welcome a contribution here.

[highsource]

I've created #105 and https://github.com/highsource/jaxb2-basics/tree/issue-105 to work on improving the quality of the generated code.