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.

I am trying to build an efficient function for splitting a list of any size by any given number of indices. This method works and it took me a few hours to get it right (I hate how easy it is to get things wrong when using indexes).

Am I over-thinking this? Can you help me minimize the code? I am just not seeing it. ALSO, is there any way to simplify the input of so many variables such as in this situation?

def lindexsplit(List,*lindex):

    index = list(lindex)

    index.sort()

    templist1 = []
    templist2 = []
    templist3 = []

    breakcounter = 0
    itemcounter = 0
    finalcounter = 0


    numberofbreaks = len(index)
    totalitems = len(List)

    lastindexval = index[(len(index)-1)]
    finalcounttrigger = (totalitems-(lastindexval+1))

    for item in List:

        itemcounter += 1

        indexofitem = itemcounter - 1

        nextbreakindex = index[breakcounter]

        #Less than the last cut
        if breakcounter <= numberofbreaks:

            if indexofitem < nextbreakindex:

                templist1.append(item)

            elif breakcounter < (numberofbreaks - 1):

                templist1.append(item)

                templist2.append(templist1)

                templist1 = []

                breakcounter +=1

            else:

                if indexofitem <= lastindexval and indexofitem <= totalitems:

                    templist1.append(item)

                    templist2.append(templist1)

                    templist1 = []

                else:

                    if indexofitem >= lastindexval and indexofitem < totalitems + 1:

                        finalcounter += 1

                        templist3.append(item)

                        if finalcounter == finalcounttrigger:

                            templist2.append(templist3)


    return templist2

Example:

mylist = [1,2,3,4,5,6,7,8,9,0,10,11,12,13,14,15]
lindexsplit(mylist,3,5,8)
return [[1, 2, 3, 4], [5, 6], [7, 8, 9], [0, 10, 11, 12, 13, 14, 15]]
share|improve this question
    
Welcome to codereview.stackexchange. In order to make reviewing easier for everyone, can you tell us a bit more about what your function is supposed to do and how it does it ? Also, examples are more than welcome :-) –  Josay Apr 22 at 11:26
    
The function takes a list of any set of objects and slices that list at any number of sorted indexes given as arguments in the second part of the function :) - I noticed that there was no split method for lists, so I made my own –  user3559226 Apr 22 at 11:42
1  
Please edit your answer to include this explanation. Also, if you could add an example, that would make things much easier. –  Josay Apr 22 at 11:46
    
Thanks for your example, this is exactly what I was hoping for. Unfortunately, the output I get is [[1, 2, 3, 4], [5, 6], [7, 8, 9], [0, 10, 11, 12, 13, 14, 15]] and not the one you've provided. –  Josay Apr 22 at 11:58
1  
I don't want to be rude, but this should be a one-liner: return [seq[i:j] for i, j in zip((0,) + indices, indices + (None,))], as shown in the answers to this Stack Overflow question. I'm also not sure why your indices are 1-based rather than 0-based: if I pass 3 as an index to your function, I would expect a split after the third element, not after the fourth. –  codesparkle Apr 22 at 12:37

2 Answers 2

There is a lot simpler way to do this. You can use list slicing and the zip function.

List slicing essentially cuts up a given list into sections. The general form is list[start:stop:step]. The start section of a slice designates the first index of the list we want to included in our slice. The stop section designates the first index of the list we want excluded in our slice. The step section defines how many indices we are moving as well as in which direction (based on whether it is positive or negative). An example:

>>> x = [1, 2, 3, 4]
>>> x[1:3]
[2, 3]
>>> x[2:]
[3, 4]

The zip function takes sequences and creates a zip object that contains tuples of their corresponding index elements:

>>> for pair in zip([1, 2, 3], ['a', 'b', 'c']):
...    print(pair)
(1, 'a')
(2, 'b')
(3, 'c')

You can combine these two strategies to simplify your function. Here is my version of your lindexsplit function:

def lindexsplit(some_list, *args):
    # Checks to see if any extra arguments were passed. If so,
    # prepend the 0th index and append the final index of the 
    # passed list. This saves from having to check for the beginning
    # and end of args in the for-loop. Also, increment each value in 
    # args to get the desired behavior.
    if args:
        args = (0,) + tuple(data+1 for data in args) + (len(some_list)+1,)

    # For a little more brevity, here is the list comprehension of the following
    # statements:
    #    return [some_list[start:end] for start, end in zip(args, args[1:])]
    my_list = []
    for start, end in zip(args, args[1:]):
        my_list.append(some_list[start:end])
    return my_list

share|improve this answer

A few cosmetic changes to makes your code more beautiful/pythonic :

Fix formatting

  • Remove some line breaks as it makes the code harder to read
  • Change variables name to follow PEP 8
  • Your code lacks documentation making it hard to understand.

Use enumerate

Enumerate does exactly what you are trying to achieve : keep track of the index while looping on an iterable. Just use for indexofitem,item in enumerate(List):.

Remove levels of nested logic

Using elif, you could make your code a bit easier to follow. The inside of the for-loop becomes :

    if breakcounter <= numberofbreaks:
        if indexofitem < nextbreakindex:
            templist1.append(item)
        elif breakcounter < (numberofbreaks - 1):
            templist1.append(item)
            templist2.append(templist1)
            templist1 = []
            breakcounter +=1
        elif indexofitem <= lastindexval and indexofitem <= totalitems:
            templist1.append(item)
            templist2.append(templist1)
            templist1 = []
        elif indexofitem >= lastindexval and indexofitem < totalitems + 1:
            finalcounter += 1
            templist3.append(item)
            if finalcounter == finalcounttrigger:
                templist2.append(templist3)

Rewrite your comparisons

In Python, you can write comparisons in a very natural way : indexofitem >= lastindexval and indexofitem < totalitems + 1 becomes lastindexval <= indexofitem < totalitems + 1.

Use smart indices to get the last element of array

You can rewrite lastindexval = index[(len(index)-1)] with the much clearer lastindexval = index[-1].

Rethink your logic

You have totalitems = len(List) and indexofitem going from 0 to len(List) - 1 (included). Thus, indexofitem <= totalitems is not an interesting condition to check. The same goes for indexofitem < totalitems + 1.

Once this is removed, we have :

    #Less than the last cut
    if breakcounter <= numberofbreaks:
        if indexofitem < nextbreakindex:
            templist1.append(item)
        elif breakcounter < (numberofbreaks - 1):
            templist1.append(item)
            templist2.append(templist1)
            templist1 = []
            breakcounter +=1
        elif indexofitem <= lastindexval:
            templist1.append(item)
            templist2.append(templist1)
            templist1 = []
        elif lastindexval <= indexofitem:
            finalcounter += 1
            templist3.append(item)
            if finalcounter == finalcounttrigger:
                templist2.append(templist3)

Re-think your logic (bis)

On the code above, the last 2 elif checks are a bit redundant : if we don't go into the indexofitem <= lastindexval block then we must have lastindexval < indexofitem and ``lastindexval <= indexofitem` must be true.

After cleaning this, the code looks like :

    for indexofitem,item in enumerate(List):
        nextbreakindex = index[breakcounter]

        #Less than the last cut
        if breakcounter <= numberofbreaks:
            if indexofitem < nextbreakindex:
                templist1.append(item)
            elif breakcounter < (numberofbreaks - 1):
                templist1.append(item)
                templist2.append(templist1)
                templist1 = []
                breakcounter +=1
            elif indexofitem <= lastindexval:
                templist1.append(item)
                templist2.append(templist1)
                templist1 = []
            else:
                finalcounter += 1
                templist3.append(item)
                if finalcounter == finalcounttrigger:
                    templist2.append(templist3)
    return templist2

Re-think your logic (ter)

Nothing happens in the loop if breakcounter > numberofbreaks as breakcounter and numberofbreaks are not changed. If this is really the case, we might as well just break out of the loop. However, things are even better than this : once again we are in a situation that cannot happen. This can be seen in two different ways :

  • if breakcounter was to be bigger than numberofbreaks, nextbreakindex = index[breakcounter] would have thrown an exception.

  • breakcounter only gets incremented one element at a time. This happens only if breakcounter < (numberofbreaks - 1). Thus, once breakcounter reaches numberofbreaks - 1, it stops growing.

At the end of this rewriting, your code looks like :

def lindexsplit(List,*lindex):
    index = list(lindex)
    index.sort()

    templist1 = []
    templist2 = []
    templist3 = []

    breakcounter = 0
    finalcounter = 0

    numberofbreaks = len(index)

    lastindexval = index[-1]
    finalcounttrigger = (len(List)-(lastindexval+1))

    for indexofitem,item in enumerate(List):
        nextbreakindex = index[breakcounter]

        if indexofitem < nextbreakindex:
            print "A"
            templist1.append(item)
        elif breakcounter < (numberofbreaks - 1):
            print "B"
            templist1.append(item)
            templist2.append(templist1)
            templist1 = []
            breakcounter +=1
        elif indexofitem <= lastindexval:
            print "C"
            templist1.append(item)
            templist2.append(templist1)
            templist1 = []
        else:
            print "D"
            finalcounter += 1
            templist3.append(item)
            if finalcounter == finalcounttrigger:
                templist2.append(templist3)
    return templist2

I reckon there is still a lot more to improve and a much more simple solution could be written (as suggested in the comments).

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.