bluelinelabs / LoganSquare

Screaming fast JSON parsing and serialization library for Android.
Apache License 2.0
3.21k stars 307 forks source link

Allow constructor injection. #3

Open evant opened 9 years ago

evant commented 9 years ago

Oftentimes you want your model object to be immutable so you don't want to make your fields public or provide setters. Would it be possible to support constructor injection for this usecase? Something like:

@JsonObject
public class Image {

    @JsonField(name = "_id")
    private int imageId;

    @JsonField
    private String format;

    @JsonField
    private String url;

    @JsonField
    private String description;

    @JsonField(name = "similar_images")
    private List<Image> similarImages;

   // Not sure if the arguments need to be annotated instead/as well.
   @JsonConstructor
   public Image(int imageId, String format, String url, String description, List<Image> similarImages) {
       this.imageId = imageId;
       this.format = foramt;
       this.url = url;
       this.description = description;
       this.similarImages = similarImages;
   }

   public int getImageId() {
      return getImageId;
   }

   public String getFormat() {
      return format;
   }

   public String getUrl() {
      return url;
   }

   public List<Image> getSimilarImages() {
      return Collections.unmodifiableList(similarImages);
   }
}
EricKuck commented 9 years ago

Interesting idea. I think this has to be fleshed out a bit to make sure that it isn't more error prone than what we have now and that resulting code isn't a big pile of ugly. I see lots of utility in something like this though.

andaag commented 9 years ago

I suspect you'd need annotations on all the arguments as well, which would make it quite annotation .. verbose.

How about not making fields private, but instead package local. Then the annotator could create the constructor file in the same package and set the fields? Not quite immutable, but better than public on everything.

evant commented 9 years ago

I'd be perfectly ok with the annotation on the constructor arguments instead of the fields. That should be sufficient right? Since the fields are manually set in the class anyway.

EricKuck commented 9 years ago

@andaag You can actually already do package local everything (constructor, fields, getters/setters). You can even do private fields with public/package accessors. Doing this is good enough for all the scenarios I can think of off the top of my head.

@evant do you have an example of where this wouldn't suffice?

evant commented 9 years ago

Hm, that does alleviate most of my concerns. Package local doesn't quite logically match the semantics I would want to have, but it would be a workable solution. It might be more convenient for constructing instances of the class not through serialization, (unit tests for example) though I guess you can just add that in addition to an empty constructor. You will have to check your constraints both in the constructor and in onParseComplete() though again, that could be workable.

All in all, I think this would make some cases slightly cleaner, but it's not strictly necessary.

evant commented 9 years ago

One thing I thought of is you could chose not to have a backing field, or choose to store it in a different way, as long as you have an argument and getter your good. I'm not sure how often that would come up in practice though.

EricKuck commented 9 years ago

Another possible solution for this would be to use something akin to the Builder pattern. Here's kinda what I'm thinking:

public class ImmutableClassExample {
    private final String mImmutableString;
    private final int mImmutableInt;

    public ImmutableClassExample(ImmutableClassBuilder builder) {
        mImmutableString = builder.getString();
        mImmutableInt = builder.getInteger();
    }

    @JsonObject
    public static class ImmutableClassBuilder {

        @JsonField
        private String mString;

        @JsonField
        private int mInt;

        public String getString() {
            return mString;
        }

        public void setString(String string) {
            mString = string;
        }

        public int getInteger() {
            return mInt;
        }

        public void setInteger(int integer) {
            mInt = integer;
        }
    }
}

In this case, you'd be parsing and serializing the ImmutableClassBuilder instead of the actual ImmutableClass object. Since the Builder pattern is used so much throughout Java, this seems like a pretty natural solution to me.

mannodermaus commented 9 years ago

The proposition made by @evant could also allow for generic types being compatible with LoganSquare, right? Jackson can deduce the type of a generic object from its annotation in the constructor, so would that be a possibility for this as well?

@JsonObject
public class GenericObject<T> {

    @JsonField(name="value")
    public int value;   // Not generic, will be parsed without any complications

    public T field;     // Generic, couldn't be deduced by LoganSquare if it were to be annotated here

    public GenericObject(@JsonField(name="field") T field) {
        this.field = field;
    }
}
EricKuck commented 9 years ago

@aurae Jackson gets away with this because it uses runtime annotation processing (which is why it's so much slower than this library). Since this library does everything at compile-time, there's no way to know what types you're going to pass in.

mannodermaus commented 9 years ago

I see, that makes total sense!

kunjan-a commented 7 years ago

I agree with @evant . Annotating constructor arguments seems a better solution than the current situation. The only issue with the builder approach @EricKuck suggested is that while it helps during deserialization, while serializing you have to again create an object of builder from the immutable object and then serialize the builder. This seems counter intuitive as a design pattern.

Honesty I have used @JsonCreator exposed by Jackson and although its a little verbose at least it ensures that we can use immutable objects naturally.

renannprado commented 7 years ago

any news on this? I would not like to have to switch to jackson to have this feature :(

Alexander-- commented 7 years ago

Implemented in this PR — https://github.com/bluelinelabs/LoganSquare/pull/213