Drone Delivery Scheduling Service

Question

I recently did a interview with WalmartLabs. I was tasked with a take home assignment. I didn't sign a NDA and a team at WalmartLabs said I could post the code on GitHub. I was tasked with developing a program that would deliver orders in such a way to maximize customer satisfaction. My approach to solve this problem was to use a priority queue. The priority queue would prioritize by order created date and distance from the target. Admittedly, I made a mistake by not considering the total passing time when prioritizing orders. I want to become a better developer. I would like to know if someone could look at my github project at https://github.com/vaughnshaun/Walmart_DroneChallenge and tell me any flaws and/or good points of my design. Below is a snippet of a main class for my program. The full project is at my github. The spec for the project is in a pdf named DroneDeliveryChallengeSpec.pdf.

public class OrderDeliverer
{
    private List<DeliveredOrder> completedOrders = new List<DeliveredOrder>();
    private IOrderStreamer orderStreamer;
    private Warehouse warehouse;
    private Double promoters;
    private Double detractors;
    private Action<DeliveredOrder> deliveredOrderAction;

    public OrderDeliverer(Warehouse warehouse, IOrderStreamer orderStreamer)
    {
        this.warehouse = warehouse;
        this.orderStreamer = orderStreamer;
    }

    public virtual void ProcessOrder()
    {
        // Artificially advance the time to the next order waiting to be created
        // This is a fail safe, just in case the the processing of orders don't advance time enough
        if (!warehouse.HasOrders)
        {
            orderStreamer.AdvanceTime();
        }

        // Keep processing orders while there are orders
        if (warehouse.HasOrders)
        {
            Order order;

            // If there isn't time for delivery the order should be moved to next day delivery
            if (!warehouse.HasTimeToDeliverNextOrder(orderStreamer.CurrentTime))
            {
                warehouse.MoveNextOrderToNextDay();
            }
            else if (warehouse.TrySendNextOrder(out order)) // Try to send the order out of the warehouse
            {
                // Create a delivered order and track its status and times
                DeliveredOrder outboundOrder = new DeliveredOrder(order.Id);
                outboundOrder.OrderPlaced = order.Created;
                outboundOrder.DepartureTime = orderStreamer.CurrentTime;
                outboundOrder.DeliveredTime = outboundOrder.DepartureTime;

                // Time traveled to the destination
                double travelMinutes = warehouse.GetOrderDeliveryMinutes(order);
                outboundOrder.DeliveredTime = outboundOrder.DeliveredTime.AddMinutes(travelMinutes);

                // Total time traveled, includes to destination and returning back to the warehouse 
                travelMinutes += warehouse.GetOrderReturnMinutes(order);

                completedOrders.Add(outboundOrder);
                deliveredOrderAction(outboundOrder);

                switch (outboundOrder.GetRating())
                {
                    case OrderHelper.RatingType.Detractor:
                        detractors++;
                        break;

                    case OrderHelper.RatingType.Promoter:
                        promoters++;
                        break;
                }

                warehouse.DockDrone();

                // Update the mock global time (will also bring more new orders depending on the time)
                orderStreamer.AddMinutesToTime(travelMinutes);
            }
        }
    }

    public void OnDeliveredOrder(Action<DeliveredOrder> deliveredAction)
    {
        deliveredOrderAction += deliveredAction;
    }

    /// <summary>
    /// The number of orders successfully delivered
    /// </summary>
    /// <returns>Returns an int for the count of delivered orders</returns>
    public int GetNumberOfCompleted()
    {
        return completedOrders.Count;
    }

    public double GetNps()
    {
        double nps = 0;

        if (completedOrders.Count > 0)
        {
            double promoterPercent = (promoters / completedOrders.Count) * 100;
            double detractorPercent = (detractors / completedOrders.Count) * 100;
            int decimalPlaces = 2;

            nps = Math.Round(promoterPercent - detractorPercent, decimalPlaces);
        }

        return nps;
    }
}

Answer 1

There are some basic considerations to make in your design.

Guard arguments

Perform at least NotNull checks on arguments on public entrypoints of your API.

public OrderDeliverer(Warehouse warehouse, IOrderStreamer orderStreamer)
    {
        this.warehouse = warehouse;
        this.orderStreamer = orderStreamer;
    }

public OrderDeliverer(Warehouse warehouse, IOrderStreamer orderStreamer)
    {
        if (warehouse == null)
            throw new ArgumentNullException(nameof(warehouse));
        if (orderStreamer== null)
            throw new ArgumentNullException(nameof(orderStreamer));
        this.warehouse = warehouse;
        this.orderStreamer = orderStreamer;
    }

Avoid nesting statements if you can

Code reads easier with the amount of nested statements kept to a minimum.

if (!warehouse.HasOrders)
{
    orderStreamer.AdvanceTime();
}

// Keep processing orders while there are orders
if (warehouse.HasOrders)
{
    // code when HasOrders ..
}

if (!warehouse.HasOrders)
{
    orderStreamer.AdvanceTime();
    return;
}

// Keep processing orders while there are orders
// code when HasOrders ..

Avoid redundant comments

Comments should be added only if they add substantial new information to the code. In the above snippet, you could do without

// Keep processing orders while there are orders

Inline variable declaration

This can be written in a more concise fashion.

 Order order;
 else if (warehouse.TrySendNextOrder(out order))

else if (warehouse.TrySendNextOrder(out Order order))

Redundant variable type

The declared type does not need to be printed out when it can be derived logically from the instantiated type.

DeliveredOrder outboundOrder = new DeliveredOrder(order.Id);

  var outboundOrder = new DeliveredOrder(order.Id);

Event lifetime management

Do you have a way to unsubscribe from the event? This is the cause of many memory leaks.

travelMinutes += warehouse.GetOrderReturnMinutes(order);

Property vs Method

How to decide? Consider using a property for a simple getter GetNumberOfCompleted.

Virtual methods

Declare virtual methods when there is a use case for it. What is the reason you declare your method virtual?

public virtual void ProcessOrder(