Make this code look more elegant

Question

I am absolutly new in python and in programing, I made this bifid cipher, I'd like to hear opinions about how to improve and make it look more elegant, thanks in advance.

I been taking courses in Codecademy and Udacity, and I've learn quite a lot.

import itertools


#Genera coodernadas-Generate Coordinates
coordinates = [[x,y] for x in range(1,6) for y in range(1,6)]

#Genera alfabeto-Generate Alphabet
alfa = []
for i in range(97,123):
    alfa.append(chr (i))
alfa.remove("i")

#Genera diccionario de coordenadas y alfabeto - Generate dictionary and coordinates alphabet 
alfacor = {}
alfacor = dict(zip(alfa,coordinates))


#Leer Txt - Read txt
document = open("Z:\\R\\Desktop\\BIFIDO\\easy.txt")
contenido = document.read()
print (contenido)
document.close()

#Encripta fase1 - Get's coordinates of txt
encripta = []
for e in contenido:
    encripta.append(alfacor[e])

#Unir lista encripta - Merge content of encropita in a new list
merged = list(itertools.chain.from_iterable(encripta))

#Divido lista merge en partes iguales - Divide meged list to get new coordinates
B = merged[:len(merged)/2]
C = merged[len(merged)/2:]

#Unir B y C - Zip B and C to get a new list of coordinates
zipped = zip(B,C)

#Make a new list from zipped to convert from tuple to list
final_list = [list(elem) for elem in zipped]

#Convert contect of alfacor to tuples
inv_alfacor = {}
for letter, coordinate in alfacor.iteritems():
inv_alfacor[tuple(coordinate)] = letter

#Substitude coordinates of final_list from elements of inv_alfacor
encripta_f = []
for element in final_list:
    element = tuple(element)
    if element in inv_alfacor:
        encripta_f.append(inv_alfacor[element])

print "Tu palabra ",encripta_f

This question looks more on-topic for [code review](http://codereview.stackexchange.com/help/on-topic). Asking for opinions about code style is off-topic here. — Radiodef, Apr 07 '15 at 07:02
Thanks for the tip, did not know about it, will take on mind in futher posts. — etsous, Apr 07 '15 at 13:14

score 0 · Answer 1 · edited May 23 '17 at 12:13

Use with statement

You can read more in python docs or in this article Understanding Python's "with" statement

Suggested modification:

#Leer Txt - Read txt
with open("Z:\\R\\Desktop\\BIFIDO\\easy.txt", "r") as document:
    contenido = document.read()
    print (contenido)

Use list comprehensions

More info in Python docs or in the tutorial Python Tutorial: List Comprehensions

Suggested modification:

#Genera alfabeto-Generate Alphabet
alfa = [chr(i) for i in xrange(97, 123) if chr(i) != "i"]

(Note, please, this change also includes a condition in the list comprehension - example at SO question)

And also:

#Encripta fase1 - Get's coordinates of txt    
encripta = [alfacor[e] for e in contenido]

Use generators

The first thing you can start with is following. When you write a list comprehension and you know you are going to iterate only one item of the list a time, change the brackets from [] to (). This is really simplified but it is the first thing you can do. Another related tip, when you use range(x) just like for i in range(x), use xrange(x) instead. xrange is a generator version of range.

More info in Python Wiki

Suggested change:

#Make a new list from zipped to convert from tuple to list
final_list = (list(elem) for elem in zipped)

Printing

In this case, using the print you used is fine, but have a look at string formatting.

More in Python docs and a few examples here.

Possible change:

print "Tu palabra {}".format(encripta_f)

You don't need to initialize all the variables.

You don't need to initialize alfacor dictionary when you assign a completely new value to the variable. However, you need to initialize variable when you work with it later.

Thus there is a difference between

# no need for initialization
alfacor = {}
# because you assign a new value here to the variable `alfacor`
alfacor = dict(zip(alfa,coordinates))

and this:

# you need to initialize this
alfacor = {}
# because you are working with the empty dictionary here
alfacor["my_key"] = "my_value"

@Marek Wow, thanks a lot for the detail explanation, this will really help in the near future. — etsous, Apr 07 '15 at 13:11
@etsous Glad I could help. Have a look at this tutorial [Code Like a Pythonista: Idiomatic Python](http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html) It is probably the most helpful Python material I have ever read. — Marek, Apr 07 '15 at 13:33

AChampion · Accepted Answer · 2015-04-07T07:35:27.717

Other than perhaps using comprehensions and avoid the unnecessary tuple -> list -> tuple conversions, reduce the number of intermediate variables then it might be slightly easier to read. I would also consider making it a function that you pass in a string and an encrypted string is returned:

from itertools import chain, product

def bifid(data):
    # Leave coordinates as tuples and appropriate use of itertools.product
    coordinates = product(range(1, 6), repeat=2)

    # Using comprehensions and generators to construct the list/dicts vs loops
    # Leave alfa as a generator as it is only used once
    alfa = (chr(i) for i in range(97, 123) if chr(i) != 'i')
    alfacor = dict(zip(alfa, coordinates))
    inv_alfacor = {coordinate: letter for letter, coordinate in alfacor.iteritems()}
    encripta = (alfacor[e] for e in data)

    merged = list(chain(*encripta))
    final_list = zip(merged[:len(merged)//2], merged[len(merged)//2:])

    return "".join(inv_alfacor[e] for e in final_list if e in inv_alfacor)

# Use with it closes automatically and handles exceptions correctly
with open("Z:\\R\\Desktop\\BIFIDO\\easy.txt") as document:
    data = document.read()]

print "Tu palabra: {}".format(bifid(data))

Output:

"helloworld" -> Tu palabra: kmcyobnalt

I am really impress about code reduction, thanks a lot. – etsous Apr 07 '15 at 13:09 — etsous, Apr 07 '15 at 13:09

Make this code look more elegant

2 Answers2