Transform letters in a string

Question

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()))

Answer 1

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:

• pycodestyle package (formerly known as pep8) to check you code

• pep8online to check your code with an online tool

• autopep8 package to fix your code automatically

• Also, this is also checked by various linters: pylint, pyflakes, flake8, etc.

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("(){}<>+_-=")  == "(){}<>+_-="

Answer 2

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

Answer 3

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.

Answer 4

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()))

Answer 5

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.

Answer 6

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.)