3
\$\begingroup\$

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())
 
toolic
14.5k5 gold badges29 silver badges203 bronze badges
asked Jul 4, 2024 at 21:15
\$\endgroup\$
3
  • 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\$ Commented 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\$ Commented 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\$ Commented Jul 5, 2024 at 12:52

2 Answers 2

3
\$\begingroup\$

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.

answered Jul 5, 2024 at 11:48
\$\endgroup\$
2
  • 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\$ Commented 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\$ Commented Jul 7, 2024 at 17:15
4
\$\begingroup\$
[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 loop

    for 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")
    
answered Jul 5, 2024 at 19:30
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.