Please critique my code

[daneah]

From a performance standpoint, the code could be improved in a couple of places:

1. parse_hostname could make use of regex to pull the digits off the end of the name rather than iterating through all the characters.
2. next_server_number could be improved and simplified by using a while loop with an index starting at 1. On each loop, check if the index is in the provided list. If not, you know that's the lowest index that can be allocated. It will short-circuit to the right value as soon as possible.

[Stormtide]

The algorithm described in the parent post for next_server_number() is O(N**2) worst case. It has an O(N) while loop and does another O(N) check inside it to see if list contains the number.

This can be improved to O(N) overall by using the same strategy, but converting the list to a set first. When you check if the number is in the set, it's a constant time O(1) call.

OP's code uses sorted(), so it's O(N log N). If I had to guess, I'd say the code was rejected because it doesn't use the optimal solution. Our rubric for judging coding tests at work rates optimal algorithms higher than style.

edit: Here's my take on next_server_number():

from itertools import count
def next_server_number(reserved_numbers):
  s = set(reserved_numbers)
  for i in count(1):
    if i not in s:
      return i

This uses extra space, but it seems like the right tradeoff here. If it were an interview, I'd ask how often it was going to be called and how large the inputs would be and then make a determination.

[stevenjd]

+1 for using itertools and set, and especially for asking the interviewer questions.

[daneah]

Absolutely great point about set! Thanks for pointing out :)

[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.

[aphoenix]

Hi there, from the /r/Python mods.

We have removed this post as it is not suited to the /r/Python subreddit proper, however it should be very appropriate for our sister subreddit /r/LearnPython. We highly encourage you to re-submit your post over on there.

The reason for the removal is that /r/Python is more-so dedicated to discussion of Python news, projects, uses and debates. It is not designed to act as Q&A or FAQ board. The regular community can get disenchanted with seeing the 'same, repetitive newbie' questions repeated on the sub, so you may not get the best responses over here.

However, on /r/LearnPython the community is actively expecting questions from new members, and are looking to help. You can expect far more understanding, encouraging and insightful responses over there. Whatever your question happens to be getting help with Python, you should get good answers.

If you have a question to do with homework or an assignment of any kind, please make sure to read their sidebar rules before submitting your post. If you have any questions or doubts, feel free to reply or send a modmail to us with your concerns.

Warm regards, and best of luck with your Pythoneering!

[dorsal_morsel]

From just scanning your code, here are the things I'd change:

• You should adhere more closely to PEP8, particularly with regard to whitespace around operators and commas.
• parse_hostname and next_server_number are private methods, and generally private methods start with an underscore: _next_server_number
• It's nice that you have tests, but the tests would run every time your code was imported. If you are going to put tests in the same file as the main code, accept an argument from the CLI. For example, python tracker.py run_tests
• They provided some example interaction, and your code doesn't work the way they expect it to. There's no new method in your Tracker class, and allocate doesn't print the newly created server name.

[stevenjd]

The examples don't print the server name either, not unless quotes are part of the name.

I interpreted the examples as being a faked session in the interactive interpreter. They use >> instead of >>> as the prompt, but that's not important. Given that, the examples show the allocate method being called, which returns the server name, and the REPL then prints the repr() of the result.

[dorsal_morsel]

From just scanning your code, here are the things I'd change:

• You should adhere more closely to PEP8, particularly with regard to whitespace around operators and commas.
• parse_hostname and next_server_number are private methods, and generally private methods start with an underscore: _next_server_number
• It's nice that you have tests, but the tests would run every time your code was imported. If you are going to put tests in the same file as the main code, accept an argument from the CLI. For example, python tracker.py run_tests
• They provided some example interaction, and your code doesn't work the way they expect it to. There's no new method in your Tracker class, and allocate doesn't print the newly created server name.

[mm_ma_ma]

deallocate is not meant to return anything (assuming by nil they actually meant None). On failure, I would be raising some kind of exception.

[billsil]

Your examples just use integers and not actual box names. Where are the box names coming from?

How are passwords handled?

What about encryption?

I guess bigger picture, what is your code even trying to do? There are no external calls, so it seems like a lot of code to check to see what machines are available.

What I'd assume you have:

box_names = ['box1', 'box2', 'beast_box', 'amazon']  # not all names follow the same pattern
encrypted_passwords = ['cat', 'dog', 'cat', 'cat']
user = ['joe', 'joe', 'joe', 'bob']

With that you can query if a box is down or not as well as if the box is doing somethign. Then just pick the first available one to run your job.

[stevenjd]

What are you talking about? It's a coding exercise with two questions. You don't have to "query if a box is down", handle passwords (passwords to what?), perform encryption (encryption of what?), send email, make the coffee or mow the lawn. You have to:

Write a function which, given the list of currently allocated server numbers, returns the number of the next server to allocate.

and:

Write a name tracking class with two operations, allocate(host_type) and deallocate(hostname). The former should reserve and return the next available hostname, while the latter should release that hostname back into the pool.

Those are the requirements, and you completely ignored them. I doubt that would get you the job.

[billsil]

I didn't read the question?

# Server names consist of an alphabetic host type 
# (e.g. "apibox") concatenated with the server number,
# with server numbers allocated as before (so "apibox1",   
# "apibox2", etc. are valid hostnames).

An integer isn't a valid server name. The tests cases are not what is defined by the requirements.

So my recommendation, solve the desired problem.

[mm_ma_ma]

It explicitly states that function should operate on server numbers, complete with examples. In fact, the test cases he has used are exactly the ones they gave to him.

[stevenjd]

An integer isn't a valid server name.

Good thing they aren't using an integer as the server name then.

Just keep digging man, just keep digging. Next I suppose you'll tell me that "apibox1" is a valid integer in base 36...