2
\$\begingroup\$

I wrote the below code and tested some example files on it. I didn't find any errors, but I still want to be thorough about this.

Also, any suggestions in improving the code?

import string
""" this implements an encryption standard over the message text
it replaces the odd place alphabets by their ordinal number plus place number increased.
it replaces the even place alphabets by their ordinal number minus place number decreased.
for example "hello" becomes "icoht" and "abc" becomes "bzf".
"""
input_filename = raw_input("Enter the input file name: ")
output_filename = raw_input("Enter the output dumping file name: ")
mode = raw_input("Do you want to encrypt or decrypt (e/d)?: ")
with open(input_filename,'r') as inputfile:
 with open(output_filename,'w') as outputfile:
 inp = inputfile.read()
 encode = lambda s: "".join(
 (chr(ord('a')+((ord(c)-(ord('a')+i+1))%26))) if ((c.isalpha()) and ((i+1)% 2 ==0))
 else
 string.ascii_lowercase[(string.ascii_lowercase.rfind(c.lower())+i+1)%26] if ((c.isalpha()) and ((i+1)% 2 ==1))
 else
 c
 for i,c in enumerate(s.lower())
 )
 decode = lambda s: "".join(
 string.ascii_lowercase[(string.ascii_lowercase.rfind(c.lower())+i+1)%26] if ((c.isalpha()) and ((i+1)% 2 ==0)) 
 else
 (chr(ord('a')+((ord(c)-(ord('a')+i+1))%26))) if ((c.isalpha()) and ((i+1)% 2 ==1))
 else
 c
 for i,c in enumerate(s.lower())
 )
 if(mode.lower() == 'e'):
 outputfile.write(encode(inp))
 else :#(mode.lower() =='d'):
 outputfile.write(decode(inp))
 s = {'e':'Encoding','d':'Decoding'}
 print s.get(mode)+" Sucessfull"
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Aug 16, 2013 at 13:17
\$\endgroup\$
4
  • 1
    \$\begingroup\$ ((i+1)% 2) ==0) is more traditionally written as (i % 2 == 1). Conversely, ((i+1)% 2) == 1) is the same as (i % 2 == 0). \$\endgroup\$ Commented Aug 16, 2013 at 17:36
  • \$\begingroup\$ thnx @200_success \$\endgroup\$ Commented Aug 16, 2013 at 17:53
  • \$\begingroup\$ What versions of python are you expecting to work with? \$\endgroup\$ Commented Aug 23, 2013 at 16:35
  • \$\begingroup\$ python 2.7 @belacqua \$\endgroup\$ Commented Aug 24, 2013 at 8:41

2 Answers 2

2
\$\begingroup\$

Functions are your friend. The remove repeated code and improve readability. I have absolutely no idea what the block of text does inside the "".join(XXXX) calls. There is too much going on there that I'm not going to take the time to see what it does or if it can be done better.

However, it does appear that you can replace that mess with something like this:

def doA():
 pass
def doB():
 pass
def makeJoinArgument(value, which_step,step1, step2):
 return step1() if (which_step) else step2() for x in value
def encode(value):
 return "".join("abc", True, doA, doB)
def decode(value):
 return "".join("abc", True, doB, doA)

This obviously doesn't run (and not exactly what you have), but it is much more manageable than what you have now.

Another note (from PEP 8: Style Guide)

Always use a def statement instead of an assignment statement that binds a lambda expression directly to a name.

Yes:

def f(x): return 2*x

No:

f = lambda x: 2*x

The first form means that the name of the resulting function object is specifically 'f' instead of the generic ''. This is more useful for tracebacks and string representations in general. The use of the assignment statement eliminates the sole benefit a lambda expression can offer over an explicit def statement (i.e. that it can be embedded inside a larger expression)

answered Aug 16, 2013 at 15:00
\$\endgroup\$
1
  • \$\begingroup\$ Your suggested code has several errors. Can you fix, please? \$\endgroup\$ Commented Dec 10, 2013 at 11:19
1
\$\begingroup\$

Your encode and decode functions are absurdly long for 'one liners'. Also they contain these two lines

(chr(ord('a')+((ord(c)-(ord('a')+i+1))%26))) if ((c.isalpha()) and ((i+1)% 2 ==0))

and

string.ascii_lowercase[(string.ascii_lowercase.rfind(c.lower())+i+1)%26] if ((c.isalpha()) and ((i+1)% 2 ==1))

which both do similar things in a different and needlessly complicated way.

A more readable and simpler version of the encode function would be:

def encode(s):
 r = list(s.lower())
 abc = string.lowercase()
 for i, c in enumerate(r):
 if c in abc:
 if i % 2:
 r[i] = abc[(abc.index(c) - i - 1) % 26]
 else:
 r[i] = abc[(abc.index(c) + i + 1) % 26]
 return ''.join(r)

In terms of structure you probably don't want to have the encode/decode functions defined inside the with in your main function, but separately:

import string
def encode(s):
 ...
def decode(s):
 ...
def main():
 """ this implements etc.
 """
 input_filename = raw_input("Enter the input file name: ")
 output_filename = raw_input("Enter the output dumping file name: ")
 mode = raw_input("Do you want to encrypt or decrypt (e/d)?: ").lower()
 with open(input_filename,'r') as inputfile:
 with open(output_filename,'w') as outputfile:
 inp = inputfile.read()
 if mode is 'e':
 outputfile.write(encode(inp))
 print 'Encoding successful'
 else :#(mode.lower() =='d'):
 outputfile.write(decode(inp))
 print 'Decoding successful'
answered Aug 17, 2013 at 1:46
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.