Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Now your button declarations would look like this (see here here for the reasoning behind using lambda for go_button):

Now your button declarations would look like this (see here for the reasoning behind using lambda for go_button):

Now your button declarations would look like this (see here for the reasoning behind using lambda for go_button):

added 30 characters in body
Source Link
BeetDemGuise
  • 4.2k
  • 12
  • 29
manager = MyFileManager()
filename_button = tkinter.Button(window, text="Browse", command=manager.get_filename)
save_filename_button = tkinter.Button(window, text="Browse", command=manager.get_save_file)
go_button = tkinter.Button(window, text="GO!", command=lambda: manager.save_users_list(
 list_name.get()))
  • Notice how in my examples above I use_underscores instead of camelCase for variable names. This is the Python convention.
  • When thinking of variable names, err on the side of being too verbose rather than too brief: list_name_label is a much better name than listNameLBL.
  • You mostly do a good job of this, but for GUI widgets remember to append the type of widget in the variable name. You forgot to do this on lyouryour filename button widget.
filename_button = tkinter.Button(window, text="Browse", command=manager.get_filename)
save_filename_button = tkinter.Button(window, text="Browse", command=manager.get_save_file)
go_button = tkinter.Button(window, text="GO!", command=lambda: manager.save_users_list(
 list_name.get()))
  • Notice how in my examples above I use_underscores instead of camelCase for variable names. This is the Python convention.
  • When thinking of variable names, err on the side of being too verbose rather than too brief: list_name_label is a much better name than listNameLBL.
  • You mostly do a good job of this, but for GUI widgets remember to append the type of widget in the variable name. You forgot to do this on lyour filename button widget.
manager = MyFileManager()
filename_button = tkinter.Button(window, text="Browse", command=manager.get_filename)
save_filename_button = tkinter.Button(window, text="Browse", command=manager.get_save_file)
go_button = tkinter.Button(window, text="GO!", command=lambda: manager.save_users_list(
 list_name.get()))
  • Notice how in my examples above I use_underscores instead of camelCase for variable names. This is the Python convention.
  • When thinking of variable names, err on the side of being too verbose rather than too brief: list_name_label is a much better name than listNameLBL.
  • You mostly do a good job of this, but for GUI widgets remember to append the type of widget in the variable name. You forgot to do this on your filename button widget.
Source Link
BeetDemGuise
  • 4.2k
  • 12
  • 29

Structure

Firstly, try not to use the global keyword. This keyword should really be used only if you HAVE to. A simple solution for this is to implement a class that wraps around all of the functions:

class MyFileManager(object):
 def __init__(self, filename='', save_filename=''):
 self.filename = filename
 self.save_filename = save_filename
 # Re-named `program` function
 def save_users_list(self, list_name):
 ...
 def get_filename(self):
 self.filename = askopenfilename(filetypes=(("Text files", "*.txt"),
 ("All files", "*.*")))
 def get_save_file(self):
 self.save_filename = asksaveasfilename(filetypes=(("Text files", "*.txt"),
 ("All files", "*.*")))

Now your button declarations would look like this (see here for the reasoning behind using lambda for go_button):

filename_button = tkinter.Button(window, text="Browse", command=manager.get_filename)
save_filename_button = tkinter.Button(window, text="Browse", command=manager.get_save_file)
go_button = tkinter.Button(window, text="GO!", command=lambda: manager.save_users_list(
 list_name.get()))

Naming

  • Notice how in my examples above I use_underscores instead of camelCase for variable names. This is the Python convention.
  • When thinking of variable names, err on the side of being too verbose rather than too brief: list_name_label is a much better name than listNameLBL.
  • You mostly do a good job of this, but for GUI widgets remember to append the type of widget in the variable name. You forgot to do this on lyour filename button widget.

program Function

Your orignal function has a few problems/tweaks that can be made:

  • The name is very ambiguous. Update it to better reflect what the function does.
  • You used the with statement to open filename. However, when you open saveName you don't. Because of that you forgot to close() that file. ALWAYS use with when accessing files as it will save you these mistakes.
  • I prefer to always say which mode we are opening files in. So add the mode ('r') to the with statement.
  • Try to make the amount of time a file is open the smallest it can be. So instead of having filename open for the duration of the function only keep it open when creating longList.
  • Instead of doing multiple string concatenations, use format() to create your finList.
  • There is no need for the parens around variables.

Here is my version of your program function. Assume it is inside the class I outlined above:

def save_users_list(self, list_name):
 with open(self.filename, 'r') as file:
 numbers_list = [line.strip() for line in file.readLines()]
 with open(self.save_filename, 'w') as file:
 file.write('{} = {}'.format(list_name, str(numbers_list)))

As a final general comment, take a look at PEP8, the official Python style guide. It will help your code look and feel more Pythonic.

lang-py

AltStyle によって変換されたページ (->オリジナル) /