I'm very new to programming, and I'm hoping I can get feedback on the code I wrote - anything is helpful - what to split into different files (right now 1 large file), perhaps a way to break up the main_excel_menu
function, etc. If you notice issues with naming conventions, etc., I appreciate that as well.
Essentially I'm doing this as a way of learning techniques in Python (please don't point out pandas
would do anything I did faster, I know).
The code currently lets a user import an Excel file, choose a sheet, and then view the data in the sheet. Eventually I will build in data manipulation and saving functions, but before doing that, I want to know how to organize everything.
import openpyxl
import sys
def open_workbook():
print("\nWhich Excel File to open?\n")
file = input()
wb = openpyxl.load_workbook(file + ".xlsx")
print(f"\nSuccessfully opened '{file}.xlsx' \n")
return wb
def choose_sheet(workbook):
sheet = None
while sheet is None:
print("please choose a sheet")
print_sheets_in_workbook(workbook)
try:
index = int(input())
sheet = workbook.worksheets[index - 1]
#to avoid an option 0, I add one to the index in the
#print function, so have to subtract here
return sheet
except ValueError:
print("please input numerical choice")
except IndexError:
print("Please pick an available choice")
def import_excel_sheet_data(sheet):
maxRow = sheet.max_row
maxColumn =sheet.max_column
importedData = []
for row in range (0, maxRow): #loop through the rows
rowList =[] #create a list to hold the entire row
for column in range(0, maxColumn): #add each column of the row to the temp list
rowList.append(sheet.cell(row+1, column+1).value)
#when finished getting all the columns add as a list to importedData as 1 row
importedData.append(rowList)
return importedData
def create_sheet(wb, sheetName, idx):
pass
def print_sheets_in_workbook(workbook):
listOfSheets = workbook.sheetnames
for idx, sheet in enumerate(listOfSheets):
print(idx+1, ":", sheet)
def main_excel_menu():
loop = True
wb = None
sheet = None
sheetData = None
options = [
"0. You shouldn't be able to see me",
"1. Open an excel / different excel file",
"2. Display current workbook and sheet",
"3. Print contents of current sheet",
"4. Choose a different sheet",
"5. Go to data management",
"6. exit the program",
]
while(loop == True):
[print(option) for idx, option in enumerate(options) if idx !=0]
try:
choice = int(input("\n"))
if choice == 1:
wb = open_workbook()
sheet = choose_sheet(wb)
sheetData = import_excel_sheet_data(sheet)
elif choice == 2:
#is there a way to write 1 line code for this? can't find it
if sheet is not None:
print("current sheet is: ", sheet.title, "\n")
else:
print("No sheet selected")
elif choice == 3:
print(sheetData)#ugly, will need to write function to format
elif choice == 4:
sheet = choose_sheet(wb)
sheetData = import_excel_sheet_data(sheet)
elif choice == 5:
if sheetData != None:
print("Transfer to data management menu")
#todo - add menu fucntion later
#have to pass wb, sheet, data as parameters probably
loop = False
else: print("No data has been loaded! \
\nPlease choose a sheet to work on first\n")
elif choice == 6:
exit()
else:
print("That is not an option\n")
except ValueError:
print("Please pick the number corresponding to your choice\n")
def main():
main_excel_menu()
if __name__ == "__main__":
sys.exit(main())
-
1\$\begingroup\$ Voted to reopen, thanks. The title could be improved to simply "Excel sheet manipulation program" and comments about linting with ruff, flake8 and adhering to PEP-8 still apply. \$\endgroup\$ggorlen– ggorlen2024年07月05日 01:20:24 +00:00Commented Jul 5, 2024 at 1:20
-
1\$\begingroup\$ please don't point out pandas would do anything I did faster - ok, but... it would. Any insightful observation on Code Review is on-topic, and you should learn how to do this properly (Pandas even uses OpenPYXL as its Excel backend I believe). \$\endgroup\$Reinderien– Reinderien2024年07月05日 11:48:22 +00:00Commented Jul 5, 2024 at 11:48
-
\$\begingroup\$ @Reinderien Yes, answerers are always allowed to answer with any IO. However I was helping my sibling learn to code one time and was using all the best practices. But my sibling became overwhelmed with the amount to learn and didn't want help any more. You really shouldn't use the any or all rule as an excuse to not teach at the level of the student. \$\endgroup\$Peilonrayz– Peilonrayz ♦2024年07月05日 12:52:13 +00:00Commented Jul 5, 2024 at 12:52
2 Answers 2
Overview
You have done a good job partitioning the code into functions. I don't think there is too much code for one file in this case.
Input checking
The main menu includes options that assume the user has already opened an Excel file. If I choose options 2-4 right after I run the code, the code dies badly. It would be better to handle the input more gracefully. Either die with an informative error message or instruct the user what options are valid.
Unused code
The following code is unused and should be deleted:
def create_sheet(wb, sheetName, idx):
pass
To do comments
Remove all "to do" comments. Here are some examples
#is there a way to write 1 line code for this? can't find it
#ugly, will need to write function to format
#todo - add menu fucntion later
#have to pass wb, sheet, data as parameters probably
Comments should describe the existing code only. You should maintain a list of future features outside of the code.
Comments
Comments like the following are not needed and should be removed because they simply repeat what the code clearly does:
for row in range (0, maxRow): #loop through the rows
rowList =[] #create a list to hold the entire row
DRY
In the open_workbook
function, you repeat some code for the file name:
file = input()
wb = openpyxl.load_workbook(file + ".xlsx")
print(f"\nSuccessfully opened '{file}.xlsx' \n")
Here is one way to simplify it:
file = input() + '.xlsx'
wb = openpyxl.load_workbook(file)
print(f"\nSuccessfully opened '{file}'\n")
Naming
You used "snake case" in naming your functions, as recommended by the
PEP 8 style guide. The guide recommends
using snake case for variables as well. Consider changing maxRow
to
max_row
, for example.
Layout
There is some inconsistent use of whitespace (blank lines, space around operators, etc.). Consider using a program like black to automatically apply consistent formatting to your code.
Simpler
The following line:
for row in range(0, maxRow):
is simpler without the 0:
for row in range(maxRow):
Also:
while(loop == True):
is simpler as:
while loop:
Similarly,
if sheet is not None:
is simpler as:
if sheet:
Lint check
pylint identified a few more issues, such as missing documentation.
-
1\$\begingroup\$ "Maintain list of future features outside code" - I hadn't thought in this way before. For solo projects do you have particular examples of why you give this advice, from personal experience or advice you have received? \$\endgroup\$Greedo– Greedo2024年07月06日 19:53:46 +00:00Commented Jul 6, 2024 at 19:53
-
1\$\begingroup\$ @Greedo, a copy of your code is on GitHub, right? (If not, go create a repo now, so you'll always have an online backup in case a Bad Thing happens to your laptop.) Each time a new Feature Request occurs to you, go click on "Issues --> New Issue", or update an existing issue. You might choose to create one new Feature Branch per issue. Close out the issue once you've implemented the feature. By default Closed issues won't be displayed in the list, but you can always go back to them by querying for closed ones. \$\endgroup\$J_H– J_H2024年07月07日 17:15:29 +00:00Commented Jul 7, 2024 at 17:15
[print(option) for idx, option in enumerate(options) if idx != 0]
There's no need to enumerate
just to exclude the first entry. You can simply iterate options[1:]
.
Also it's not Pythonic to use list comprehensions only for side effects.
Suggested alternatives:
I would use a simple
for
loopfor option in options[1:]: print(option)
If you want to be fancier (which doesn't always mean better), you can print by unpacking
print(*options[1:], sep="\n")