You'll get into religious wars here, but let me say that I am on the "ignore SRP" side on this one. Yes, you are coupling to a specific Serialization scheme, as correctly pointed out by @amon. But, in this example, you are coupling to a very abstract and standard scheme, which makes it "o.k.". Lots of Java classes have valueOf(String), and all have toString(), and the world hasn't ended due to lack of SRP.
SRP exists not because SRP itself a goal, but because SRP lessens the effect of change, an admirable goal. In this example, what is more likely to change?
Product, e.g. by adding a new field named newFeature, changing it to an array, etc... or- The JSON specification and library that you use.
In my experience, "#1" is always the right answer. YMMV.
If Product changes, and fromJSON() is a method of Product, one class changes. And it was going to change anyway, so there's no "extra" change. Also, you could keep the new fields of Product private and final, increasing data-hiding and thread-safety.
OTOH, if you are using a ProductBuilder or ProductSerializer layer, then two classes change. You are adding stuff about newFeature in multiple places, which violates DRY. It is trickier to keep the new field private and final. It is just plain extra work and tedium for little or no benefit, violating LEAN or YAGNI. See, I can cite principles too. :-)
One more thing: consider making fromJSON a real instance method, not a static class function. (It still returns a new Product object) Yes, it feels weird, and you may decide against it. But we "just tried it" in one project and it was wonderful. You may need to add a final static Product.PROTOTYPE (or similar - maybe call it builder?). But, it opens up some good features:
- It can go into an interface,
JSONable. Static methods cannot be in an interface
- Now you can have a
Collection<JSONable> which suddenly does a lot for you.
- You can provide a testing version, a mock version, etc. If there is a class for which @amon is right and you really do want to decouple the logic into a
ProductBuilder, you could call it here.
Caveats:
If you do change JSON libraries, you will have to change a lot of your classes, so be sure to write them in an intelligent way so that the JSON code is abstracted, as it should be, and as all the libraries do. This is easy.
And, if you were serializing to some weird, offbeat, non-standard, very specific, and likely to change format, yes, then I agree with @amon that the code should be separated. Or it you were tracking somebody else's JSON and they kept changing the names of their field, that would be an issue. Or if the parsing is exceedingly complex, involves combining / "joining" a couple of JSON streams, etc.
Best Answer
Downsides of reflection in general
Reflection is harder to understand than straight-line code.
In my experience, reflection is an "expert-level" feature in Java. I would argue that most programmers never use reflection actively (i.e. consuming libraries that use reflection doesn't count). That makes code using it harder to understand for these programmers.
Reflection code is inaccessible to static analysis
Suppose I have a getter
getFooin my class and I want to rename it togetBar. If I use no reflection, I can just search the code base forgetFooand will find every place that uses the getter so I can update it, and even if I miss one, the compiler will complain.But if the place that uses the getter is something like
callGetter("Foo")andcallGetterdoesgetClass().getMethod("get"+name).invoke(this), then the above method won't find it, and the compiler won't complain. Only when the code is actually executed will you get aNoSuchMethodException. And imagine the pain you're in if that exception (which is tracked) is swallowed bycallGetterbecause "it's only used with hard-coded strings, it can't actually happen". (Nobody would do that, someone might argue? Except that the OP did exactly that in his SO answer. If the field is renamed, users of the generic setter would never notice, except for the extremely obscure bug of the setter silently doing nothing. Users of the getter might, if they're lucky, notice the console output of the ignored exception.)Reflection code is not type-checked by the compiler
This is basically a big sub-point of the above. Reflection code is all about
Object. Types are checked at runtime. Errors are discovered by unit tests, but only if you have coverage. ("It's just a getter, I don't need to test it.") Basically, you lose the advantage using Java over Python gained you in the first place.Reflection code is unavailable for optimization
Maybe not in theory, but in practice, you won't find a JVM that inlines or creates an inline cache for
Method.invoke. Normal method calls are available for such optimizations. That makes them a lot faster.Reflection code is just slow in general
The dynamic method lookup and type checking necessary for reflection code is slower than normal method calls. If you turn that cheap one-line getter into a reflection beast, you might (I have not measured this) be looking at several orders of magnitude of slowdown.
Downside of generic getter/setter specifically
That's just a bad idea, because your class now has no encapsulation anymore. Every field it has is accessible. You might as well make them all public.