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 9 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
1  
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 Aug 27 at 11:52
    
If you are going to use Python, write in Python. –  Jared Burrows Aug 28 at 2:50
    
@JaredBurrows That's not constructive, mind expanding upon this in an answer. –  Codingo Aug 28 at 8:06

5 Answers 5

up vote 18 down vote accepted

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 Aug 27 at 10:27
up vote 1 down vote

Similar to Naftuli Tzvi Kay, I immediately thought of the reversed builtin. reversed returns an iterator, so you need to force evaluation of it, which is why it needs to be fed into ''.join().

''.join(letter for letter in reversed('foobar'))
share|improve this answer
    
Very useful insight, thank-you –  Codingo Aug 28 at 8:08
up vote 3 down vote

Building on Caridorc's answer, there's a more Pythonic way to approach the problem which has an even better readability:

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

Here's an explanation from furthest depth outward:

  1. range(len(string)): Create an iterator object which iterates from 0 to len(string) - 1 (ie: the parameter is non-inclusive).
  2. reversed(range(len(string))): Reverse this iterator so you're starting at len(string) - 1 and going backward one-by-one to 0.
  3. [string[i] for i in ...]: This is a list comprehension, basically a way to create a list with a single line.
  4. ''.join(...): Join each element together into a string from the list, using an empty string as a separator.

Documentation can be found for each of these concepts here:

  1. PEP 281: loop counters with range and xrange.
  2. PEP 322: reversed iteration.
  3. PEP 202: list comprehensions.

The hope is that the above code will be fairly readable to someone nominally proficient in Python.

share|improve this answer
up vote 4 down vote

Other option for reverse string is use a deque object.

from collections import deque

def reverse_deque(string):
    d = deque(string)
    d.reverse()
    return ''.join(d)

In [245]: reverse_deque('reverse')
Out[245]: 'esrever'

comparison

  1. string slice ([::-1]):

    In [243]: %timeit "reverse"[::-1]
1000000 loops, best of 3: 310 ns per loop

  2. deque reverse:

    In [244]: %timeit reverse_deque("reverse")
100000 loops, best of 3: 2.61 µs per loop

  3. `reverse' function from Caridorc:

    In [241]: %timeit reverse_caridorc("reverse")
100000 loops, best of 3: 3.98 µs per loop

  4. function in question of Codingo

    In [250]: %timeit reverse_codingo("reverse")
100000 loops, best of 3: 3.61 µs per loop

Clearly the slice option is the winner.

share|improve this answer
up vote 6 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
    
Python's recommended way of dealing with iterating backwards is reversing the iterator, as it makes more sense: reversed(xrange(0, len(string)). (PEP 0322) That would definitely help readability. –  Naftuli Tzvi Kay Aug 27 at 19:24
    
@NaftuliTzviKay The other answer is already about built-ins, I suggest a way to reinvent the whell differently. –  Caridorc Aug 27 at 19:30

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.