3
\$\begingroup\$

"Funny String" problem from Hackerrank

Suppose you have a String, \$S\$ of length \$N\$ indexed from \$0\$ to \$N-1\$. You also have some String, \$R\$, that is the reverse of \$S\$, where \$S\$ is funny if the condition is \$\left|S[i] - S[i-1]\right| = \left|R[i] - R[i-1]\right|\$ is true for all \$i \in [1, N-1]\$.

I code mainly in Java. I'm very new to Python, so I would appreciate feedback on how I can make this code more Pythonic, cleaner and more efficient in general.

import math def isFunny(word): i = 0 length = len(word) arr = [] revarr = [] for i in range(0,math.ceil(length/2)): sdiff = abs(ord(word[i])-ord(word[i+1])) rdiff = abs(ord(word[length-i-1])-ord(word[length-i-2])) if sdiff == rdiff: continue else: return False return True if __name__ == "__main__": n = int(input()) for _ in range(n): word = input(); if isFunny(word): print("Funny") else: print("Not Funny") 
\$\endgroup\$
0

    1 Answer 1

    5
    \$\begingroup\$

    Code organisation and other things well done

    You've extracted the algorithm in a function on its own. Also, you've written you code behind the "if name == main" guard. Congratulations on this, these are two good things beginners usually don't do properly. This makes your code clearer, easier to maintain and easier to test.

    Also, I'd like to take this chance to congratulate you for using _ as a name for a throw-away value which is quite a nice habit.

    Finally, you did notice that there was not point in looping over the whole string once you've reached the middle.

    Getting rid of useless things

    It seems that perfection is attained not when there is nothing more to add, but when there is nothing more to remove. - Antoine de Saint Exupéry

    I can see in your code artifacts of old versions of your code : arr and revarr are not used at all. Also you don't need to define i before the loop.

    continue is nice keyword, available in most programming languages. However, from my experience, you don't need it that often. In your case, you might as will just write :

     if sdiff != rdiff: return False 

    Finally, 0 is not required as a first argument to range.

    Style

    Python has a style guide called PEP 8. It is definitly worth a read. You'll find various tools online to check your code's compliancy to PEP 8. From what I can see, the main "issue" is that the function name is CamelCased and not snake_cased.

    Documentation

    If you wanted to do things properly, it might be interesting to add some documentation to your function.

    More details (and personal preferences)

    We have a nice ternary operator in Python. Using it, you could write :

     print("Funny" if is_funny(word) else "Not Funny") 

    Also, your is_funny function looks like the typical situation where you could use the all/any Python builtin. However, because of the length of the expressions involved, it becomes a bit of a matter of personal preference. I'd write this (but you don't have to like it) :

    def is_funny(word): """Check if a word is funny.""" length = len(word) return all( abs(ord(word[i])-ord(word[i+1])) == abs(ord(word[length-i-1])-ord(word[length-i-2])) for i in range(math.ceil(length/2)) ) 

    Another thing that could be changed but might not make the code clearer to every one is the way you get the n-th element of the reversed string. You can use word[length-i-1] like you did but you can also take the most out of Python's index handling : negative indices are taken from the end of the container. So that you can write word[-i-1] instead (and the same principle applies to word[length-i-2]).

    \$\endgroup\$
    4
    • \$\begingroup\$Funny that you should call Python's ternary operator "nice". Most people curse its syntax.\$\endgroup\$CommentedJun 3, 2016 at 17:32
    • \$\begingroup\$Could you expand on how all/any work? Does your is_funny method then return a Boolean automatically without specifying?\$\endgroup\$
      – nyim
      CommentedJun 3, 2016 at 21:14
    • \$\begingroup\$@200_success I, too, find it rather nice. In any case it's way better than the .. and .. or .. trick we had before.\$\endgroup\$CommentedJun 4, 2016 at 12:31
    • \$\begingroup\$@Trinity : all (resp. any) takes an iterable (something you can iterate/loop over) and check if all elements (resp. any) is true-ish (can be considered as true in a boolean context, this is the case for True, non-0 integers, non empty lists, non empty strings, etc) stopping as soon as possible. It goes well with generator expression and list comprehension ( stackoverflow.com/questions/47789/… ). You can see the implementation of the function in the doc. It is a simple function but useful to make your code cleaner and more concise.\$\endgroup\$
      – SylvainD
      CommentedJun 6, 2016 at 9:03

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.