8
\$\begingroup\$

There is a list consisting of zeroes and ones. I want to find out the length of the longest streak of ones. Is there a better solution?

def consecutive_one(data):
   one_list = []
   size = 0
   for num in data:
       if num == 1:
           one_list.append(num)
       elif num == 0 and size < len(one_list):
           size = len(one_list)
           one_list = []

    return size

if __name__ == '__main__':
    data = [0, 1, 0, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0]
    print(consecutive_one(data))
\$\endgroup\$
0

3 Answers 3

Reset to default
10
\$\begingroup\$

Both your and janos' implementations are broken:

data = [1, 0] * 10000
consecutive_one(data)
#>>> 140

This is because you don't always reset after seeing a 0. Going from janos', you should have

longest = 0
current = 0
for num in data:
    if num == 1:
        current += 1
    else:
        longest = max(longest, current)
        current = 0

return max(longest, current)

and equivalent for the original.

You'll find that this functionality is largely provided by itertools.groupby, though:

from itertools import groupby

def len_iter(items):
    return sum(1 for _ in items)

def consecutive_one(data):
    return max(len_iter(run) for val, run in groupby(data) if val)
\$\endgroup\$
0
8
\$\begingroup\$

You have a bug

If the last value is a 1, and it is the end of the longest consecutive sequence, it won't be taken into account. The fix is to change the return statement to this:

return max(size, len(one_list))

Unnecessary condition

If you know your input only contains 0 and 1 values, then you can simplify this condition:

if num == 1:
    # ...
elif num == 0 and size < len(one_list):
    # ...

By dropping the num == 0:

if num == 1:
    # ...
elif size < len(one_list):
    # ...

But note that this is not good enough, as there's still a bug hiding there as @veedrac explains in his answer, instead of an elif, this should be rewritten using an else.

Improving storage efficiency

There's no need to store the 1s as you count them. You can just keep the count in a variable.

Testing

Instead of running your function using test data, give a try to doctests, like this:

def consecutive_one(data):
    """
    >>> consecutive_one([0, 1, 0, 1, 1, 0])
    2
    >>> consecutive_one([0, 1, 0, 1, 1, 1])
    3
    >>> consecutive_one([0, 1] * 10)
    1
    """
    # ... the implementation ...

To run all doctests within a file, run python -m doctest yourfile.py. When all tests pass, there is no output. When something fails you will get a detailed report. This is an excellent way to test your implementation, and also to document usage with examples and expected outputs.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Just to be more precise, he doesn't have to know that the list contains only 0 and 1. The purpose is to count every 1, so it's not really relevant if it's 0 or another character, as long as it's not 1. \$\endgroup\$ Commented Aug 12, 2016 at 20:48
1
\$\begingroup\$

You have a bug on elif statement size < len(one_list): 

if __name__ == '__main__':
    n = int(input())       

    binary = [int(x) for x in bin(n)[2:]]  

    one_list = []
    size = 0

    for num in binary:
        if num == 1:
            one_list.append(num)

            if size < len(one_list):
                size = len(one_list)

        elif num == 0 :
            one_list.clear()

    print(size)
\$\endgroup\$
1
  • 1
    \$\begingroup\$ (Welcome to Code Review!) (While valid, this has been stated before.) \$\endgroup\$ Commented Dec 14, 2018 at 21:27

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.