Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default Serializing #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Default Serializing #2

wants to merge 4 commits into from

Conversation

gzougianos
Copy link

Added the feature to serialize objects without using a specific CsvSerializer. Instead a default one which serializes all fields (calling the getters - using java.lang.reflect package ) of the class will be used.

Snippet:

try (CsvWriter<Person> writer = CsvWriter.open(f, Person.class)) {
	writer.put(rms);
	writer.put(mneri);
}

@mneri
Copy link
Owner

mneri commented Dec 10, 2018

Hi, and thank you so much for your contribution! I really appreciate, although I need some clarifications.

DefaultCsvSerializer and DefaultCsvDeserializer should work in pair: objects serialized by the former should be deserialized by the latter without errors. In your implementation, fields are serialized in the order given by Class.getFields() method, which (as said in the documentation) doesn't guarantee any specific order. We can reasonably expect but we can't be sure DefaultCsvSerializer and DefaultCsvDeserializer read fields in the same order.

Let me give you another example.

class Person {
    private String lastName;
    private String firstName;
    // Getters and setters
}

Let's assume that Class.getFields() return fields in declaration order (which I didn't test). One day someone modifies Person class and switches lastName and firstName.

class Person {
    private String firstName;
    private String lastName;
    // Getters and setters
}

Now, csv files serialized using DefaultCsvSerializer (prior the modification) cannot be deserialized by DefaultCsvDeserializer (after the modification).

In my opinion, serialization process shouldn't be dependent of internal class details.

@gzougianos
Copy link
Author

Hi there! Let's take it one by one...

Class.getFields and order of field serialization: I made tons of tests on this one. Class.getMethods return methods in random order indeed. getFields though, in all of my tests, was returning fields in declaration order ( i was not aware of the doc, and that actually hurt me a bit :( ) so i gave it a chance. The alternative i have in mind is a kind of annotation like @order = 1 above of the field. So we have the order of the fields. In the other hand, this will not have the benefit (we)-I want, which is to serialize objects (mostly simple models) within 2 lines. Changing the field declaration order is indeed a problem (but let's face it, you do not change field declaration often, especially in models), but is solved by @order = x too.

Collections and Arrays? One more problem i was thinking after i pull requested, is what about arrays and collection fields in the class. Whatever though, we could figure this out.

Failed test of DEFAULT deserialization: This is actually on me. My test is wrong. In tests, defaultDeserialization runs before defaultSerialization, which means the file is empty, so (at least me) i get NoSuchElementEx, which makes sense. Then default serialization runs. After that, in next test, defaultDeserialization runs fine and without AssertionError, because the file is not empty. I guess the proper test should be like one method with write - serialize and then read - deserialize.

I really like the whole idea of serializing objects (and i say it again, mostly simple models) without having the need to write down an extra CsvSerializer. You could either decline it (i am fine with it, it is your project), or we could really make this work :). It is your choice, let me know.

@mneri mneri self-assigned this Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants