first draft of 'atm' - #128
Conversation
| except Exception as e: | ||
| print(e) |
There was a problem hiding this comment.
is there a reason that you catch just any exception here and print it?
if no, it is a VERY BAD habit to except the exception of the class Exception.
| @@ -0,0 +1,137 @@ | |||
| # ATM Exercise | |||
There was a problem hiding this comment.
it's very good that you added documentation for the script.
but when you do that, use """My documentation""" and not # My documentation
| A dictionary, the key is each users id. | ||
| the value is a list of user's password and balance. |
There was a problem hiding this comment.
In the Return: section of the documentation, the doc need to start with the type of the returned value, and then the description of that value.
| Adictionary, thekeyiseachusersid. | |
| thevalueisalistofuser'spasswordandbalance. | |
| dict. WithusersIDaskeysanduser'sdataasvalues. |
| BALANCE_INDEX = 1 | ||
| def read_file_move_to_dict(file_path): |
| def read_file_move_to_dict(file_path): | ||
| """Read file and move atm information to dictionary. | ||
There was a problem hiding this comment.
the actions of this function look a bit complicated, and it's hard to understand what you are doing.
Add a description here of what you expect the file to look like.
| while True: | ||
| user_id_input = input("Enter costumer ID, to turn off the ATM enter " | ||
| "-1 -> ") | ||
| if user_id_input == -1: | ||
| atm_options_dict = None | ||
| break | ||
| elif user_id_input in atm_options_dict: | ||
| break | ||
| else: | ||
| print("Invalid ID") |
| else: | ||
| print("Invalid ID") | ||
| while True and atm_options_dict is not None: |
There was a problem hiding this comment.
as i see it, there is no need for the True and
| whileTrueandatm_options_dictisnotNone: | |
| whileatm_options_dictisnotNone: |
| user_option = input("Enter wanted option, to turn off the ATM enter -1" | ||
| + "\nEnter 'options' to see ATM's options. -> ") | ||
| user_option = user_option.lower() | ||
| if user_option == "-1": | ||
| read_dict_move_to_file(file_path, atm_options_dict) | ||
| break | ||
| elif user_option == "options": | ||
| print("The ATM allow the following options to a using costumer:" + | ||
| "\n1. 'check' the balance.\n2. cash 'withdrawal'." + | ||
| "\n3. cash 'deposit'.\n4. 'change' password.") | ||
| else: | ||
| atm_options_dict = check_input_execute(atm_options_dict, | ||
| user_option, user_id_input) |
| user_option = input("Enter wanted option, to turn off the ATM enter -1" | ||
| + "\nEnter 'options' to see ATM's options. -> ") | ||
| user_option = user_option.lower() | ||
| if user_option == "-1": |
There was a problem hiding this comment.
it looks like you are going to use the -1 multiple times in your code.
when you have such a case, better save the -1 into a CONSTANT variable, and use the variable instead of the -1 in your code.
this way, if some changes are about to come (for example, -1 changed to "turn off"), it will be easy for you to handle it.
| import sys | ||
| FILE_PATH = 1 |
There was a problem hiding this comment.
this constant variable sounds like a string. why is it equal to 1?
maybe add a comment that describes these constant variables
| atm_dict (dictionary): the dictionary, containing all the atm information. | ||
| file_path (string): path to atm information file. | ||
| atm_dict (dictionary): the dictionary, containing all the atm | ||
| information. |
There was a problem hiding this comment.
indent for a continued line should only be 4 spaces inward from the previous line.
| file_path (string): path to atm information file. | ||
| Return: | ||
| A dictionary, the key is each users id. |
There was a problem hiding this comment.
write the return as follows:
Returns:
: information about it.
See here for most of the important style things you need to know https://github.com/osherdp/python-training/blob/yaakov_branch/Style_Guide.md
| the value is a list of user's password and balance. | ||
| """ | ||
| try: | ||
| atm_options_dict = {} |
There was a problem hiding this comment.
We don't use the type in the variable name. You can just call it atm_options.
Look over your code and change the name of all of your variable names which contain types (pycharm's refactor function should make it easier).
| while True: | ||
| user_id_input = input("Enter costumer ID, to turn off the ATM enter " | ||
| "-1 -> ") | ||
| if user_id_input == -1: |
There was a problem hiding this comment.
You need to convert the input to a number first in order for this if statement to work
| print("Invalid ID") | ||
| while True and atm_options_dict is not None: | ||
| user_option = input("Enter wanted option, to turn off the ATM enter -1" |
There was a problem hiding this comment.
You don't need the ''+"
When a string goes onto the next line python understands that its one big string (even though each line has its own quotation marks)
| try: | ||
| with open(file_path, 'w') as file_pointer: | ||
| for key in atm_dict: | ||
| file_pointer.write(','.join([str(key), atm_dict[key][0], |
There was a problem hiding this comment.
This will be easier to read if you first assign atm_dict[key][0] and atm_dict[key][1] to variables. That way it will be clear exactly what you are writing to the file without someone going back to you dictionary and seeing how values are stored inside it.
| import sys | ||
| FILE_PATH = 1 |
There was a problem hiding this comment.
good use of constants.
However is this case the name is a but unclear. FILE_PATH sounds like the string telling me where the file goes not the index of the file path in the arguments
| Return: | ||
| A dictionary, the key is each users id. | ||
| the value is a list of user's password and balance. |
There was a problem hiding this comment.
For cases like this we usually use tuples instead of lists. Tuples are immutable (cannot be overwritten) so we use them for cases where we aren't changing (ie. mutating) the information inside of them.
All you will need to do to change the brackets you used to create the list to parenthesis to create a tuple.
read more here especially number 11. https://data-flair.training/blogs/python-tuples-vs-lists/
There was a problem hiding this comment.
but i need to change the vlues in it, when the user want to change password or deposit or withdraw money. i think that if i will use a tuple, i will need to cahnge it to list, change the thing the user want and then change it back.
| if user_input == "check": | ||
| print(f"You'r balance: {atm_dict[user_id][BALANCE_INDEX]}") | ||
| elif user_input == "withdrawal": | ||
| withdrawal = input("Enter how much you want to withdrawal -> ") | ||
| if float(withdrawal) > 0: | ||
| atm_dict[user_id][BALANCE_INDEX] -= float(withdrawal) | ||
| print("You have withdrawn: " + withdrawal) | ||
| elif user_input == "deposit": | ||
| deposit = input("Enter how much you want to withdrawal -> ") | ||
| if float(deposit) > 0: | ||
| atm_dict[user_id][BALANCE_INDEX] += float(deposit) | ||
| print("Deposit succeeded") | ||
| elif user_input == "change": | ||
| new_password = input("Enter you'r new password -> ") | ||
| atm_dict[user_id][PASSWORD_INDEX] = new_password | ||
| print("Password changed to: " + new_password) |
There was a problem hiding this comment.
Your function is too long, divide it to smaller methods.
It will make your code clearer and more readable even if it will be a bit longer.
| print("Changes saved :)") | ||
| except FileNotFoundError: | ||
| print("Something went wrong, when accessing file") |
There was a problem hiding this comment.
The program is raising an exception saying the file was not found.
You catch it and instead print only partial information that something went wrong.
First thing would be to print all the information about the file the user tried to open and the fact that it doesn't exist.
Or you could simply just let the program crash throwing an exception saying that the file was not found.
Chose wisely ;)
| print("Invalid input") | ||
| except ValueError: | ||
| print("Invalid input") |
There was a problem hiding this comment.
When will that happen? I see that if a user input a non existing command that would be noticed above.
| elif user_input == "withdrawal": | ||
| withdrawal = input("Enter how much you want to withdrawal -> ") | ||
| if float(withdrawal) > 0: |
There was a problem hiding this comment.
If a user entered a negative amount, nothing will happen and the user will not even know it. output something to the user telling him his input wasn't correct.
| @@ -0,0 +1,162 @@ | |||
| """ATM Exercise""" | |||
| FILE_PATH_INDEX = 1 # describe placement of the argument containing file path. | ||
| PASSWORD_INDEX = 0 | ||
| BALANCE_INDEX = 1 | ||
| END_ATM_SERVICE = "-1" |
| """Read file and move atm information to dictionary. | ||
| The lines of the file will contain user's id, balance and password, |
There was a problem hiding this comment.
blank line between function description and additional notes
| dict. with user's id as keys, | ||
| and user's password and balance in a list as values. |
There was a problem hiding this comment.
if you have more then 1 line for return \ arg doc, the lines after the first one should start with indent
for example:
dict. something
____continue something (it doesn't show space in comment, so I putted _ instead)
| print(f"Can't access the path to the file you have entered:" | ||
| f"{file_path}") |
There was a problem hiding this comment.
its look like file not found error, not file cannot be accessed
| def check_balance(atm_info, user_id): | ||
| """function to check user's balance in the atm dict""" | ||
| new_password = input("Enter you'r new password -> ") | ||
| atm_info[user_id][PASSWORD_INDEX] = new_password | ||
| print("Password changed to: " + new_password) |
There was a problem hiding this comment.
function name and doc doesn't fit the logic
| def check_balance(atm_info, user_id): | ||
| """function to check user's balance in the atm dict""" | ||
| new_password = input("Enter you'r new password -> ") | ||
| atm_info[user_id][PASSWORD_INDEX] = new_password | ||
| print("Password changed to: " + new_password) |
There was a problem hiding this comment.
Who would want his password to print on screen :)
| user_id_input, atm_options = get_user_id(atm_options) | ||
| while atm_options is not None: |
There was a problem hiding this comment.
This doesn't make very much sense.
I would expect to keep waiting for input until -1 received, not until my DB is None
(its counter intuitive)
| user_id_input, atm_options = get_user_id(atm_options) | ||
| while atm_options is not None: |
There was a problem hiding this comment.
The ATM should support multiple users in one run
| """ | ||
| try: | ||
| with open(file_path, 'w') as file_pointer: | ||
| for key in atm_info: |
| Returns: | ||
| dict. with user's id as keys, | ||
| and user's password and balance in another dict as values. |
There was a problem hiding this comment.
When one dictionary (or list or anything else) is in another the term for that is "nested." Your comment is therefore better expressed as "in a nested dict as values."
Its clearer what you mean this way
| atm_options = {} | ||
| with open(file_path, 'r') as file_pointer: | ||
| for line in file_pointer: | ||
| line_list = line.rstrip().split(',') |
There was a problem hiding this comment.
Its our convention to avoid using types ("list", "dict", "string") in variable names. Better to add more information in the name or just leave it as something like "lines"
No description provided.