5
\$\begingroup\$

I have to implement a relatively basic order system in Java for part of the process of a job interview. At the moment I have working code that allows for the creation, and updating of orders for an imaginary company that manufactures bricks.

Before I submit the codebase to the company I am interviewing for I want my code to be reviewed so that I can improve any issues that exists within my program. I have tried to be as professional as possible with my development; obeying Java conventions and best practices, utilising JUnit testing to ensure my code works, keeping in mind SOLID principles and attempting to use them within my code to the best of my ability.

This is the first time I have attempted to produce professional-level (for a graduate with no experience) code, so I would love for someone to be able to review my program and tell me where I can improve.

I obviously don't want to post the entire program within this thread, so I'll post a few of the bigger classes. If you'd like to see the entire program, please just ask and I'd be happy to send it to you.

OrderSystem.java

package core;

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

/*
 * Design principles ideas to implement
 * - Reduce responsibility of this class, move excess functionality into different classes
 *   -> Order builder class, instead of building orders within this class
 *   -> Printer class?
 *   -> Reference number generator class?
 */

public class OrderSystem {

    /*
     * The amount of orders that have been created by the system in this session.
     */
    private int ordersCreated = 0;

    /*
     * The order db that stores the information of all created orders.
     */
    private static OrderDB db;

    public OrderSystem()
    {
        db = new OrderDB();
    }

    /*
     * Utilises OrderBuilder class to construct a BrickOrder object
     * using the number of bricks on the order, as a parameter.
     * 
     * Reference number is generated and is unique to the order that
     * has been created. Reference number is returned by the method 
     * after the BrickOrder object has been created and added into 
     * the database of orders. 
     */
    public String createOrder(int numBricks)
    {
        OrderBuilder builder = new OrderBuilder();
        BrickOrder o = builder.constructOrder(ordersCreated, numBricks);
        incOrdersCreated(); //To keep refNum unique to each order. 
        db.addOrder(o);
        System.out.println("Order ref " + o.getReferenceNumber() + " for " + o.getNumberOfBricks() + " bricks, created and added to DB.");
        return o.getReferenceNumber();
    }

    public String updateOrder(String refNum, int newNumBricks)
    {
        ArrayList<BrickOrder> orderList = db.getOrderList();

        for(BrickOrder o : orderList)
        {
            if(refNum.equals(o.getReferenceNumber()))
            {
                o.setNumberOfBricks(newNumBricks);
                System.out.println("Order " + refNum + " details updated to " + newNumBricks + " bricks.");
                return o.getReferenceNumber();
            }
        }

        return "No order with reference number " + refNum + " in database.";
    }


    /*
     * Return a specific order from the database using a reference number
     */
    public BrickOrder getOrder(String refNum)
    {
        return db.getOrder(refNum);
    }

    /*
     * Return all of the orders in the database in the form of an ArrayList
     */
    public ArrayList<BrickOrder> getOrders()
    {
        return db.getOrderList();
    }

    /*
     * Print the details of all orders present in the database to the console. 
     */
    public void printAllOrders()
    {
        ArrayList<BrickOrder> orderList = db.getOrderList();
        Printer pr = new Printer();
        pr.printOrderList(orderList);
    }

    /*
     * Remove a specific order from the database using a reference number.
     */
    public void removeOrder(String refNum)
    {
        db.removeOrder(refNum);
    }

    /*
     * Return the number of orders created in this session. 
     */
    public int getOrdersCreated()
    {
        return ordersCreated;
    }

    /*
     * Increments the value of ordersCreated
     */
    private void incOrdersCreated()
    {
        ordersCreated++;
    }

    /*
     * Returns a reference to the db.
     */
    public OrderDB getDb()
    {
        return db;
    }

}

OrderSystemTest.java

package test;

import static org.junit.Assert.*;

import java.util.ArrayList;

import org.junit.Before;
import org.junit.Test;

import core.BrickOrder;
import core.OrderSystem;

public class OrderSystemTest {

    private OrderSystem sys;

    @Before
    public void setup()
    {
        sys = new OrderSystem();
        sys.getDb().clearDb();
    }

    /* A reference number must be returned after a new order
     * has been created.
     */
    @Test   
    public void testCreateOrderReturnsReference()
    {
        assertEquals(sys.createOrder(20), "BRICK1");
    }

    /*
     * A reference number returned by a created order must be unique 
     * to the order that has just been created.
     */
    @Test
    public void testCreateOrderReturnsUniqueReference()
    {
        sys.createOrder(1);
        sys.createOrder(1);
        sys.createOrder(1); //Create three orders 

        BrickOrder o = sys.getOrder("BRICK1"); //Get details of first order
        assertFalse(o.getReferenceNumber().equals("BRICK2"));
        assertFalse(o.getReferenceNumber().equals("BRICK3")); //Check ref number is unique

        o = sys.getOrder("BRICK2"); //Get details of second order
        assertFalse(o.getReferenceNumber().equals("BRICK1"));
        assertFalse(o.getReferenceNumber().equals("BRICK3")); //Check ref number is unique

        o = sys.getOrder("BRICK3"); //Get details of third order
        assertFalse(o.getReferenceNumber().equals("BRICK2"));
        assertFalse(o.getReferenceNumber().equals("BRICK1")); //Check ref number is unique


    }

    /*
     * Reference numbers of created orders must follow the correct 
     * trend of increasing reference numbers.
     */
    @Test
    public void testCreateOrderSetsCorrectReference()
    {
        sys.createOrder(1);
        sys.createOrder(1);
        sys.createOrder(1);
        sys.createOrder(1);
        sys.createOrder(1);
        sys.createOrder(1);
        assertEquals(sys.createOrder(5), "BRICK7");
    }

    /*
     * Created orders must set the number of bricks within the order object. 
     */
    @Test
    public void testCreateOrderSetsNumberOfBricks()
    {
        sys.createOrder(15);
        BrickOrder o = sys.getOrder("BRICK1");
        assertEquals(o.getNumberOfBricks(), 15);
    }

    /*
     * If a user tries to attempt to retrieve an order that doesn't
     * exist or uses an invalid reference number, null is returned.
     */
    @Test
    public void testReturnOfInvalidOrderReturnsNothing()
    {
        assertTrue(sys.getOrder("ROCK1") == null);
    }

    /*
     * When a specific reference number is used to retrieve
     * an order from the database, the correct details of
     * the order must be returned. 
     */
    @Test
    public void testOrderDetailsReturnedCorrectly()
    {
        sys.createOrder(50);
        sys.createOrder(43);
        sys.createOrder(112);
        BrickOrder o = sys.getOrder("BRICK2");
        assertEquals(o.getReferenceNumber(), "BRICK2");
        assertEquals(o.getNumberOfBricks(), 43);
    }


    /*
     * When a "get orders" request is submitted, all of the active orders
     * are returned in the form of an ArrayList. Each order's specific 
     * details are then accessible via the ArrayList get() method.
     */
    @Test
    public void testFullOrderListReturnedCorrectly()
    {
        sys.createOrder(10);
        sys.createOrder(100);
        sys.createOrder(1000); //Create three orders

        ArrayList<BrickOrder> orderList = sys.getOrders(); //Return full order list

        BrickOrder o = orderList.get(0);
        assertEquals(o.getReferenceNumber(), "BRICK1");
        assertEquals(o.getNumberOfBricks(), 10);

        o = orderList.get(1);
        assertEquals(o.getReferenceNumber(), "BRICK2");
        assertEquals(o.getNumberOfBricks(), 100);

        o = orderList.get(2);
        assertEquals(o.getReferenceNumber(), "BRICK3");
        assertEquals(o.getNumberOfBricks(), 1000);
    }

    /*
     * The number of orders created in this session is recorded in
     * the "createdOrders" variable. Correct function of this variable 
     * is crucial for correct reference number generation This variable 
     * is incremented by 1 each time the createOrder() method is called. 
     * This number does not decrease when orders are removed from the database.
     */
    @Test
    public void testOrdersCreatedIncrementsCorrectly()
    {
        assertEquals(sys.getOrdersCreated(), 0);
        sys.createOrder(1);
        sys.createOrder(1);
        sys.createOrder(1);
        assertEquals(sys.getOrdersCreated(), 3);
        sys.removeOrder("BRICK2");
        sys.removeOrder("BRICK3");
        assertEquals(sys.getOrdersCreated(), 3);
        sys.createOrder(1);
        assertEquals(sys.getOrdersCreated(), 4);
    }

    /*
     * After an order is successfully updated, the reference
     * number from the order must be returned.
     */
    @Test
    public void testUpdateOrderReturnsCorrectReferenceNumber()
    {
        sys.createOrder(5);
        assertEquals(sys.updateOrder("BRICK1", 10), "BRICK1");
    }

    @Test
    public void testUpdateOrderUpdatesOrderDetailsCorrectly()
    {
        sys.createOrder(5); //Create initial order
        sys.updateOrder("BRICK1", 100); //Update order with matching reference number
        BrickOrder o = sys.getOrder("BRICK1"); //Pull order information from db
        assertEquals(o.getNumberOfBricks(), 100); //Ensure order details have been updated.
    }

    @Test
    public void testUpdateOrderHandlesInvalidReferenceNumber()
    {
        sys.createOrder(5); //Create initial order
        assertEquals(sys.updateOrder("ROCK1", 100), "No order with reference number ROCK1 in database."); //Update order with matching reference number
    }


}

OrderDB.java

package core;

import java.util.ArrayList;
/*
 * OrderDB class stores all of the orders created by OrderSystem
 * within an ArrayList<BrickOrder> variable. The stored ArrayList
 * can be returned to OrderSystem at any time for updating and 
 * information extraction. 
 */
public class OrderDB {

    private ArrayList<BrickOrder> database;

    public OrderDB()
    {
        database = new ArrayList<BrickOrder>();
    }

    /*
     * Returns the order that corresponds to the reference number passed.
     * 
     * Correct order is found by iterating through DB entries and
     * comparing refNum to reference number stored in orders. Once
     * match is found, the order is returned.
     */
    public BrickOrder getOrder(String refNum)
    {   
        for(BrickOrder o : database)
        {
            if(refNum.equals(o.getReferenceNumber()))
                return o;
        }

        return null;
    }

    /*
     * Adds a passed order into the db arraylist.
     */
    public void addOrder(BrickOrder o)
    {
        database.add(o);
    } 

    /*
     * Removes an order that matches the passed reference number
     * from the order db.
     */
    public void removeOrder(String refNum)
    {
        try
        {
            int index = -1;
            for(BrickOrder o : database) //Iterate through the whole database
            {
                if(refNum.equals(o.getReferenceNumber()))
                    index = database.indexOf(o); //Save index of matched order - Prevents concurrency exception
            }

            database.remove(index);
            System.out.println("Order with reference number " + refNum + " removed.");
        }
        catch(IndexOutOfBoundsException e) //Catch exception when no matching order found
        {
            System.out.println("No order with reference number: " + refNum + "."); 
        }
    }

    public ArrayList<BrickOrder> getOrderList()
    {
        return database;
    }

    public void clearDb()
    {
        database.clear();
        System.out.println("DB cleared");
    }

}

OrderDBTest.java

package test;

import static org.junit.Assert.*;

import org.junit.Before;
import org.junit.Test;

import core.BrickOrder;
import core.OrderDB;
import core.OrderSystem;

public class OrderDBTest {

    private OrderSystem sys;

    @Before
    public void setup()
    {
        System.out.println("\nSetup");
        sys = new OrderSystem();
        sys.getDb().clearDb();
    }


    @Test
    public void testOrderDatabaseAddsOrdersCorrectly()
    {
        System.out.println("testOrderDatabaseAddsOrders");
        sys.getDb().clearDb();
        sys.createOrder(25);
        assertEquals(sys.getDb().getOrderList().size(), 1);

        sys.createOrder(30);
        assertEquals(sys.getDb().getOrderList().size(), 2);

        sys.createOrder(35);
        assertEquals(sys.getDb().getOrderList().size(), 3);
    }


    @Test
    public void testOrderDatabaseRemovesOrdersCorrectly()
    {
        System.out.println("testOrderDatabaseRemovesOrdersCorrectly");
        sys.createOrder(25);
        assertEquals(sys.getDb().getOrderList().size(), 1);
        sys.removeOrder("BRICK1");
        assertEquals(sys.getDb().getOrderList().size(), 0);
    }

    @Test
    public void testOrderDatabaseReturnsOrdersCorrectly()
    {
        System.out.println("testOrderDatabaseReturnsOrdersCorrectly");
        sys.createOrder(25);
        BrickOrder o = sys.getOrder("BRICK1");
        assertEquals(o.getNumberOfBricks(), 25);
    }

}

BrickOrder.java

package core;

/*
 * Implementation of BrickOrder class, implements Order interface.
 *  
 * BrickOrder class allows storage of reference number and number 
 * of brings per each order. One BrickOrder class is instantiated 
 * for each order. 
 */

public class BrickOrder implements Order{

    private String referenceNumber;
    private int numberOfBricks;

    public BrickOrder(String refNum, int brickNum)
    {
        setReferenceNumber(refNum);
        setNumberOfBricks(brickNum);
    }

    public String getReferenceNumber()
    {
        return referenceNumber;
    }

    public int getNumberOfBricks()
    {
        return numberOfBricks;
    }

    public void setReferenceNumber(String refNum)
    {
        referenceNumber = refNum;
    }

    public void setNumberOfBricks(int brickNum)
    {
        numberOfBricks = brickNum;
    }

}
\$\endgroup\$
4
  • \$\begingroup\$ I've embedded the code in the thread. \$\endgroup\$ Jul 7, 2018 at 19:03
  • \$\begingroup\$ Is there no requirement that you actually store the orders? Your "database" is very ephemeral, as in everything in it is lost when the program exits... \$\endgroup\$
    – Phrancis
    Jul 7, 2018 at 19:49
  • \$\begingroup\$ No there isn't, I've thought about including some kind of persistent storage. But the brief doesn't mention it anywhere, so I thought it would be better not to bother. \$\endgroup\$ Jul 7, 2018 at 19:52
  • \$\begingroup\$ "I have to implement a relatively basic order system in Java for part of the process of a job interview. [...] Before I submit the codebase to the company I am interviewing for I want my code to be reviewed so that I can improve any issues that exists within my program." - Will you share a part of your future income with the users that improved your code? After all they also passed that interview somehow... ;o) \$\endgroup\$ Jul 8, 2018 at 16:15

3 Answers 3

6
\$\begingroup\$

Encapsulation and protecting your data

OrderDB is not encapsulated well. The getOrderList method exposes the internal data structure of the class. This is especially bad because users of OrderSystem have access to it too, through the getOrders method. As it is, a user of OrderSystem can wipe out the database:

sys.getOrders().clear();

One simple fix is to remove the getOrderList from OrderDB.

If for some reason you really want to expose a list of all orders, you can do so safely by returning a defensive copy instead of the real list.

public ArrayList<BrickOrder> getOrderList()
{
    return new ArrayList<>(database);
}

Do you see the improvement? Thanks to this change, sys.getOrders().clear(); will not wipe out the internal database, because it will only affect the ad-hoc copy that was returned.

But this is not enough. Do you see why? It's not enough, because this defensive copy is a shallow copy. The objects inside it are the same objects as the originals in OrderDB. Which would be fine if BrickOrder was immutable, but it's not. That is, a user could do sys.getOrders().get(0).setNumberOfBricks(99), modifying the state of objects inside OrderDB.

So to safely expose the list of orders, you would need to return a defensive deep copy, with copies of the original objects.

Use interface types when possible

Instead of ArrayList in signatures, it would be better to use a List.

Performance

OrderSystem.updateOrder is inefficient: it gets a list of all orders, and then loops over the list to find the right one to modify it. This filtering task should be delegated to OrderDB. And indeed a method is available to retrieve an order by reference, and that's what should be used here.

It's interesting to note that you would not have made this implementation mistake if you had not exposed the internal structure of the database. The method returning a list of objects would simply not be there, so naturally you would have made it work with OrderDB.getOrder. That's why it's important to think through good encapsulation.

Testing

This essentially tests that value X is not Y and not Z:

BrickOrder o = sys.getOrder("BRICK1"); //Get details of first order
assertFalse(o.getReferenceNumber().equals("BRICK2"));
assertFalse(o.getReferenceNumber().equals("BRICK3")); //Check ref number is unique

It would be more direct and natural (and shorter) to test what it is, instead of testing what it is not:

BrickOrder o = sys.getOrder("BRICK1"); //Get details of first order
assertEquals("BRICK1", o.getReferenceNumber());

After all, if the value is equal to "BRICK1", it cannot possibly be also equal to "BRICK2", "BRICK3", and so on.

\$\endgroup\$
2
\$\begingroup\$
  1. Avoid premature specialization. What happens when you have to handle a thousand different products rather than just bricks? You'd need a class for each of them to keep consistent, which clearly doesn't scale. Instead I would use the generic Order since you really don't need anything other than a reference number and an item count.
  2. Private methods are a code smell (and to any hecklers, no that doesn't mean it's always bad). For example, incOrdersCreated reduces overall clarity by spreading the use of ordersCreated across several methods.
  3. What @janos said :)
\$\endgroup\$
2
\$\begingroup\$

General

Returning ArrayList

Returning "Interfaces" is preferred, either Collection or List.

OrderSystem

createOrder method

  • The JavaDoc explains implementation (e.g. stuff is made by OrderBuilder, this is implementation detail and the caller shouldn't care about that). And if you write JavaDoc, write it correctly: use @param numBricks and @return blah.
  • numBricks: Well, if you only want to buy bricks, and only of one type, that's okay. But I have the feeling, that there should be the possibility, to order more than one article, which will change a lot of your program. Also: I'd prefer numberOfBricks -> it's the method you expose to the 'outside', there must not be any abberviations. I tend to use less abbreviations than the other way around, because it will make the code more clear imo.
  • builder.constructOrder ...: Why the builder needs the amount of orders created is a mystery to me.
  • System.out.println: Maybe not too relevant, but try to use a Logging-API.

updateOrder

  • db.getOrderList -> maybe the backend should provide a method findOrder(referenceNumber: String).
  • The return value is suspicious. At first, you return a referenceNnumber - why? The caller of that method does already pass the same referenceNumber. And below, you return an 'error' message as String, and this is really bad. I think in that case it's okay to throw an Exception, because this method must not be called with a "wrong" referenceNumber.

getOrder

  • Aye, there you return a BrickOrder. You might want to reuse that when you place an Order?

Tests in general

  • Try to apply the 'given-when-then' pattern, meaning, to split your test case into those three blocks: Given means, to set up your data, when, is the actual thing you want to test, and then means, what you expect.
  • There's a lot of code which you can get rid of, and the test will run fine.

OrderSystemTest

In general, you use lots of those "BRICKS"-Strings. But: If you place an order, you get the actual referenceNumber. Just think it about this way: If you are going to change the logic on how the referenceNumber is generated, you change everything.

testCreateOrderReturnsReference

  • ...returnsReferenceNumber! And: the referenceNumber isn't created by the OrderSystem, but by the OrderBuilder.

testCreateOrderReturnsUniqueReference

  • Same: That shouldn't be tested here.
  • Also, referenceNumber! The word reference can be quite misleading, because a reference in programming terms, is something separate.
  • You test the same thing three times. If it works once, it should work three times. If it doesn't work once, it won't work three times.

testOrderDetailsReturnedCorrectly

  • Same here: You create three orders. But if you don't create the other two orders (BRICK1 and BRICK2), there test works the same, right?
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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