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.

Quite a specific question can the below code be made more clearer / pythonic in any way.

Essentially based on the condition of the string x((len(x) / 3) - 1), I use this to extract out parts of the string.

def split_string(x):
    splitted_string = [x[counter*3:(counter*3)+3] for counter in range ((len(x) /3) - 1)]
    return splitted_string

>>> split_string('ABCDEFGHI')
['ABC', 'DEF']
share|improve this question
add comment

3 Answers 3

For integer division, use the // operator. In Python 3, / will do floating-point division.

The task could be more succinctly accomplished using re.findall().

Also, the goal of the function is not obvious. Pythonic code would include a docstring explaining its purpose.

import re

def split_string(s):
    """
    Splits a string into a list, with each element consisting of
    three characters.  Any incomplete group at the end is discarded,
    and then the last complete group is also discarded.
    """
    return re.findall(r'.{3}', s)[:-1]
share|improve this answer
add comment

I always try to evaluate my function and see if there's anything else that could enhance it.

You're already evenly splitting these strings, so why not add a default parameter, size to control the section size. You also don't take the last cut (I'm guessing to avoid index error), you can add that as a default parameter as well.

Also, just return the comprehension

def split_string(word, size = 3, trim = 1):
    return [word[ii*size:(ii+1)*size] for ii in range(len(word)/size - trim )]

>>> split_string('ABCDEFGHI')
['ABC', 'DEF']
>>> split_string('ABCDEFGHI',4)
['ABCD', 'EFGH']
>>> split_string('ABCDEFGHI',3,0)
['ABC', 'DEF','GHI']
>>> split_string('ABCDEFGHI',3,2)
['ABC']
share|improve this answer
3  
Menaningful parameter names would be even better :) –  Morwenn Apr 10 at 21:56
1  
@Morwenn I agree,.. I was just lazy :p –  Calpratt Apr 10 at 21:57
add comment

The range command has a step argument.

def split_string(x):
    return [x[i:i+3] for i in range(0, len(x) - 5, 3)]

>>> split_string('ABCDEFGHI')
['ABC', 'DEF']

I also used i instead of counter since it's shorter and i is the standard way of expressing an array index in most langauges.

share|improve this answer
1  
That's a nice idea, but the behaviour is quite different from the original code. –  200_success Apr 10 at 21:43
    
It was just missing the -1 on the end of the range. –  David Harkness Apr 11 at 2:29
    
@DavidHarkness It's still not equivalent to the original. –  200_success Apr 11 at 7:39
    
@200_success I corrected my correction. –  David Harkness Apr 11 at 8:09
    
@DavidHarkness There was still an off-by-one error, which I've fixed in Rev 4. –  200_success Apr 11 at 9:23
add comment

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.