HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Finding the longest palindromic substring

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thesubstringfindinglongestpalindromic

Problem

I have the following code that works correctly. I am wondering if there is a simpler way to go about implementing my solution and/or if there is anything that looks non-standard in my code. If anyone wants to offer criticism I'm all ears.

### Question 2 main function and helper functions.
"""Given a string a, find the longest palindromic substring contained in a.
Your function definition should look like question2(a), and return a string."""
# Gives substrings of s in decending order.
def substrings(s):

    # Declare local variable for the length of s.
    l = len(s)

    # Here I chose range over xrange for python version compatibility.
    for end in range(l, 0, -1):
        for i in range(l-end+1):
            yield s[i: i+end]

# Define palindrome.
def palindrome(s):
    return s == s[::-1]

# Main function.
def Question2(a):
    for l in substrings(a):
        if palindrome(l):
            return l

# Simple test case.
print Question2("stresseddesserts")
# stresseddesserts

Solution

In terms of efficiency, I like the code.

I'd just fix a few stylistic issues:

-
Naming: l, a, s don't say anything about what the variable means. Also, you named your function palindrome, but that's returning a boolean, so I'd name it is_palindrome. The i in the substring function is a bit puzzling to me: you have a variable named end, why not call the other one start?

-
As @kyrill mentioned, you should probably change comments above the functions to docstrings.

-
The comment Declare local variable for the length of s. is useless.

-
I'd instead add another comment explaining why it works. If you have a function simply called substrings I expect it to return the substrings. This code only works because it returns the substrings starting from the longest to the shortest. I'd at least change the comment descending order (which may mean for example lexicographical order or alphabetical order) to from longest to shortest. Better be explicit.

-
I know, it's a bit long, but since you're using a generator, I'd rename the function substrings to get_next_substring.

-
As @zondo mentioned Question2 is not a PEP8 compliant name, but I'm not sure if the automatic grader will need it to be called like that.

Context

StackExchange Code Review Q#160142, answer score: 4

Revisions (0)

No revisions yet.