2
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Just one thing – change comments from #... above functions to docstrings inside functions. \$\endgroup\$ – kyrill Apr 8 '17 at 14:33
3
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ get_next_substring would be a misleading name. The function does not return the next substring – when you call it, you get a generator. And that generator returns the next substring when you call its __next__ method. So I think substrings is a more apt name. \$\endgroup\$ – kyrill Apr 8 '17 at 18:28
  • \$\begingroup\$ @kyrill What you say is correct of course, but seeing how the function is used, it really looks like it's returning a list and that's not the case, that's why I think substrings is not a good enough name. As you say, also get_next_substring is not perfect, but I couldn't come up with anything better. \$\endgroup\$ – ChatterOne Apr 8 '17 at 20:04
  • \$\begingroup\$ I don't see why returning a generator instead of a list means that substrings is a misleading name. You would still do for substring in substrings(...), and other similar things. A generator is still a sequence in many respects. \$\endgroup\$ – zondo Apr 8 '17 at 21:02
2
\$\begingroup\$

I saw your substrings function and expected to tell you to use yield instead of creating a list, but you already do. I also like your comment on why you use range. If you want to be completely compatible with Python 3, though, you'll need to use parentheses for print:

print(Question2("stresseddesserts"))

The only other thing I'll mention is that Question2 does not follow official Python naming guidelines as defined in PEP 8. According to that document, function names should use snake_case. Your assignment also uses lowercase, so I'm a little surprised you changed it especially since your other functions are lowercase.

All in all, the program is concise, efficient, and organized. Well done.

\$\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.