"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")
1 Answer 1
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]
).
-
\$\begingroup\$ Funny that you should call Python's ternary operator "nice". Most people curse its syntax. \$\endgroup\$200_success– 200_success2016年06月03日 17:32:52 +00:00Commented Jun 3, 2016 at 17:32
-
\$\begingroup\$ Could you expand on how
all
/any
work? Does youris_funny
method then return a Boolean automatically without specifying? \$\endgroup\$nyim– nyim2016年06月03日 21:14:26 +00:00Commented Jun 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\$301_Moved_Permanently– 301_Moved_Permanently2016年06月04日 12:31:54 +00:00Commented Jun 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– SylvainD2016年06月06日 09:03:01 +00:00Commented Jun 6, 2016 at 9:03
Explore related questions
See similar questions with these tags.