4
\$\begingroup\$

As a part of a challenge, I was asked to write a function to determine if there was repetition in a string (if no repetition, output is 1), and if a substring is repeated throughout the string (for example if the string is "abcabc," "abc" is repeated twice 2, if the string is "bcabcabc," the repetition is 0 since we start from the first string (no leftovers)).

My code works and I have submitted it to the challenge, but I am curious if there are ways to improve this code:

def answer(s):
     length = (len(s))
     range_list = (range(1, len(s)+1))
     list_char = list(s)
     divisors = []
     divisors = [x for x in range_list if (length)% (x) == 0]
     max_pieces = []
     for i in range (len(divisors)):
         size = divisors[i]
         split = [list_char[i:i+size] for i in range(0, length, size)]
         if(all(x == split[0] for x in split)):
             max_pieces = int(length/size)
             break
     return max_pieces
\$\endgroup\$
2
\$\begingroup\$
 length = (len(s))

Unnecessary ( )’s.

 range_list = (range(1, len(s)+1))

Again, unnecessary ( )’s. Does not actually create a list, just a range object, so a better name is in order. And you can remove this entire statement (see below).

 list_char = list(s)

You don’t need to create a list of characters to solve this problem. (More later.)

 divisors = []

Unnecessary statement. Made obsolete by very next statement:

 divisors = [x for x in range_list if (length)% (x) == 0]

Unnecessary ( )’s ... twice. Could replace range_list with range(1, len(s)+1), and remove above range_list = .....

 max_pieces = []

Misleading and useless. You store a single integer in this variable later, not a list.

 for i in range (len(divisors)):
     size = divisors[i]

A more pythonic way would be:

 for size in divisors:

I might use divisor instead of size, when looping over a list of divisors.

     split = [list_char[i:i+size] for i in range(0, length, size)]

Confusing. i is already used in outer loop. List comprehension’s i is (fortunately) a separate variable ... but give it a different name ... maybe j.

Instead of list_char[i:i+size], you could simply use s[i:i+size]. This would return a substring instead of a sublist of characters, which would be just as effective.

     if(all(x == split[0] for x in split)):

The if statement does not need the outer ( )’s.

         max_pieces = int(length/size)

More pythonic would be max_pieces = length // size

         break
 return max_pieces

break could be replaced with just return max_pieces. And the current return statement replaced with an error message or return 1 catch-all.

\$\endgroup\$
1
\$\begingroup\$

Inconsistent return values

The function returns an integer for non-empty input, and [] for empty input. It would be better to use a consistent return type.

Performance issues

To find the divisors, no need to iterate until the length of the input, it would be enough to look until the square root of the length.

The program uses more memory than necessary: instead of creating the split list, it would be more efficient to compare segment by segment.

In x == split[0] for x in split the first segment is unnecessarily compared to itself.

Alternative implementation

Consider this alternative implementation that handles the above comments:

from math import sqrt


def answer(s):
    """
    >>> answer('')
    0

    >>> answer('bcabcabc')
    1

    >>> answer('ababab')
    3

    >>> answer('abcabcd')
    1

    >>> answer('abcabc')
    2

    >>> answer('abc')
    1

    >>> answer('aaa')
    3

    """    
    length = len(s)
    divisors = (x for x in range(1, int(sqrt(length)) + 2) if length % x == 0)
    for size in divisors:
        first = s[:size]
        if all(s[start:start+size] == first for start in range(size, length, size)):
            return length // size
    return 1
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.