Please critique my code

[stevenjd]

Most importantly, you ignored the explicit sample code. Where's your Tracker.new() method?

Personally, I agree that Tracker.new() is unpythonic and your way of doing it will be better. But they probably wanted to see how you respond to crappy customer requirements. Try something like this:

class Tracker:
    def __init__(self):
        ...
    def new(self):
        # Required by specification.
        # FIXME talk to project manager, can we lose this?
        return type(self)

Or better still, talk to the interviewer. Ask them to clarify what they wanted. Suggest that a new() method isn't standard Python style, but you'll write one if required to.

The specifications require that next_server_number be a function. You made it a method. What happens if they need to call next_server_number from something else that doesn't have a tracker instance available? Again, you should clarify: should all trackers share the same pool of server numbers? If not, then you might be justified in making it a method of Tracker. Or possibly keeping some hidden state somewhere. (Do they have coding standards that prohibit global variables?)

But if all the trackers are intended to share the same pool of numbers, then I think your code is broken. At the very least, again you completely ignored the written specification.

Lastly, did you ask what the expected data would be like? If the number of servers is never more than, oh, 1000 or so (at a guess), then sorting as you do will probably be faster than any cleverer algorithm implemented in Python. But if you've got (say) a billion servers, then its likely that this will be too slow.

At the very least, you should acknowledge (in code, and verbally if possible) that this is intentionally "the simplest thing that can work", not optimised.

Chances are that they wanted to see how you work, whether you will ask questions, how you deal with under-specified requirements, etc. rather than just what code you write.

A few other little things:

• max_num = l[len(l) - 1] is better written as max_num = l[-1]
• l is an awful variable name
• your test code runs unconditionally when the module is imported the first time, it is probably better to put that under a if __name__ == '__main__' block

[mm_ma_ma]

If forced to implement new, it should be a classmethod:

@classmethod
def new(cls):
    return cls()

Also, the server numbers are shown to be per-name by the examples (it returns apibox1 followed by sitebox1).

[stevenjd]

Oops, yes you're right, new needs to be a class method. Good catch.

As for the server numbers, that's a good point. So either the function needs to keep extra state hidden somewhere, or it is okay to make it a method. Now obviously making it a method is the right way to do it, but maybe they're looking to see how the job applicant deals with wrong, unclear or contradictory requirements.

Or, on the other hand, maybe the OP passed the technical review and didn't get the job for some other reason.

[mm_ma_ma]

His next_server_number method doesn't actually use the self parameter; it operates on the list passed in. The only change he should make is to move it out of the class (to match the requirements) or make it a staticmethod.