leighflix
commented
9 years ago wow... thanks so much for the feedback!
On Tue, Aug 18, 2015 at 1:56 AM, Ryan Kraus notifications@github.com wrote:
I saw on Reddit that you were looking for some feedback on your Morse Code translator. It inspired me to quickly hack together my own to show you some different concepts with Python that you may not have come across yet. It is posted here https://gist.github.com/rmkraus/f245f0663ca25b4c18f3.
The biggest difference from yours is that I have created one generic function for doing all the translating. The MORSE to ALPHA conversion is quite similar to the ALPHA to MORSE conversion so they don't necessarily require their own functions. In order to accommodate this change, I am storing the alphabets in independent lists rather than a dictionary. I also used a list comprehension to do most of the translating. You may not be familiar with those yet, but you eventually will want to learn about them and how they work. They are a very powerful Python tool.
With yours, I would suggest using only descriptive variable names. Names like item, item2, or item3 don't really tell anyone what is actually inside of those variables. Instead use things like index, morse_letter, or alpha_letter.
I also see that you used a loop to go through a dictionary. This is a rather inefficient way to do what dictionaries already do. In Python, you can also loop through a list of items directly rather than relying on an index only. If I were to rewrite your lines 43 to 52, it would look something like this:
alpha_alphabet = {morse: alpha for alpha, morse in Morse_Alphabet.items()} # interpreted dict to flip the Morse_Alphabet dictionaryfor ind, letter in enumerate(morse_list): secondary_list[ind] = alpha_alphabet[letter]
This uses the enumerate function to loop through the morse_list as well as get the current index. It also uses a dictionary as it was intended to be used. It is worth noting, that with this example if a character is provided that is not in the dictionary, a KeyError will be thrown. This example also needs the / character to be defined in the dictionary. This makes sense because it is a valid character in the alphabet you are converting.
Even in that example, it is a bit weird to flip a dictionary like that. There is nothing wrong with it, but it is just weird. Having to do weird things is a pretty good sign that you may want to consider using a different data type for storing your alpha to morse conversions. Dictionaries are great for one way conversions, but are not explicitly built to reverse the operation.
As a small point of coding style, you should read PEP 8 https://www.python.org/dev/peps/pep-0008/. It defines a good universal coding style that many Python developers use. If you applied PEP8 to your code, Morse_Alphabet would be renamed to MORSE_ALPHABET and you would have two blank lines before your functions. Other than that, it is pretty clean.
With your list copying to secondary_list, that is also kind of an odd thing to do. Usually you shouldn't have to do something like this. Instead, maybe, you could create seconday_list as an empty list and just append values to it rather than replacing things in place. That is more the norm for things like this. Interpreted lists are also your friend in situations like this.
That is all that comes to mind this late at night. My example isn't quite perfect, but it is a different view on the same problem. I am not satisfied with my main function. If I were deploying code like this, I would clean it up a bit. At either rate, it looks like you are headed down the right path and this is quite good code for someone who has only been using the language for about a month. Once you become more comfortable, I would recommend finding a project on GitHub that sparks your interest and doing some contributions. The best way to learn is to read code form people with more experience, make your own code, and get feedback.
Never stop learning and happy coding!
— Reply to this email directly or view it on GitHub https://github.com/leighflix/Small-Projects/issues/1.
I saw on Reddit that you were looking for some feedback on your Morse Code translator. It inspired me to quickly hack together my own to show you some different concepts with Python that you may not have come across yet. It is posted here.
The biggest difference from yours is that I have created one generic function for doing all the translating. The MORSE to ALPHA conversion is quite similar to the ALPHA to MORSE conversion so they don't necessarily require their own functions. In order to accommodate this change, I am storing the alphabets in independent lists rather than a dictionary. I also used a list comprehension to do most of the translating. You may not be familiar with those yet, but you eventually will want to learn about them and how they work. They are a very powerful Python tool.
With yours, I would suggest using only descriptive variable names. Names like item, item2, or item3 don't really tell anyone what is actually inside of those variables. Instead use things like index, morse_letter, or alpha_letter.
I also see that you used a loop to go through a dictionary. This is a rather inefficient way to do what dictionaries already do. In Python, you can also loop through a list of items directly rather than relying on an index only. If I were to rewrite your lines 43 to 52, it would look something like this:
This uses the enumerate function to loop through the morse_list as well as get the current index. It also uses a dictionary as it was intended to be used. It is worth noting, that with this example if a character is provided that is not in the dictionary, a KeyError will be thrown. This example also needs the / character to be defined in the dictionary. This makes sense because it is a valid character in the alphabet you are converting.
Even in that example, it is a bit weird to flip a dictionary like that. There is nothing wrong with it, but it is just weird. Having to do weird things is a pretty good sign that you may want to consider using a different data type for storing your alpha to morse conversions. Dictionaries are great for one way conversions, but are not explicitly built to reverse the operation.
As a small point of coding style, you should read PEP 8. It defines a good universal coding style that many Python developers use. If you applied PEP8 to your code, Morse_Alphabet would be renamed to MORSE_ALPHABET and you would have two blank lines before your functions. Other than that, it is pretty clean.
With your list copying to secondary_list, that is also kind of an odd thing to do. Usually you shouldn't have to do something like this. Instead, maybe, you could create seconday_list as an empty list and just append values to it rather than replacing things in place. That is more the norm for things like this. Interpreted lists are also your friend in situations like this.
That is all that comes to mind this late at night. My example isn't quite perfect, but it is a different view on the same problem. I am not satisfied with my main function. If I were deploying code like this, I would clean it up a bit. At either rate, it looks like you are headed down the right path and this is quite good code for someone who has only been using the language for about a month. Once you become more comfortable, I would recommend finding a project on GitHub that sparks your interest and doing some contributions. The best way to learn is to read code form people with more experience, make your own code, and get feedback.
Never stop learning and happy coding!