For practice purposes I implemented a function that checks for palindromes:
def palindrom(name):
name_reversed = ""
for buchstabe in name[::-1]:
name_reversed += buchstabe
return name.lower() == name_reversed.lower()
print(palindrom("Tom"))
print(palindrom("Bob"))
How may I improve this code, especially the for
-loop?
-
2\$\begingroup\$ Great first question! \$\endgroup\$Reinderien– Reinderien2020年07月21日 14:47:39 +00:00Commented Jul 21, 2020 at 14:47
-
\$\begingroup\$ btw what's a buchstabe? \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年07月21日 19:33:44 +00:00Commented Jul 21, 2020 at 19:33
-
\$\begingroup\$ @VisheshMangla It's German for "letter". \$\endgroup\$In Hoc Signo– In Hoc Signo2020年07月22日 00:10:47 +00:00Commented Jul 22, 2020 at 0:10
-
\$\begingroup\$ Thank's @TheDaleks \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年07月22日 05:17:55 +00:00Commented Jul 22, 2020 at 5:17
4 Answers 4
Localisation
It's fine to localise your user-facing strings (e.g. print('Die Buchstaben in diesem Wort bilden ein Palindrom')
, but it is not advisable to write the code itself in a language other than English. For better or worse, English is the de-facto language of programming. Thus, buchstabe
would be better as letter
.
For-loop
The loop is not necessary:
name_reversed = name[::-1]
name
is a string, which is itself a sequence of one-character strings. Applying a slice, as is done here, reverses the sequence but does not change the type: the output of the expression is still a string.
-
1\$\begingroup\$ Not only that "the loop is not necessary", creating an empty string and then concatenating to it in a loop is an anti-pattern in Python since it has a very poor performance. \$\endgroup\$DeepSpace– DeepSpace2020年07月22日 09:53:07 +00:00Commented Jul 22, 2020 at 9:53
For your for-loop, you can use reversed(name)
instead of name[::-1]
. This will increase performance by preventing unnecessary copying by creating a generator instead of another string. However in your case, because you are only checking if the reversed is the same as the original, your function can be even simpler
def palindrome(name):
return name.lower() == name[::-1].lower()
-
1\$\begingroup\$ This lowercases two strings - why not lowercase the string on input (i.e. just once) then do the reverse and compare? \$\endgroup\$user7761803– user77618032020年07月22日 07:36:49 +00:00Commented Jul 22, 2020 at 7:36
-
1\$\begingroup\$ Ideally when comparing strings you would use
.casefold()
instead of.lower()
since it handles cases such asß
->ss
. \$\endgroup\$QuantumChris– QuantumChris2020年07月22日 14:25:31 +00:00Commented Jul 22, 2020 at 14:25 -
\$\begingroup\$ @QuantumChris yes you are correct, I left it as
.lower()
because I wanted it to look simple (not necessarily perfect) and from what I've seen, no one seems to know about.casefold()
\$\endgroup\$vlovero– vlovero2020年07月22日 14:39:55 +00:00Commented Jul 22, 2020 at 14:39
It's also possible to work with two pointers. This is not the shortest solution, but it should be very fast and you don't have to store the reversed string.
def palindrome(name):
name = name.lower()
left = 0
right = len(name) - 1
while left < right:
if name[left] != name[right]:
return False
left += 1
right -= 1
return True
-
\$\begingroup\$ This is Python - there are no pointers. \$\endgroup\$Reinderien– Reinderien2020年07月22日 12:34:53 +00:00Commented Jul 22, 2020 at 12:34
-
\$\begingroup\$ Also, this will not be fast. I encourage you to compare it with
timeit
. \$\endgroup\$Reinderien– Reinderien2020年07月22日 12:36:24 +00:00Commented Jul 22, 2020 at 12:36
@Reinderien and @vlovero have already told whatever could be done but maybe you would also like the reversed
function. Also you do not need to lower()
your strings twice when you can do it only once. Strings(immutable type) are passed by value in python and doesn't modify the original string since a shallow copy of your string is sent to the function.
def palindrome(name):
name = name.lower()
return "".join(reversed(name)) == name #or simply name[::-1] == name
print(palindrome("Tom"))
print(palindrome("Bob"))
-
1\$\begingroup\$ This also points to another python perfomance issue, you rarely want to do repeated += with strings as strings are immutable so you end up making a copy of the intermediate string each time. Instead if you need to build a string up, add each element to a list then do "".join() at the end. \$\endgroup\$Rob– Rob2020年07月22日 08:16:55 +00:00Commented Jul 22, 2020 at 8:16
-
\$\begingroup\$ Yep, you are right. \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年07月22日 08:31:43 +00:00Commented Jul 22, 2020 at 8:31
-
1\$\begingroup\$ This beats every advantage that
reversed
brings to the table (ie the fact it returns an iterator rather than a string). Just use[::-1]
and get back a reversed string directly instead of needing to use''.join
. \$\endgroup\$DeepSpace– DeepSpace2020年07月22日 09:56:34 +00:00Commented Jul 22, 2020 at 9:56 -
\$\begingroup\$ I 'm not sure how [::-1] works. I can't say anything. When I learned C++ 3-4 years ago I was told strings take 1 byte(256 ascii) and they require contiguous memory and thus they are immutable since if you break the pattern of the string like swap 3rd and 4th character the addresses order ruins. Since python's too implemented in c++ I do believe this slicing wouldn't be as simple as you thing it to be. \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年07月22日 20:21:27 +00:00Commented Jul 22, 2020 at 20:21