3
\$\begingroup\$

Recently I have attended an Interview with a firm. They gave me a task to complete. I have done it and tested it as well. But they rejected my code. Is the given code properly written? Is my coding style good enough?

The Question is as below:

Problem Description The objective of this problem is to allocate the orders from a shopping cart to our warehouses while minimzing the cost of shipment.

Our warehouses contain different products in varying quantity, and we want to figure out a way to take a shopping cart from a user, and distribute the order among the warehouses. The distribution is dependent on the distance of the warehouses from the user, and the quantity of the ordered product each warehouse contains. Minimzing the total distance will help us minimize the cost of shipment.

In code, our warehouses are represented through an enum like so;

public enum Warehouse {
    EDMONTON, MONTREAL, TORONTO, VANCOUVER
}

The values in the enum represent the locations of our warehouses.

We want to optimize the distribution by using the warehouses closest to the user. A helper function is provided to assist you with this.

/**
This function returns a List of Warehouse sorted by distance from the 
provided Address.

Input: Address object that will be used to compare warehouses' distance
Output: A List of Warehouse sorted by distance from the inputted 
Address
**/

 public List<Warehouse> getNearestWarehouses(Address addressOfCustomer);

For the scope of this problem, you won't have to be concerned with innards of an Address.

Since we also need to know the quantity of a product in the warehouses, another helper function will be provided to assist you with this.

/**
Given a product ID this function will return a Map with a warehouse as 
key, and the quantity of the
given product in that warehouse as the value

Input: A String representing the product ID
Output:  Map with a warehouse (Warehouse) as key, and the quantity 
(Integer) of the
given product in that warehouse as the value
**/
public Map<Warehouse, Integer> getInventory(String product);

Using the given information, you will implement the following function.

/**
 This function will take a shopping cart (Map with product ID as 
 key, and desired quantity as Integer),
 and an Address and return the inventory allocation.

Input: Shopping cart (map between product ID and desired quantity) and 
Address
Output: 
`Map<Warehouse, Map<String, Integer>>`  will be the structure that you 
will output from your function.

The outer map's key will be the warehouse, and the value will be the 
inner map.

The inner map's key is a Prodcut ID and the value is the quantity that 
will be retrieved from that warehouse (outer map's key) for the product ID. 
**/

public Map<Warehouse, Map<String, Integer>>  
getInventoryAllocation(Map<String, Integer> shoppingCart, Address 
addressOfCustomer) {

}

You may assume every entry in the shopping cart has a quantity > 0 and that the shopping cart contains at least one entry. Output should not contain any warehouse or any product with 0 quantity.

And my solution was as follow:

public class InventoryAllocation {
public static void main(String[] args) {
    InventoryAllocation allocation = new InventoryAllocation();
    Map<String, Integer> pdtInChart = new ConcurrentHashMap<>();
    pdtInChart.put("car", 2);
    pdtInChart.put("jeep", 2);
    allocation.getInventoryAllocation(pdtInChart, new Address());
}

public Map<Warehouse, Map<String, Integer>> getInventoryAllocation(Map<String, Integer> shoppingCart, Address addressOfCustomer) {

    List<Warehouse> nearestWarehouses = getNearestWarehouses(addressOfCustomer);
    Map<Warehouse, Map<String, Integer>> allocatedWarehouseInventoryMap = new ConcurrentHashMap<>(); //used concurrentHashmap bcoz in future we can make the stream parallel in future

 /** Considered shoppingCart first even though we need map with key of 'warehouse',
     bcoz this is more efficient, as shopping cart size will be < warehouse list */
    shoppingCart.entrySet().stream().forEach(requiredInventoryEntry -> {
        String productId = requiredInventoryEntry.getKey();
        int requiredQuantity = requiredInventoryEntry.getValue();
        Map<Warehouse, Integer> availableInventory = getInventory(productId);

        for (int i = 0; i < nearestWarehouses.size(); i++) {
            if (!availableInventory.containsKey(nearestWarehouses.get(i)))
                continue;
            int availableQuantity = availableInventory.get(nearestWarehouses.get(i));
            if (availableQuantity <= 0)
                continue;

            if (requiredQuantity <= availableQuantity) {
                addNewProductToWarehouseForOrdering(nearestWarehouses.get(i), allocatedWarehouseInventoryMap, productId, requiredQuantity);
                break;
            } else {
                addNewProductToWarehouseForOrdering(nearestWarehouses.get(i), allocatedWarehouseInventoryMap, productId, availableQuantity);
                requiredQuantity -= availableQuantity;
            }
        }
    });
    return allocatedWarehouseInventoryMap;
}

private void addNewProductToWarehouseForOrdering(Warehouse warehouse, Map<Warehouse, Map<String, Integer>> warehouseMapMap, String productId, int orderQuantity) {
    if (warehouseMapMap.containsKey(warehouse)) {// if this warehouse has already existing order for other pdt
        Map<String, Integer> productsQuantityMap = warehouseMapMap.get(warehouse);
        productsQuantityMap.put(productId, orderQuantity);
    } else {
        Map<String, Integer> productsQuantityMap = new ConcurrentHashMap<>();
        productsQuantityMap.put(productId, orderQuantity);
        warehouseMapMap.put(warehouse, productsQuantityMap);
    }
}
}
\$\endgroup\$
0

3 Answers 3

4
\$\begingroup\$

The only things that leap out at me, in order of importance:

  1. You should use the iterator version of the for statement to move through the warehouse list, instead of the index: for (Warehouse warehouse : nearestWarehouses) ...

  2. Your comment /** Considered shoppingCart ... should use the regular comment style, not the Javadoc double-asterisk version: /* Considered shoppingCart ...

  3. Maybe they want braces around all function blocks for if statements, even if they're single line?

\$\endgroup\$
2
  • \$\begingroup\$ Why to use Iterator version over for loop? Is there any benefit? \$\endgroup\$
    – srth12
    Jun 14, 2018 at 18:49
  • 2
    \$\begingroup\$ The main benefit is it abstracts away the iteration so you have fewer things to keep track of. You know that each iteration you'll get the next element in the sequence; you don't care how. \$\endgroup\$
    – user34073
    Jun 14, 2018 at 19:41
3
\$\begingroup\$

What leaps out at me:

  • Apart from the modify-the-result function, you do not use any methods at all. For me as an inteviewer this shouts: "this person does not know how to divide a problem into manageable parts!"
  • Double checking key and quantity: Map has the method getOrDefault(). So, if you want to know the available quantity for a product in a warehouse inventory, use availableInventory.getOrDefault(0).

What I would have liked to see:

  • A method doing the calculation of how-many-items-do-I-ship-from-which-warehouse for a single product and distance-ordered warehouse list.
  • A method which iterates over the shopping cart to call and accumulate the first method.
  • Another method to wrap this all up by getting the ordered warehouse list by the customer's address and then call the second method I mentioned.

... and what I would have liked to hear from you is the following question: "if the nearest warehouse cannot satisfy the customers order completely, which is cheaper: splitting the order, oder using a more distant warehouse which can ship all at once?"

One afterthought: "bcoz"?? I'm not a native speaker, but this sounds like kiddy-street-slang. Write "because" or "b/c".

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

I think they rejected your code, because you solved this problem using imperative coding style (basically using loops). It's even worse that you use continue and break, which significantly increase complexity of your code.

Functional style is more concise and readable. You could use java Stream API to solve it more elegant way.

Suggested solution (not complete)

import java.util.Map;
import java.util.List;
import java.util.stream.Collectors;

class Warehouse {}
class Address {}

public class Test {

    private static Map<Warehouse, Map<String, Integer>> getInventoryAllocation(Map<String, Integer> shoppingCart, Address addressOfCustomer) {
        Map<String, Warehouse> productsInWarehouses = shoppingCart.keySet().stream()
                .collect(
                        Collectors.toMap(p -> p, p -> bestFitWarehouse(getNearestWarehouses(addressOfCustomer), p, shoppingCart.get(p)))
                );

        // Then you need to convert Product -> Warehouse map (productsInWarehouses) to result map, but the problem is that you can have
        // different products in the same warehouse, so keys of the map will be not unique, that's why you will lost some data.
    }

    private static Warehouse bestFitWarehouse(List<Warehouse> warehouses, String product, Integer cartQuantity) {
        return warehouses.stream().filter(w -> isWarehouseFit(w, product, cartQuantity)).findFirst().get(); // Check for presence needed
    }

    private static Boolean isWarehouseFit(Warehouse warehouse, String product, Integer cartQuantity) {
        return getQuantityInWarehouse(warehouse, product) >= cartQuantity;
    }

    private static Integer getQuantityInWarehouse(Warehouse warehouse, String product) {
        return getInventory(product).get(warehouse);
    }

    public static native Map<Warehouse, Integer> getInventory(String product);
    public static native List<Warehouse> getNearestWarehouses(Address addressOfCustomer);
}
\$\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.