6
\$\begingroup\$

About 2 months ago I started studying python and then 3 days ago sat and began to write a program (found the dumbest & simplest thought I could hehe (a pyglatin translator)). I finally got it working 100%. Being a beginner I would love to hear some constructive criticisim on my program because I know I have probably wrote more then was needed on some stuff and taken a long way around on others.

another_word = " "
sentence1 = " "
new_list = []
another = " "



def trans_one_word (i):
    global another_word
    pyg = 'ay'

    original = (i)

    if len(original) > 0 and original.isalpha():
        word = original.lower()
        first = word[0]
        new_word = word + first + pyg
        another_word = new_word[1:len(new_word)]
        return another_word
    else:
        print('empty')


def input_state():

    sentence = input("enter a word to be translated: ")
    list = sentence.split()
    for i in list:
        trans_one_word(i)
        new_list.append(another_word)
    s = " "
    print (s.join( new_list ))


input_state()


another = input("Would you like to enter another sentence to be     translated? \n Y/N")

if (another == 'y') or (another == 'Y'):
    input_state()

All in all I am proud of myself for actually writing it without any help, or all of it except the last "if" statement (I have minor problems with it).

\$\endgroup\$
3
\$\begingroup\$

Some comments on your code. Overall you are using a lot of unnecessary variables.

another_word = " "
sentence1 = " "
new_list = []
another = " "

You really don't need these variables declared outside of your functions, they add no value and just pollute the namespace.

def trans_one_word (i):

PEP8 doesn't recommend a before the args. And you should consider having more meaningful arg names.

    global another_word

Unnecessary global, you are returning this variable from the function so don't need it to be global

    pyg = 'ay'

    original = (i)

Parens are unnecessary and you could just call the arg original as you don't need this assignment.

    if len(original) > 0 and original.isalpha():
        word = original.lower()
        first = word[0]
        new_word = word + first + pyg
        another_word = new_word[1:len(new_word)]

You don't need len(new_word) on a slice an empty slice implies to the end of the word, e.g. new_word[1:]

        first = word[0]
        new_word = word + first + pyg
        another_word = new_word[1:len(new_word)]
        return another_word

This whole block can be simplified to return word[1:] + word[0] + pyg

    else:
        print('empty')

You don't need the else: clause because the previous block returned. You should also return the original word.

def input_state():

    sentence = input("enter a word to be translated: ")

Assume you meant to ask for a words or a sentence given the variable name:

    list = sentence.split()

list is a python type, so shouldn't be used for a variable name.

    for i in list:
        trans_one_word(i)
        new_list.append(another_word)
    s = " "

You should look into list comprehensions and generators as this can be significantly simplified, e.g. print(" ".join(trans_one_word(i) for i in sentence.split()))

    print (s.join( new_list ))

It's probably better to return this result and let the caller print it.

input_state()

another = input("Would you like to enter another sentence to be     translated? \n Y/N")

if (another == 'y') or (another == 'Y'):
    input_state()

This condition can be replaced with if another in 'yY': You should really put this all into a loop so with an exit clause.

Simplifying the code:

def trans_one_word(word):
    pyg = 'ay'

    if word and word.isalpha():
        word = word.lower()
        return word[1:] + word[0] + pyg
    return word

def input_state():
    sentence = input("enter a sentence to be translated: ")
    return " ".join(trans_one_word(word) for word in sentence.split())

while True:
    print(input_state())
    another = input("Would you like to enter another sentence to be translated?\nY/N")
    if another not in 'yY':
        break
\$\endgroup\$
  • 1
    \$\begingroup\$ Note that the original code would translate one or two sentences. Your solution offers to translate infinitely many sentences. \$\endgroup\$ – 200_success Oct 30 '16 at 5:10
  • \$\begingroup\$ I suggest also eliminating pyg = 'ay' and writing return word[1:] + word[0] + 'ay'. \$\endgroup\$ – 200_success Oct 30 '16 at 5:10
  • \$\begingroup\$ Yes, a loop structure for the request was part of my feedback, as it was unclear whether the intent was for 1+ vs. just 1 or 2. So an assumption on my part - which may ultimately be incorrect. \$\endgroup\$ – AChampion Oct 30 '16 at 14:11
  • \$\begingroup\$ AChampion, certain lines I did the way that I did for my own clarification (to keep it clear in my head). I agree, I should have used a lont of comments rather then coding in the way I did. \$\endgroup\$ – Michael Howard Oct 31 '16 at 17:48
  • \$\begingroup\$ it is simply meant to take one sentence and translate it then ask if you wish to do another sentence. if len(original) > 0 and original.isalpha(): I used len(original) > to make sure there was an actual word there or at least a letter. I used original.isalpha() to make sure it was letters and not numbers. print empty if the info entered was either blank or if it was numbers. \$\endgroup\$ – Michael Howard Oct 31 '16 at 17:59

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.