11
\$\begingroup\$

I am learning python and want to improve my skills in it. So could someone advice of how to improve the code below (this is a simple programming problem taken from one of the web sites). I feel like my code is not really Pythonic.

Problem: Using the Python language, have the function LetterChanges(str) take the str parameter being passed and modify it using the following algorithm. Replace every letter in the string with the letter following it in the alphabet (ie. c becomes d, z becomes a). Then capitalize every vowel in this new string (a, e, i, o, u) and finally return this modified string.

from sets import Set

def LetterChanges(str): 
   vowels = Set(['a', 'e', 'i', 'o', 'u'])
   new_str = ''
   for i in str:
      cur_char = i.lower()
      if cur_char == 'z':
         new_str += 'A'
      elif ord(cur_char) >= 97 and ord(cur_char) < 122:
         new_char = chr(ord(i) + 1)
         new_str += (new_char if new_char not in vowels else new_char.upper())
      else:
         new_str += i
   return new_str

if __name__=='__main__': 
   print(LetterChanges(raw_input()))
\$\endgroup\$
  • \$\begingroup\$ Why two Python versions? \$\endgroup\$ – Cristian Ciupitu Mar 12 '18 at 19:21
  • \$\begingroup\$ sorry, my bad. I will edit it now. \$\endgroup\$ – David_Zizu Mar 12 '18 at 19:22
  • \$\begingroup\$ Welcome to codereview.SE! Your code does not seem to run on any version of Python 3 because the sets module is removed (it is deprecated since Python 2.6 docs.python.org/2/library/sets.html ) \$\endgroup\$ – SylvainD Mar 12 '18 at 19:31
  • \$\begingroup\$ @Josay I'd say he wanted python-2.x tag. \$\endgroup\$ – Grajdeanu Alex. Mar 12 '18 at 19:32
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Mar 12 '18 at 19:46
18
\$\begingroup\$

Your code defines a function with a clear input and a clear output. Also, you've put the call to it behind the if __name__ == '__main__':. You've taken good habits on this points. Nonetheless, a few things can still be improved.

Python version

If you are learning Python, there is no good reason to start with Python 2 which is reaching its end of life. You should focus on learning Python 3, going to be supported for a while and with various warts corrected (and obviously more features).

In any case, the sets module is not required since Python 2.6 since we have the set builtin.

Style

There is an official standard Python style guide called PEP 8. This is highly recommended reading. It gives guidelines to help writing code that is both readable and consistent. The Python community tries to follow these guidelines, more or less strictly (a key aspect of PEP 8 is that it provides guidelines and not strict rules to follow blindly).

It deals with various aspects of the code style: naming conventions, indentation convention, etc.

You'll find various tools to try to check whether your code is PEP 8 compliant and if is it not, to try and fix this:

In your case, the major points I would fix are:

  • indentation should be 4 space

  • function names should snake_cased

  • you could try to take into account the advice to use join instead of "CPython's efficient implementation of in-place string concatenation".

Naming

On top of the snake-case point mentioned above, a few things could be improved in the naming. Naming is a difficult part of programming so what I suggest are just personal, far from perfect, suggestions (from a non-native English speaker). In your case, I'd call the function tranform_string so that it conveys the fact that:

  • it is an action (a noun would correspond to an object/class name)

  • it applies to string (and not to single character even if the distinction is fuzzy in Python compared to other programming languages)

The name str is a bit of a problem because it hides the str builtin. I do not find any alternative better than str_ which is still pretty poor :-(.

Edit: I don't know why I didn't think about "s" in the first place but after reading it in another answer, it looks like a much better option indeed. I am too lazy to edit my code everywhere.

Finally, i is a good name for an (integer) index. For a single character, c seems easier to understand (char if you feel verbose).

Chained comparison

In Python, you can chain comparison. If your case, you can write:

elif 97 <= ord(cur_char) < 122:

More beautiful character check

From the Zen of Python:

Beautiful is better than ugly. Explicit is better than implicit. Simple is better than complex.

In your case, you could avoid the call the ord and get rid of the magic number by writing the simple:

elif 'a' <= cur_char < 'z':

Rewriting the logic

You could have the logic adding a character to new_str in a single place.

    cur_char = c.lower()
    if cur_char == 'z':
        new_char = 'A'
    elif 'a' <= cur_char < 'z':
        new_char = chr(ord(c) + 1)
        if new_char in vowels:
            new_char = new_char.upper()
    else:
        new_char = c
    new_str += new_char

Then, you could try to makes the logic as close as possible to the way it is given in the problem for instance by making "z" a special case only for the operation where you take the following letter.

    low_char = c.lower()
    if 'a' <= low_char <= 'z':
        new_char = 'a' if low_char == 'z' else chr(ord(c) + 1)
        if new_char in vowels:
            new_char = new_char.upper()
    else:
        new_char = c
    new_str += new_char

Shorter way to define vowels

Because strings are iterable and set takes any kind of iterable, you could write:

vowels = set('aeiou')

Better ascii check

You could reuse values from the string module to make your test more beautiful:

   if low_char in string.ascii_lowercase:

Removing string concatenation

As mentionned earlier, it is advised not to use string concatenation and use join instead, when relevant.

At this stage, the code (with added tests) looks like:

import string

def transform_string(s):
    vowels = set('aeiou')
    lst = []
    for c in s:
        if c in string.ascii_letters:
            new_c = 'a' if c.lower() == 'z' else chr(ord(c) + 1)
            if new_c in vowels:
                new_c = new_c.upper()
        else:
            new_c = c
        lst.append(new_c)
    return "".join(lst)

if __name__=='__main__':
    assert transform_string("")            == ""
    assert transform_string("qwertyuiop")  == "rxfsUzvjpq"
    assert transform_string("asdfghjkl")   == "btEghIklm"
    assert transform_string("zxcvbnm")     == "AydwcOn"
    assert transform_string("1234567890")  == "1234567890"
    assert transform_string("QWERTYUIOP")  == "RXFSUZVJPQ"
    assert transform_string("ASDFGHJKL")   == "BTEGHIKLM"
    assert transform_string("ZXCVBNM")     == "AYDWCON"
    assert transform_string("(){}<>+_-=")  == "(){}<>+_-="
\$\endgroup\$
  • \$\begingroup\$ I was under the impression that uppercase letter shouldn't be transformed. \$\endgroup\$ – Cristian Ciupitu Mar 12 '18 at 20:26
  • 1
    \$\begingroup\$ set(['a', 'e', 'i', 'o', 'u']) could be replaced with {'a', 'e', 'i', 'o', 'u'} \$\endgroup\$ – Cristian Ciupitu Mar 12 '18 at 20:32
  • 1
    \$\begingroup\$ Why not use string instead of str? \$\endgroup\$ – Solomon Ucko Mar 12 '18 at 20:54
  • 1
    \$\begingroup\$ @SolomonUcko because there's a string module. \$\endgroup\$ – Cristian Ciupitu Mar 12 '18 at 20:56
  • 1
    \$\begingroup\$ @EricDuminil I've updated my answer. \$\endgroup\$ – SylvainD Mar 13 '18 at 13:40
13
\$\begingroup\$

Instead of having those ifs you could use a translation table to speed things up (see str.maketrans and str.translate).

import string

def _build_translation_table():
    ascii_vowels = 'aeiou'
    d = {c: chr(ord(c)+1) for c in string.ascii_lowercase}
    d['z'] = 'a'             # z is a special case
    for k, v in d.items():   # and so are vowels
        if v in ascii_vowels:
            d[k] = v.upper()
    return str.maketrans(d)

_TABLE = _build_translation_table()

def LetterChanges(s):
    """Change letters in string s using a special algorithm.

    Replace every letter in the string with the letter following it in the
    alphabet (ie. c becomes d, z becomes a) ...

    >>> LetterChanges('qwertyuiop')
    'rxfsUzvjpq'
    >>> LetterChanges('1234567890')
    '1234567890'
    >>> LetterChanges('zxcvbnm')
    'AydwcOn'
    """

    return s.translate(_TABLE)

if __name__ == '__main__':
    import doctest
    doctest.testmod()

I took the liberty to rename the parameter str to s for reasons already mentioned by others. _build_translation_table and _TABLE start with an underscore because it doesn't make much sense for them to be "public".

I also took some tests from Josay's answer and put them into the documentation string of the function, so that the doctest module can run them.

# python3 letterchanges.py -v
Trying:
    LetterChanges('qwertyuiop')
Expecting:
    'rxfsUzvjpq'
ok
Trying:
    LetterChanges('1234567890')
Expecting:
    '1234567890'
ok
\$\endgroup\$
  • \$\begingroup\$ Excellent answer with a much different approach. Nice to be able to see the bigger picture instead of focusing on details! \$\endgroup\$ – SylvainD Mar 12 '18 at 20:38
7
\$\begingroup\$

Believing the Python-3.x tag, here's a way to rewrite your method:

def letter_changes(text):
    vowels = set('aeiou')
    new_str = ''
    for c in text:
        cur_char = c.lower()
        if cur_char == 'z':
            new_str += 'A'
        elif ord(cur_char) >= 97 and ord(cur_char) < 122:
            new_char = chr(ord(c) + 1)
            new_str += new_char.upper() if new_char in vowels else new_char
        else:
            new_str += i 
    return new_str

if __name__ == '__main__':
    print(letter_changes(input()))

With those modifications:

  • snake_case method name
  • set for Python3
  • 4 spaces indent
  • reversed logic for upper if the letter is a vowel.

Another alternative would be to work with generators and join the letters at the end instead of building a new string character by character.

\$\endgroup\$
5
\$\begingroup\$

Here are some improvements that I'd make (supposing that your code is using Python 2.x):

  • I'd indent the code so that it'll be a multiple of four
  • There's no reason to make the vowels a Set(). A string should work just fine as strings are iterable in python
  • rename the str argument to something else as that's a builtin keyword in python
  • I'd simplify the chained comparison: ord(cur_char) >= 97 and ord(cur_char) < 122 to 97 <= ord(cur_char) < 122

That being said you'd have so far:

def LetterChanges(str_):
    vowels = 'aeiou'
    new_str = ''
    for i in str_:
        cur_char = i.lower()
        if cur_char == 'z':
            new_str += 'A'
        elif 97 <= ord(cur_char) < 122:
            new_char = chr(ord(i) + 1)
            new_str += (new_char if new_char not in vowels else new_char.upper())
        else:
            new_str += i
    return new_str


if __name__ == '__main__':
    print(LetterChanges(raw_input()))
\$\endgroup\$
0
\$\begingroup\$

There are some excellent pointers and implementations already, here are a few more.

  1. If you can implement your algorithm in an idiomatic fashion without extraneous imports, then do so. creating a set of the vowels is great however, the same effect can be achieved with data structures that are lighter weight. Strings and tuples and tuples are lighter and have the added benefits of being immutable. For this application, a string of vowels will suffice.

  2. As Cristian Ciupitu pointed out, a having a direct transformation is great for this problem as it avoids unnecessary computations. Rather than using the string library, the same can be achieved using a plain 'ol dict

  3. Rather than building up a string using the + operator, we can do better (speed, memory utilisation and portability across python implementations) with the "".join method.

  4. The function letter_changes seems to do a lot, it'd be better to approach it in a modular fashion i.e. build simpler functions to abstract away the different tasks

The setup: building the shift map

letters = tuple(map(chr, range(97, 97+26)))  # tuple of lowercase letters
vowels = "aeiou"
letters_right_shifted = letters[1:] + (letters[0],) # tuple (z, a, b, ..., y)
dest = tuple(c.upper() if c in vowels else c for c in letters_right_shifted)
shift_map = dict(zip(letters, dest))

The workhorse: change a single letter

def change_letter(some_character):
    """(chr) -> chr
    Return a single character shifted (and capitalised) appropriately
    according to the problem statement

    All characters are first converted to lower case.

    >>> change_letter('a')
    'b'
    >>> change_letter('d')
    'E'
    """
    return shift_map[char.lower()]

The Full function: handling strings

def letter_changes(some_string):
    """(str)->str
    Return a new string with every letter shifted (and capitalised) according to the problem statement.

    All characters are first converted to lower case.
    >>> letter_changes('done')
    'EpOf'
    """
    return "".join(map(change_letter, some_string))

The higher order function map applies the function change_letter to every element in some_string. The same effect can be achieved with the comprehension "".join((change_letter(c) for c in some_string)).

Now to extend the function to handle strings with non letter elements e.g. (1, 2, ?, ., /) one only has to modify the change_letter function.

\$\endgroup\$
  • 1
    \$\begingroup\$ The point of a set is that it's fast to check c in vowels. To be fair, there won't be much difference between O(1) and O(n) with n = 5. \$\endgroup\$ – Eric Duminil Mar 14 '18 at 7:35
0
\$\begingroup\$

I notice that there are a lot of answers that compare each letter to its ASCII value. This isn't bad, per se, but we can use the string module (which contains a full list of ASCII letters) to our advantage:

import string
ordered_letters = string.ascii_lowercase * 2 + string.ascii_uppercase * 2

Do you see what I did there? By putting the ordered strings in twice, I automatically handle the wrapping from Z/z to A/a.

Here's my full code:

import string
ordered_letters = string.ascii_lowercase * 2 + string.ascii_uppercase * 2


def change_letters(s):
    result = []  # will be returned as a joined-string

    # Loop through each character, transforming them as necessary,
    # and add them to the result list:
    for c in s:
        # Do the rotation, if necessary:
        i = ordered_letters.find(c)
        if i >= 0:
            c = ordered_letters[i+1]
        # Convert vowels to uppercase:
        if c in 'aeiou':
            c = c.upper()
        # Store the character in the result to be returned:
        result.append(c)

    # Return the result as a join()ed string:
    result ''.join(result)

Using the ordered_letters approach may seem strange, but it provides a convenient way to "rotate" letters by an arbitrary amount. For example, if you change:

c = ordered_letters[i+1]

to:

c = ordered_letters[i+13]

you will have modified the code to be a ROT13 encoder/decoder! And if you want to go backwards one step, you can add 25 instead of 1. (Or you could subtract 1, provided that you change str.find to str.rfind.)

\$\endgroup\$
  • \$\begingroup\$ You don't need both to be doubled; instead, just having string.ascii_lowercase + 'a' would work, and then do something like for c in s.lower() (because the OP lowercases every character first). \$\endgroup\$ – Dannnno Mar 14 '18 at 16:59
  • \$\begingroup\$ @Dannnno, you're correct in that I don't need both to be doubled in this case -- but I did that to accommodate arbitrary rotations (such as by 2, 13, and 25). That way, you don't have to worry about adding 'a', or 'abc', or 'abcdef...'; it's the same no matter how much you rotate (provided that 0 <= amount <= 26, of course!). \$\endgroup\$ – J-L Mar 14 '18 at 17:35
  • \$\begingroup\$ Even then, you at most only need the lower case due to the casing requirements. Furthermore, it isn't at all obvious that the OP wants or needs arbitrary rotations. As-is it takes a little bit to realize why you're doing what you're doing; at least factoring it out into a function, something like rotate_letter(char, n) would be cleaner. \$\endgroup\$ – Dannnno Mar 14 '18 at 17:38
  • \$\begingroup\$ @Dannnno, I'm pretty sure you're correct about the casing requirements; the reason I kept uppercase as uppercase is because of Josay's example, and the fact that the original poster didn't specify in the problem that vowels should always be lowercase (I know the original poster's code contradicts this, but I decided to follow the requirements in the problem statement instead). \$\endgroup\$ – J-L Mar 14 '18 at 20:51

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.