current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

I have a simple function that takes a string and reverses it:

def reverse(string): 
    begin = 0 
    end = len(string) - 1 
    strlist = [i for i in string] 
    while(begin < end): 
        temp = strlist[begin] 
        strlist[begin] = strlist[end] 
        strlist[end] = temp 
        begin += 1 
        end -= 1 
    return ''.join(strlist)

print reverse('flash')

I know it's only a simple function but I'm keen to see where my thought process could be improved (even in such a simple context) for future projects.

share|improve this question
    
For one thing, the function does not handle Unicode combining characters, such as diacritical marks, properly. Also, it does not handle leading BOM, trailing NUL bytes, or trailing newlines properly. What about character pairs such as directional quotes, brackets, and parentheses? Though superficially simple (and more so in a high-level language such as Python that understands multi-byte characters), string reversing is a nightmare in the real world! –  dotancohen 4 hours ago

2 Answers 2

up vote 6 down vote

Well, this is technically a way to invalidate your function, but either the built in slicing operator or the reversed function would do this for you in one line:

"flash"[::-1]
>>> "hsalf"

The slice operator takes three parameters, where it starts and ends, but also what step to take as it increments. You could pass in 2 to get every second letter, but you can also pass in a negative value to start at the end and work backwards, hence reversing your string.

The reversed function can take any iterator and return it in reverse as a generator object. This means that it wont strictly print properly, ie. this is what you would get when printing it:

reversed("flash")
>>> <reversed object at 0x0000000002F50898>

But calling join with it will give the result you need:

''.join(reversed("flash"))
>>> "hsalf"

But for your approach I'll still give suggestions. You use a slightly complicated approach. Swapping the values at opposite ends is not entirely obvious so I'd recommend adding comments to elaborate a bit on what the intent is as code is not always readable on its own.

Also you could use Python's multiple assignment to clean up some code. A more simple example is your begin and end could be set at the same time:

    begin, end = 0, len(string) - 1

This doesn't add a lot but can look a little neater. It essentially lets you assign two variables at once. It might be unnecessary, but the same principle can be used to eliminate the need for your temp.

    strlist[end], strlist[begin] = strlist[begin], strlist[end]

This allows you to safely assign both at the same time. The right hand side of the expression is fully evaluated before any assignment, so you wont lose either value.

share|improve this answer
1  
This is great and gives me a lot to think about, thank-you! –  Codingo 5 hours ago
up vote 3 down vote

Python encourages the use of generator expressions. Taken directly from Functional Programming, this language construct condenses a lot of meaning in just a line. It is also very easy to read.

def reverse(string):
    return ''.join(string[i] for i in range(len(string) - 1, -1, -1))

It reads almost as english, join all the i-th-s elements of the string where i goes from the len of the string to zero.

share|improve this answer

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.