Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is the requirement I have (from the book: Cracking the Coding Interview)

Imagine you have a call center with three levels of employees: fresher, technical lead (TL), and product manager (PM). There can be multiple employees, but only one TL or PM. An incoming telephone call must be allocated to a fresher who is free. If a fresher can't handle the call, he or she must escalate the call to technical lead. If the TL is not free or not able to handle it, then the call should be escalated to PM. Design the classes and data structures for this problem. Implement a method getCallHandler().

And this is my implementation:

public interface CallAllocator {
    public Employee getCallHandler() throws NoEmployeeInTheHouseException;
    void setTL(TechnicalLead technicalLead);
    void setPM(ProductManager productManager);
    void addFresher(Fresher fresher);
}

Implementation for the interface:

public class CallAllocatorImpl implements CallAllocator {

    private TechnicalLead technicalLead;
    private ProductManager productManager;
    private List<Fresher> freshers = new ArrayList<Fresher>();

    @Override
    public Employee getCallHandler() throws NoEmployeeInTheHouseException {

        if (freshers.isEmpty() && technicalLead == null && productManager == null) {
            throw new NoEmployeeInTheHouseException();
        }

        if (!freshers.isEmpty()) {
            Employee fresher = freshers.get(new Random().nextInt(freshers.size()));
            if (fresher.getCanHandle()) {
                return fresher;
            }
        }

        if (technicalLead != null && technicalLead.getCanHandle()) {
            return technicalLead;
        }

        if (productManager != null && productManager.getCanHandle()) {
            return productManager;
        }

        throw new NoEmployeeInTheHouseException();

    }
    @Override
    public void setTL(TechnicalLead technicalLead) {
        this.technicalLead = technicalLead;
    }
    @Override
    public void setPM(ProductManager productManager) {
        this.productManager = productManager;
    }
    @Override
    public void addFresher(Fresher fresher) {
        if (fresher.isFree()) {
            freshers.add(fresher);
        }
    }
}

Employee class:

public class Employee {
    private boolean free;
    private boolean canHandle;
    public boolean isFree() {
        return free;
    }
    public void setFree(boolean free) {
        this.free = free;
    }
    public boolean getCanHandle() {
        return canHandle;
    }
    public void setCanHandle(boolean canHandle) {
        this.canHandle = canHandle;
    }
}

I have 3 classes with names: Fresher, TechnicalLead and ProductManager. They all extend Employee but do not override any methods or anything..

And this is my TestClass:

public class TestClass {

    public static void main(String[] args) throws NoEmployeeInTheHouseException {

        CallAllocator callAllocator = new CallAllocatorImpl();

        Fresher fresherOne = new Fresher();
        fresherOne.setCanHandle(false);
        fresherOne.setFree(true);

        Fresher fresherTwo = new Fresher();
        fresherTwo.setCanHandle(true);
        fresherTwo.setFree(true);

        Fresher fresherThree = new Fresher();
        fresherThree.setCanHandle(false);
        fresherThree.setFree(true);

        Fresher fresherFour = new Fresher();
        fresherFour.setCanHandle(false);
        fresherFour.setFree(false);

        callAllocator.addFresher(fresherOne);
        callAllocator.addFresher(fresherTwo);
        callAllocator.addFresher(fresherThree);
        callAllocator.addFresher(fresherFour);

        TechnicalLead technicalLead = new TechnicalLead();
        technicalLead.setCanHandle(false);
        technicalLead.setFree(true);

        callAllocator.setTL(technicalLead);

        ProductManager productManager = new ProductManager();
        productManager.setCanHandle(true);
        productManager.setFree(true);

        callAllocator.setPM(productManager);

        Employee callHandler = callAllocator.getCallHandler();
        System.out.println(callHandler.getClass().getSimpleName());
    }
}

So how can I improve this code? Any suggestions?

share|improve this question

4 Answers 4

up vote 17 down vote accepted

Firstly, I'm not sure you're actually implementing the requirement as it's written. The description says:

An incoming call must be allocated to a fresher who is free. If a fresher can't handle the call, he or she must escalate to technical lead.

This sounds like if there are no free freshers, the call shouldn't be handled at all (an exception thrown?), rather than skipping to the TL. This sort of makes sense as a real world requirement: it may be preferable for a caller to have to call back a bit later if there are no freshers available, rather than to waste the technical lead's time with a call that a fresher should be able to handle. Whatever calls this could, for example, plan to catch that exception and do addCallToUnhandledCallQueue() or whatever.

This also goes a bit more to the meat of the question, which I believe is trying to get you to answer with the chain-of-responsibility pattern. In this pattern each object (in this case an Employee) responsible for processing a command (in this case a call) contains logic to check whether it is capable of processing a given command, and if not, also knows the next object in the chain to call.

One benefit from this pattern is adherence to the open/close principle. As you'll see below, doing something like adding a new employee type or changing the structure a bit is unlikely to require you to fiddle around with if{...} else{...} logic in getCallHandler(). Additionally, it means that Employees only need to know about their immediate boss, rather than some master class having to know and persist the entire employee structure (which would fast become unpleasant, especially if you need to add other methods which also require knowing this structure).

A meta-benefit, given that this is apparently to be approached as an interview question, is that if somebody asked me this in an interview, I'd be pretty sure they'd want me to talk about this pattern, so even if for whatever reason you ultimately decide there's a better solution, it's important to understand this one if only to be able to describe intelligently why you reject it.

So using this pattern, your getCallHandler(Call call) method would look something like this:

Employee fresher = getAnyFreeFresher(); //Should throw if there are none
return fresher.handle(call);

Then the Employee class would look something like:

public class Employee{
    private Employee boss;

    private bool canHandle(Call call){
    //...
    }

    public Employee handle(Call call){
        if(canHandle(call)){
            return this;
        }
        if(boss == null){
            //Nobody in the chain could handle, throw
        }
        return boss.handle(call);
    }
}

That's a very rough outline, there's detail to fill in on how bosses are set, and you'd probably want employee types to inherit from Employee to implement canHandle, etc.

share|improve this answer
    
Thanks for the answer, I see your point. Maybe you are right, maybe I totally got the requirement wrong. :) –  Koray Tugay Feb 28 at 19:32
    
Absolute agreement on the main point: you are not solving a problem, you are in an interview. Your priority is not to understand the problem; it is to understand the problem under the interviewer eyes. –  vals Feb 28 at 22:00

Overall, code is easy to read.

A few minor nit-picky items.

Try not us use booleans, use enums instead.

i.e.

public enum EmployeeStatus {
    OnCall,
    Available        
}

This allows you to add more status as requirements change:

public enum EmployeeStatus {
    OnCall,
    Available,
    OutToLunch,
    OnVacation,
    OnBreak
}

I don't like that getCallHandler() can populate the freshers. I would inject the list or maybe a factory that will be able to do that. This will decouple the code, and allow for much easier unit testing in the future.

I also don't like the set methods. Using inheritance should eliminate the need for that. Admittedly I don't see the solution right now, but there is one there. Basically by having different methods to handle different instances, you are tying yourself to those three types. What happens if you add a forth type, say ProductExpert? You now have to change this class to deal with it.

The canHandle method seems like it should be a calculation based off the status enum. This way you only have to set one flag in the class, not two.

public class Employee {

    // code

    public boolean isFree() {
        return status == EmployeeStatus.Available;
    }

    // code
}

Fresher fresherTwo = new Fresher();
fresherTwo.setStatus(EmployeeStatus.Available);

I also don't like the name `NoEmployeeInTheHouseException', I find it a little too causal. I would do something line 'NoEmployeeAvailableException' or something. It comes across as a little more businesslike.

This is a good start, and I'm glad you are concerned about this stuff. Keep working at it.

share|improve this answer
    
Thanks for the great answer! –  Koray Tugay Feb 28 at 19:18
    
What do you mean I don't like that getCallHandler() can populate the freshers. ? This method does not populate freshers? –  Koray Tugay Feb 28 at 19:22
1  
I think its bad practice when I class can populate itself. The analogy that is most used is that you don't expect a car to know how to build itself, you build it up with pieces using other machines... –  Jeff Vanzella Feb 28 at 19:31

First I will comment on your implementation, then I will propose my version at the end.

Here we go, from top to bottom:

public interface CallAllocator {
    public Employee getCallHandler() throws NoEmployeeInTheHouseException;
    void setTL(TechnicalLead technicalLead);
    void setPM(ProductManager productManager);
    void addFresher(Fresher fresher);
}
  • "Employee" is too generic a term for somebody who handles calls. I would call it ICallHandler. The actual employee classes are irrelevant in the discussion of handling calls, they should not be part of the model design.

  • The setTL, setPM, addFresher methods all use terms and arguments that are too implementation specific. An interface definition should be as abstract as possible.

Next:

if (freshers.isEmpty() && technicalLead == null && productManager == null) {
    throw new NoEmployeeInTheHouseException();
}

I see that in your implementation you handle the availability of freshers/lead/manager as being empty or null. This is state management, not modeling. It would be better to capture the notion of being available or not by the model, using explicit interface methods.

if (!freshers.isEmpty()) {
    Employee fresher = freshers.get(new Random().nextInt(freshers.size()));
    if (fresher.getCanHandle()) {
        return fresher;
    }
}
  • getCanHandle is really awkward, canHandle would be more natural
  • The notion of getting the next available fresher deserves its own method: you can think of different implementations, such as pick any free fresher at random, or pick the least picked, or pick the most picked, and so on.

Ok so here's my solution to model the description quite accurately:

interface ITicket {}

interface ICallHandler {
    boolean isAvailable();
    boolean canHandle(ITicket ticket);
}

interface ICallHandlerPicker {
    ICallHandler getAvailableCallHandler();
}

interface ICallCenter {
    ICallHandler getCallHandler(ITicket ticket);
}

class SingleLeadSingleManagerCallCenter implements ICallCenter {
    private final ICallHandlerPicker picker;
    private final ICallHandler lead;
    private final ICallHandler manager;

    SingleLeadSingleManagerCallCenter(ICallHandlerPicker picker, ICallHandler lead, ICallHandler manager) {
        this.picker = picker;
        this.lead = lead;
        this.manager = manager;
    }

    @Override
    public ICallHandler getCallHandler(ITicket ticket) {
        ICallHandler handler = picker.getAvailableCallHandler();
        if (handler == null) {
            // nobody available. perhaps throw new NoSuchElementException() ?
            return null;
        }
        if (handler.canHandle(ticket)) {
            return handler;
        }
        if (lead.isAvailable() && lead.canHandle(ticket)) {
            return lead;
        }
        return manager;
    }
}

This sticks to the well-defined parts of the description and leaves the undefined parts unimplemented on purpose, such as:

  • If there are many free freshers, which one to pick? --> implement as you like
  • If there are no free freshers, what to do? --> null implies that there's nobody to handle, though I admin I don't like this part much

Other things to note:

  • The interfaces are short and to the point, with only getters, no mutators
  • The class members are all final, and the class is fully defined at construction time, there's no room left for guessing
  • In terms of handling calls, the tech lead and product manager are call handlers just like the freshers, so using interfaces makes good sense, no need to give them dedicated classes

Of course this is not scalable. The description itself excluded scalability by specifying a single tech lead and a single product manager. I would fix that by modeling the multiple levels as a chain of call centers:

class MultiLevelCallCenter implements ICallCenter {
    private final ICallHandlerPicker picker;
    private final ICallCenter nextCallCenter;

    MultiLevelCallCenter(ICallHandlerPicker picker, ICallCenter nextCallCenter) {
        this.picker = picker;
        this.nextCallCenter = nextCallCenter;
    }

    @Override
    public ICallHandler getCallHandler(ITicket ticket) {
        ICallHandler handler = picker.getAvailableCallHandler();
        if (handler == null) {
            // nobody available. perhaps throw new NoSuchElementException() ?
            return null;
        }
        if (handler.canHandle(ticket)) {
            return handler;
        }
        return nextCallCenter.getCallHandler(ticket);
    }
}

class UltimateCallCenter implements ICallCenter {
    private final ICallHandler handler;

    UltimateCallCenter(ICallHandler handler) {
        this.handler = handler;
    }

    @Override
    public ICallHandler getCallHandler(ITicket ticket) {
        return handler.isAvailable() ? handler : null;
    }
}

Then, we could implement the call center in the description in terms of these more scalable classes as:

ICallCenter getSingleLeadSingleManagerCallCenter(ICallHandlerPicker picker, final ICallHandler lead, ICallHandler manager) {
    ICallCenter managerCallCenter = new UltimateCallCenter(manager);
    ICallCenter leadCallCenter = new MultiLevelCallCenter(new ICallHandlerPicker() {
        @Override
        public ICallHandler getAvailableCallHandler() {
            return lead.isAvailable() ? lead : null;
        }
    }, managerCallCenter);
    return new MultiLevelCallCenter(picker, leadCallCenter);
}

Again, I left out the details of the freshers and how we pick them. These are not specified in the description, and not really relevant. This leaves you free to inject whatever implementation you like.

share|improve this answer
1  
Clean Code by Robert C. Martin, Interfaces and Implementations, p24 has a good argument agains the I interface prefixes. "I don’t want my users knowing that I’m handing them an interface. I just want them to know that it’s a ShapeFactory." –  palacsint Mar 1 at 12:00
    
Hm, I guess that's a debatable subject. I dropped that point from my post, thanks. –  janos Mar 1 at 12:04

Naming Issue

  • CallAllocatorImpl have a group of freshers, TL & a PM. So I think naming it Office or CallCenter seems feasible.
  • getCanHandle is misleading. You won't usually see methods name have get and can both. Secondly what to handle? Call. So canHandleCall seems reasonable.
  • Same goes for setCanHandle.

Answer the Question properly

There can be multiple employees, but only one TL or PM

Smelling a singleton-ish work.

So setTL needs few more LOC

public void setTL(TechnicalLead technicalLead) {
    if(technicalLead == null) {
        this.technicalLead = technicalLead;
    }
    else {
        throw new TLAlreadyExistsException("Only one Team-Leader allowed").
    }
}

Same goes for setPM.

share|improve this answer
3  
Well, two points of disagreement. First I wouldn't use the word "singleton" with to that, even after appending an "ish". The singleton pattern is a specific thing, which that isn't. Second, that exception makes no sense there. If you set something that already exists, you're replacing it, not trying to add another one. What you're suggesting is actually immutability, which is pretty much unrelated to only having one. Just having a single TechnicalLead rather than a collection is sufficient. –  Ben Aaronson Feb 28 at 23:07
    
CallAllocatorImpl is the implementation of CallAllocator, so if you changed the name of the implementation you need to change the name of the interface too. –  Marc-Andre Mar 1 at 1:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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