3
\$\begingroup\$

The gist of the code is that there's an assembly line of products, in this case Toy and Book, that get inspected by workers who then add them to a box. The box is then sent to someone who knows what to do when they receive a box for a given product. I made the fields of Product, Toy, and Book public to shorten the code because it is already on the long side (just under 200 lines), though it is easy to read. A potential downside I noticed is that if I were to introduce a new product, then I would need to add a new Product class, a new AssemblyWorker class, new Receiver class, and another else if statement in the fetchWorker method. Is this normal or is my approach flawed?

Any feedback will be greatly appreciated.

package org.poly;

import java.util.*;

class Product {
    int id;
    double cost;
    public String toString() {
        return id+" "+cost;
    }
}

class Toy extends Product {
    String name;
    public String toString() {
        return super.toString()+" "+name;
    }
}

class Book extends Product {
    String title;
    String author;
    public String toString() {
        return super.toString()+" "+title+" "+author;
    }
}

class ProductBox<T extends Product> {
    private List<T> products;

    ProductBox() {
        products = new ArrayList<>();
    }

    void addProduct(T product) {
        products.add(product);
    }

    T getProduct(int idx) {
        if (idx < 0 || idx >= products.size())
            throw new ArrayIndexOutOfBoundsException();
        return products.get(idx);
    }

    List<T> getProducts() {
        return products;
    }
}

abstract class AssemblyWorker {

    void startAssemblyLine(String text) {
        Scanner scanner = new Scanner(text);
        while (scanner.hasNext()) {
            List<String> tokens = new ArrayList<>(
                    Arrays.asList(scanner.nextLine().split(",")));
            Iterator<String> it = tokens.iterator();
            inspectProduct(it);
        }
    }

    abstract void inspectProduct(Iterator<String> it);
    abstract Receiver deliverProduct();
}

class ToyWorker extends AssemblyWorker {
    private ProductBox<Toy> productBox;

    ToyWorker() {
        productBox = new ProductBox<>();
    }

    void inspectProduct(Iterator<String> it) {
        Toy toy = new Toy();
        while (it.hasNext()) {
            String next = it.next();
            if (next.matches("ID") && it.hasNext()) {
                toy.id = Integer.parseInt(it.next());
            } else if (next.matches("NAME") && it.hasNext()) {
                toy.name = it.next();
            } else if (next.matches("COST") && it.hasNext()) {
                toy.cost = Double.parseDouble(it.next());
            }
        }
        productBox.addProduct(toy);
    }

    public Receiver deliverProduct() {
        return new ToyReceiver(productBox);
    }
}

class BookWorker extends AssemblyWorker {
    private ProductBox<Book> productBox;

    BookWorker() {
        productBox = new ProductBox<>();
    }

    void inspectProduct(Iterator<String> it) {
        Book book = new Book();
        while (it.hasNext()) {
            String next = it.next();
            if (next.matches("ID") && it.hasNext()) {
                book.id = Integer.parseInt(it.next());
            } else if (next.matches("TITLE") && it.hasNext()) {
                book.title = it.next();
            } else if (next.matches("AUTHOR") && it.hasNext()) {
                book.author = it.next();
            } else if (next.matches("COST") && it.hasNext()) {
                book.cost = Double.parseDouble(it.next());
            }
        }
        productBox.addProduct(book);
    }

    public Receiver deliverProduct() {
        return new BookReceiver(productBox);
    }
}

class AssemblyManager {
    static AssemblyWorker fetchWorker(String type) {
        if (type.equals("TOY")) {
            return new ToyWorker();
        } else if (type.equals("BOOK")) {
            return new BookWorker();
        }
        return null;
    }
}

interface Receiver {
    void emptyBoxContents();
}

class ToyReceiver implements Receiver{
    private ProductBox<Toy> productBox;

    ToyReceiver(ProductBox<Toy> productBox) {
        this.productBox = productBox;
    }

    public void emptyBoxContents() {
        for (Toy toy : productBox.getProducts()) {
            System.out.println(toy.toString());
        }
    }
}

class BookReceiver implements Receiver {
    private ProductBox<Book> productBox;

    BookReceiver(ProductBox<Book> productBox) {
        this.productBox = productBox;
    }

    public void emptyBoxContents() {
        for (Book book : productBox.getProducts()) {
            System.out.println(book.toString());
        }
    }
}

public class PolyTest {

    private static void testProduct(String data) {
        String[] splitString = data.split("\\n", 2);
        String type = splitString[0].split(",")[1];
        AssemblyWorker worker = AssemblyManager.fetchWorker(type);
        if (worker == null) {
            System.exit(1);
        }
        worker.startAssemblyLine(splitString[1]);
        Receiver receiver = worker.deliverProduct();
        receiver.emptyBoxContents();
    }

    public static void main(String[] args) {
        String toyData = ""+
                "TYPE,TOY\n"+
                "ID,0,NAME,SPIDERMAN,COST,10\n"+
                "ID,1,NAME,GODZILLA,COST,20\n";

        String bookData = ""+
                "TYPE,BOOK\n"+
                "ID,2,TITLE,PROGRAMMING IN C,AUTHOR,STEPHEN KOCHAN,COST,30\n"+
                "ID,3,TITLE,DATA STRUCTURES AND PROBLEM SOLVING,AUTHOR,MARK ALLEN WEISS,COST,40\n";

        System.out.println("Toys:");
        testProduct(toyData);
        System.out.println("Books:");
        testProduct(bookData);
    }
}
\$\endgroup\$
2
\$\begingroup\$

Below are some general remarks and some ideas on how to refactored the code to avoid creating multiple classes when adding a new product (java 8 is assumed):

ProductBox

In the method getProduct, you do not need to check the index validity, the get method of the products list will do it for you. So the getProduct method should simply be :

 public P getProduct(int idx) {
     return products.get(idx);
 }

Do not return the list with getProducts. If you are wrapping the list into a specific class, then why should you provide access to it with a getter : use the list directly instead of ProductBox in that case.

If you need to iterate over the products in the ProductBox, then implement the Iterable interface :

ProductBox

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class ProductBox<P extends Product> implements Iterable<P> {

    private final List<P> products = new ArrayList<>();

    public void addProduct(P product) {
        products.add(product);
    }

    public P getProduct(int idx) {
        return products.get(idx);
    }

    @Override
    public Iterator<P> iterator() {
        return products.iterator();
    }
}

Prepare your input data

As noted by Timothy Truckle, all input lines are lists of key,value pairs. I would then convert a line into a Map<String,String>.

Afterward, you needs some int or double value in that map. To centralized this, I would put the map into a class that will expose some methods getInt or getDouble. This will lighten your code when building a Product (no more Integer.parseInt but something linke propertyHelper.getInt.) and you will be able to handle errors a bit better.

Below is an example of the two classes that can be used for that :

LineParser

import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

public class LineParser {

    /**
     * Use this function to convert a line into a map (assuming the line is of the form <code>key1,value1,key2,value2,...</code>)
     */
    public static final Function<String,Map<String,String>> TO_MAP = line -> new LineParser(line).parse();

    /**
     * Function to convert a line into a {@link PropertyHelper} (assuming the line is of the form <code>key1,value1,key2,value2,...</code>)
     */
    public static final Function<String,PropertyHelper> TO_PROPERTY_HELPER = line -> new PropertyHelper(line, TO_MAP.apply(line));

    /**
     * the line to parse
     */
    private final String inputLine;

    /**
     * the tokens obtained by splitting the line
     */
    private String[] tokens;

    /**
     * the result of the parsing
     */
    private Map<String,String> result;

    private LineParser(String inputLine) {
        this.inputLine = inputLine;
    }

    private Map<String,String> parse() {
        this.splitInputLineIntoTokens();
        if (!areTokensValid()) {
            throw new IllegalArgumentException("Invalid input line. Cannot split into valid tokens : "+ inputLine);
        }
        this.createResultMapFromTokens();
        return result;
    }

    private void splitInputLineIntoTokens() {
        this.tokens = inputLine.split(",");
    }

    private boolean areTokensValid() {
        return areTokensInEvenNumber();
    }

    private boolean areTokensInEvenNumber() {
        return tokens.length % 2 == 0;
    }

    private void createResultMapFromTokens() {
        this.result = new HashMap<>();
        for (int keyIndex = 0, valueIndex = 1; valueIndex < tokens.length; keyIndex+=2, valueIndex+=2) {
            this.result.put(tokens[keyIndex], tokens[valueIndex]);
        }
    }
}

PropertyHelper

import java.util.Map;
import java.util.Optional;

public class PropertyHelper {

    /**
     * The line used to produce the property map.
     */
    private final String inputLine;

    /**
     * the map obtained by parsing the input line.
     */
    private final Map<String,String> properties;

    public PropertyHelper(String inputLine, Map<String,String> properties) {
        this.inputLine = inputLine;
        this.properties = properties;
    }

    /**
     * @return the value associated with the provided key
     * @throws IllegalArgumentException is the key is not associated with a value
     */
    public String getString(String key) {
        return Optional.ofNullable(properties.get(key)).orElseThrow(() -> createException(key, "No such property"));
    }

    /**
     * @return the int value associated with the provided key
     * @throws IllegalArgumentException is the key is not associated with an int value
     */
    public int getInt(String key) {
        final String value = getString(key);
        try {
            return Integer.parseInt(value);
        } catch (Exception e) {
            throw createException(key, "Could not get an int from value '"+value+"'", e);
        }
    }

    /**
     * @return the double value associated with the provided key
     * @throws IllegalArgumentException is the key is not associated with a double value
     */
    public double getDouble(String key) {
        final String value = getString(key);
        try {
            return Double.parseDouble(value);
        } catch (Exception e) {
            throw createException(key, "Could not get a double from value '"+value+"'", e);
        }
    }

    private IllegalArgumentException createException(String key, String message) {
        return new IllegalArgumentException("Error while parsing line '"+inputLine+"' for key '"+key+"' : "+message);
    }
    private IllegalArgumentException createException(String key, String message, Exception cause) {
        return new IllegalArgumentException("Error while parsing line '"+inputLine+"' for key '"+key+"' : "+message, cause);
    }
}

Use static method as product factory

In each product class, I would add a static method that takes a PropertyHelper and returns the built product. For instance for a book:

Book

public class Book extends Product {

    public static final String BOOK_TITLE_KEY = "TITLE";
    public static final String BOOK_AUTHOR_KEY = "AUTHOR";

    public static Book create(PropertyHelper propertyHelper) {
        return new Book(
                propertyHelper.getInt(PRODUCT_ID_KEY),
                propertyHelper.getDouble(PRODUCT_COST_KEY),
                propertyHelper.getString(BOOK_TITLE_KEY),
                propertyHelper.getString(BOOK_AUTHOR_KEY)
        );
    }

    private final String title;

    private final String author;

    public Book(int id, double cost, String title, String author) {
        super(id, cost);
        this.title = title;
        this.author = author;
    }

    //... getters and ToString
}

with

Product

public abstract class Product {

    public static final String PRODUCT_ID_KEY = "ID";
    public static final String PRODUCT_COST_KEY = "COST";

    private final int id;

    private final double cost;

    public Product(int id, double cost) {
        this.id = id;
        this.cost = cost;
    }

    //... getters and ToString    
}

You need one AssemblyWorker only

Your AssemblyWorker needs two things:

  1. a function that convert a string to a product (which is then added to a product box)
  2. a function that convert a product box to a receiver

Instead of providing these functions by extending AssemblyWorker, use delegation by providing Functions at construction:

AssemblyWorker

import java.util.Arrays;
import java.util.function.Function;

public class AssemblyWorker<P extends Product> {

    private final Function<? super PropertyHelper, ? extends P> productFactory;

    private final Function<? super ProductBox<P>,? extends Receiver> receiverFactory;

    private final ProductBox<P> productBox = new ProductBox<>();

    public AssemblyWorker(Function<? super PropertyHelper,? extends P> productFactory, Function<? super ProductBox<P>,? extends Receiver> receiverFactory) {
        this.productFactory = productFactory;
        this.receiverFactory = receiverFactory;
    }

    void startAssemblyLine(String text) {
        Arrays.stream(text.split("\\n")) //stream line by line
                .map(LineParser.TO_PROPERTY_HELPER) //parse the line
                .map(productFactory) //create the product
                .forEach(productBox::addProduct); //add the product to the box
    }

    public Receiver deliverProduct() {
        return receiverFactory.apply(productBox);
    }
}

AssemblyManager

Use a map instead of multiple if/else statements. For a given type, you need an AssemblyWork. That looks like a key/value pattern. With all the classes above, you can define a map of type Map<String,Supplier<AssemblyWorker<?>>>: with a type you get a supplier that create the right AssemblyWorker<?>. If you add a new product, just add an entry to the map.

Also, thank to lamba expression and the static factory method of the products, the instantiation of an AssemblyWorker<?> is quite easy to write.

AssemblyManager

class AssemblyManager {

    public static final String TOY_TYPE = "TOY";
    public static final String BOOK_TYPE = "BOOK";

    private static final Map<String,Supplier<AssemblyWorker<?>>> ASSEMBLY_WORKER_MAP;

    static {
        ASSEMBLY_WORKER_MAP = new HashMap<>();
        ASSEMBLY_WORKER_MAP.put(TOY_TYPE , () -> new AssemblyWorker<>(Toy::create, ToyReceiver::new));
        ASSEMBLY_WORKER_MAP.put(BOOK_TYPE, () -> new AssemblyWorker<>(Book::create, BookReceiver::new));
    }

    public static AssemblyWorker<?> fetchWorker(String type) {
        return Optional.ofNullable(ASSEMBLY_WORKER_MAP.get(type))
                       .orElseThrow(() -> new IllegalArgumentException("Unknown product type '"+type+"'"))
                       .get();
    }
}

And then the PolyTest class is then:

PolyTest

public class PolyTest {

    private static void testProduct(String data) {
        final String[] splitString = data.split("\\n", 2);
        final PropertyHelper propertyHelper = LineParser.TO_PROPERTY_HELPER.apply(splitString[0]);

        final String type = propertyHelper.getString("TYPE");
        final AssemblyWorker<?> worker = AssemblyManager.fetchWorker(type);

        worker.startAssemblyLine(splitString[1]);

        final Receiver receiver = worker.deliverProduct();

        receiver.emptyBoxContents();
    }

    public static void main(String[] args) {
        String toyData = ""+
                "TYPE,TOY\n"+
                "ID,0,NAME,SPIDERMAN,COST,10\n"+
                "ID,1,NAME,GODZILLA,COST,20\n";

        String bookData = ""+
                "TYPE,BOOK\n"+
                "ID,2,TITLE,PROGRAMMING IN C,AUTHOR,STEPHEN KOCHAN,COST,30\n"+
                "ID,3,TITLE,DATA STRUCTURES AND PROBLEM SOLVING,AUTHOR,MARK ALLEN WEISS,COST,40\n";

        System.out.println("Toys:");
        testProduct(toyData);
        System.out.println("Books:");
        testProduct(bookData);
    }
}

Receivers

The code does not provide enough information on how the receiver handles the ProductBox to perform any optimization.

If the emptyContents() method cares on the type of the products in the box only individually, you might be able to use only one Receiver and to use the visitor Pattern to differentiate the actions for each products.

\$\endgroup\$
  • \$\begingroup\$ Why AssemblyManager instead of the much more domain-fitting (and much more semantically meaningful) AssemblyLine? \$\endgroup\$ – Vogel612 Jan 5 '17 at 16:27
2
\$\begingroup\$

Spaces are not only cheap but free. These free spaces will add readability to your code and makes it easier to maintain.

class Book extends Product {
    String title;
    String author;
    public String toString() {
        return super.toString()+" "+title+" "+author;
    }
}  

would be better like so

class Book extends Product {
    String title;
    String author;
    public String toString() {
        return super.toString() + " " + title + " " + author;
    }
}  

or you can use String.format().

\$\endgroup\$
2
\$\begingroup\$

favour immutable DTOs

Your class Product and all its decendants look line Data Transfer Objects which means they have no business logic that might change any of their properties during the programs runtime. You should make such object immutable by defining their properies final. In consequence you need to set the properties via contructor:

class Product {
    final int id;
    final double cost;
    Product(int id, double cost){
      this.id = id;
      this.cost = cost;
    }
    public String toString() {
        return id+" "+cost;
    }
}

favour composition over inheritance

one problem from the prevoius point is that the constructor parameter list grows with every inheritance level. Another Problem is the equals/hashcode dilema.

The way to go around this is not having the classes inherit from each other. Rather express the inheritance via interfaces:

interface IProduct {
}

interface IToy extends IProduct {
}

class Product implements IProduct{
  // ...
}

class Toy implements IToy{
  private final IProduct product;
  private final String name;
  Toy(String name, IProduct product){
    this.name = name;
    this.product = product;
  }
  public String toString() {
      return product.toString()+" "+name;
  }
}

Then use it this way:

/** throws NoSuchElementException if Toy information is incomplete. */
void inspectProduct(Iterator<String> it) {
    Map<StringString> properties = new HashMap();
    for(int i = 0 ; i< TOY_PROPERTIES_COUNT_IS_3; i++ ) {
        String next = it.next();
        properties.put(next,it.next());
    }
    Toy toy = new Toy( properties.get("NAME"),
                       new Product( Integer.paresInt(properties.get("ID"),
                                    Double.parseDouble(properties.get("COST"))));
    productBox.addProduct(toy);
}

Do not access object fields directly

The previous point raises another issue with your example code. You cannot access the "parents" fields via direct access.

Never use direct access!

With DTOs use getters (and setters if mutabibilty cannot be avoided) which you declare in the interface(es).

Any other object type sould not allow access to its properties, therefore they should not have getters/setters.

\$\endgroup\$
  • \$\begingroup\$ The code you present can also throw NumberFormatException in case the Toy information is malformed... \$\endgroup\$ – Vogel612 Jan 4 '17 at 12:19
  • \$\begingroup\$ @Vogel612 yes, but NumberFormatException may have occured with the original code too, the possibility of NoSuchElementException has been introduced by my change... \$\endgroup\$ – Timothy Truckle Jan 4 '17 at 14:33

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.