8
\$\begingroup\$

Note: see the next iteration.

I have this tiny library for (de)serializing lists of objects. The requirement is that each object's state may be represented textually as a single line of text. Later on, the deserialization routine attempts to map each text line to the object it encodes.

Factory.java:

package net.coderodde.lists.serial;

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

/**
 * This class holds the methods for serializing/deserializng the lists. The 
 * convention used here is that every object can be serialized to a single line
 * describing the state of that object. Upon deserialization, each row should
 * gracefully produce the object with its recorded state. For instance, the
 * format of comma-separated values (CSV) resembles this very well.
 * 
 * @author Rodion "rodde" Efremov
 * @version 1.6
 */
public class Factory {

    /**
     * Serializes the all the elements. Each element should serialize to a 
     * single line, as the deserialization routine assumes that each line
     * represents the state of exactly one element.
     * 
     * @param <E>        the actual element type.
     * @param list       the list of elements to serialize.
     * @param serializer the element serializer.
     * @return           the string representation of the entire list.
     */
    public static <E> String serialize(List<E> list, 
                                       LineSerializer<E> serializer) {
        StringBuilder sb = new StringBuilder();

        list.stream().forEach((element) -> {
            sb.append(serializer.serialize(element)).append("\n");
        });

        return sb.toString();
    }

    /**
     * Deserializes the entire text <code>text</code> to the list of elements 
     * being encoded. This routine assumes that each line describes a single 
     * element.
     * 
     * @param <E>          the actual element type.
     * @param text         the text to deserialize.
     * @param deserializer the deserialization object.
     * @return             the list of elements.
     */
    public static <E> List<E> deserialize(String text, 
                                          LineDeserializer<E> deserializer) {
        return deserialize(text, deserializer, new ArrayList<>());
    }

    /**
     * Deserializes the entire text <code>text</code> to the list of elements 
     * being encoded. This routine assumes that each line describes a single 
     * element.
     * 
     * @param <E>          the actual element type.
     * @param text         the text to deserialize.
     * @param deserializer the deserialization object.
     * @param list         the list for holding the elements.
     * @return             the list of elements.
     */
    public static <E> List<E> deserialize(String text, 
                                          LineDeserializer<E> deserializer,
                                          List<E> list) {
        if (list == null) {
            return deserialize(text, deserializer);
        }

        list.clear();
        Scanner scanner = new Scanner(text);

        while (scanner.hasNextLine()) {
            list.add(deserializer.deserialize(scanner.nextLine()));
        }

        return list;
    }
}

LineSerializer.java:

package net.coderodde.lists.serial;

/**
 * This interface defines the API for serializing an element to a string.
 * 
 * @author Rodion "rodde" Efremov
 * @version 1.6
 * @param <E> the element type.
 */
@FunctionalInterface
public interface LineSerializer<E> {

    /**
     * Returns the textual representation of the input element.
     * 
     * @param  element the element to serialize.
     * @return the textual representation of the input element.
     */
    public String serialize(E element);
}

LineDeserializer.java:

package net.coderodde.lists.serial;

/**
 * This interface defines the API for deserializing the elements from their 
 * textual representation.
 * 
 * @author Rodion "rodde" Efremov
 * @version 1.6
 * @param <E> the element type.
 */
@FunctionalInterface
public interface LineDeserializer<E> {

    /**
     * Deserializes the element from its textual representation.
     * 
     * @param  text the string representing the state of the element.
     * @return the actual, deserialized element.
     */
    public E deserialize(String text);
}

Demo.java:

package net.coderodde.lists.serial;

import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
import java.util.Random;

public class Demo {

    public static void main(final String... args) {
        // Create.
        List<Integer> input = getRandomInts(100, new Random());
        // Serialize.
        String text = Factory.serialize(input, (e) -> e.toString());
        // Deserialize.
        List<Integer> output = 
                Factory.deserialize(text, 
                                    (e) -> Integer.parseInt(e), 
                                    new LinkedList());

        System.out.println("Input list size:  " + input.size());
        System.out.println("Input list type:  " + input.getClass().getName());
        System.out.println("Output list size: " + output.size());
        System.out.println("Output list type: " + output.getClass().getName());

        for (int i = 0; i < input.size(); ++i) {
            if (!Objects.equals(input.get(i), output.get(i))) {
                throw new IllegalStateException("Lists do not agree! :-[");
            }
        }

        System.out.println("Lists agree! :-]");
    }

    private static List<Integer> getRandomInts(int size, Random random) {
        List<Integer> ret = new ArrayList<>();

        for (int i = 0; i < size; ++i) {
            ret.add(random.nextInt());
        }

        return ret;
    }
}

So, what do you think?

\$\endgroup\$
8
\$\begingroup\$

Factory is a terrible name for a class. What does it make? As best as I can tell, it doesn't make anything, it just de/serializes. Honestly, I don't think you even need it; you could easily put (de)serialize in the respective classes.

LineSerializer and LineDeserializer operate on Strings, which may very well contain newlines or carriage returns or both. It'd recommend renaming them to StringSerializer and StringDeserializer, respectively.

In serialize:

list.stream().forEach((element) -> {
    sb.append(serializer.serialize(element)).append("\n");
});

Why are you making a stream just to iterate over a List? Use an advanced for (for (a : b)):

for (E element : list) {
    sb.append(serializer.serialize(element)).append("\n");
}

Streams are designed to be used when you have to do lots of fancy stuff, like filter, map, and all that good stuff. Building one just to iterate over things is kinda a waste. Sure, it works, and it's fancy, but you may as well use the old methods.

Your method documentation is mostly good, and very complete. I like it. Keep it up. You should continue documenting even in the methods, however -- whenever you make an unintuitive choice, leave a note explaining why.

public static void main(final String... args) {
    // ...
}

You know, I've never seen it written like this. It's always been String[] args, rather than final String... args. Functionally, they're the same, but just thought I'd point out that the convention seems to be an array, not varargs.

One big issue though: What if people want to have newlines in their object definitions? It'd be more extensible to take a List<String> and iterate over that, then maybe provide an overloaded method that takes a String and splits it along platform-specific newlines and calls the normal method on the resulting List. In pseudocode that looks awfully like Ruby:

def serialize(list_of_strings, serializer)
  return list_of_strings.map { |string| serializer.serialize(string) }
end

def serialize(string, serializer)
  return serialize(string.split(/\r?\n/), serializer)
end

You know, rereading the Ruby, I just realized that if you chose to use List<String>/List<T>s, it's a lot simpler: Just use Collection#Stream, Stream#map, and Stream#toArray(IntFunction<A[]<). If you'd like an example of what I mean, I'd be happy to type up what I mean. It's a little confusing to use.

\$\endgroup\$
  • \$\begingroup\$ Any idea how I should rename Factory? \$\endgroup\$ – coderodde Jun 20 '15 at 10:06
  • \$\begingroup\$ @coderodde Its up to you, though you may want to move the methods into your interfaces (if that's possible; I can't remember off the top of my head) and get rid of Factory entirely. \$\endgroup\$ – Nic Hartley Jun 20 '15 at 15:12
  • \$\begingroup\$ @coderodde Updated my answer \$\endgroup\$ – Nic Hartley Jun 21 '15 at 3:06
6
\$\begingroup\$

I'm.... concerned about this code.

public static <E> List<E> deserialize(String text, 
                                      LineDeserializer<E> deserializer) {
    return deserialize(text, deserializer, new ArrayList<>());
}

public static <E> List<E> deserialize(String text, 
                                      LineDeserializer<E> deserializer,
                                      List<E> list) {
    if (list == null) {
        return deserialize(text, deserializer);
    }

    ...
}

For the purposes of this review, I'll refer to them by the number of arguments they take in, so 2 and 3. Let's walk through the execution path here.

  1. 3 gets called with a null arg for list.
  2. 3 calls 2.
  3. 2 calls 3 again, only this time, 2 gave 3 a new List to work with.

Why?

Wouldn't it be much simpler to just create the list inline to work with?

More importantly, why does it need a list at all? The first thing you do to it is clear it.

    list.clear();
    Scanner scanner = new Scanner(text);

    while (scanner.hasNextLine()) {
        list.add(deserializer.deserialize(scanner.nextLine()));
    }

    return list;

I think you minimally owe your future self some comments as to why it was done this way, if not a general rethink about what you're doing here.

\$\endgroup\$
  • 1
    \$\begingroup\$ Oh. Bummer.. Did not notice that. \$\endgroup\$ – coderodde Jun 19 '15 at 19:47
  • 1
    \$\begingroup\$ Dangit, I was gonna bring up the point about clearing the list without using it \$\endgroup\$ – Nic Hartley Jun 19 '15 at 19:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.