(#706) : Default value for property of "integer enum" type caused POJ…

[s13o]

Also added support for boolean & double (number), in accordance with reference

[joelittlejohn]

@s13o Is it not possible to use getDefaultValue instead of adding this getDefaultEnum method? It seems like there is duplication here.

[s13o]

The 'getDefaultEnum' is not a new method actually. It was introduced since the first revision of the 'DefaultRule' class. The method "getDefaultValue" has used it to resolve the 'JExpression' for 'default value of Enum' even before my changes.
The only improvement I did with the 'getDefaultEnum' method that now it will check the signature of static method "fromValue" of Enum class (it is reflected in Reference). Before my change this method assumed that the only type of it's argument is a "string", but types are described here

[joelittlejohn]

I mean instead of this code here, you should be able to re-use getDefaultValue and not duplicate the if/else if block.

[s13o]

I prefere not to write spaghetti-code with dozens of if-else at all.
But you proposed me to move some more if-else from getDefaultEnum to 'getDefaultValue' and delete the getDefaultEnum method after that, is not it?
Well, not bad idea to remove the getDefaultEnum but I'll better use some AbstractFactory pattern right from the top apply method for this instead of if-else.

[joelittlejohn]

@s13o Sorry, this conversation is getting very confused.

I originally added a comment here:
#707 (comment)

What I'm saying is: there is no need to duplicate if/else blocks from getDefaultValue into getDefaultEnum. The getDefaultEnum method can call getDefaultValue if I am not mistaken. Inside getDefaultEnum (line 269) you have JType paramType, you have JsonNode node and you need a JExpression result. You have everything you need on line 269 to call getDefaultValue instead of implementing a duplicate if/else block.

[s13o]

Man, the conversation is beautiful!
I understood your idea, it is reasonable because signatures of both methods are more or less similar. And right after that I've seen that adding some more if-else into the getDefaultValue is a crime :)
Much more clever is to use something like getFactory(JType::type).apply(JsonNode::node).
I'll check later how to make it easy :)
Also I'll check your idea about recursion
Regards!

[joelittlejohn]

@s13o I appreciate you getting involved here but I simply can't bring myself to merge an additional 700 lines of code into this project to fix this problem.

There's a simple if/else block here to choose a type, and replacing this with polymorphic matchers and factories has made the whole thing a lot harder to understand IMO and introduced a lot more code to maintain. I understand you're trying to solve this problem in a general, re-usable, extensible way but I feel like this is not the answer. I have to think about the long-term maintainability of this project - it has few permanent maintainers, a lot of contributors and a lot of users.

Sorry if this feels very negative, it's not my intention to be difficult for its own sake. I can take a look at solving #706 soon and see if I can find a different solution and in the meantime you are free to use a version built from your own fork.

[s13o]

There are ~500 lines of tests were added.
In case we could use the lambda-syntax the number of code lines could to be decreased with my change, but...
There was already a simple "if-else" fix before this one without any change of any method name but it was also rejected.
Probably the #392 will never been resolved - it is definitely more than 500 lines.
But a I wish you a luck!
Have a nice time!

P.S.
Please don't be negative because of my comments here too :)
Your project is amazing, it is true.

[joelittlejohn]

Closed by #713.