4
\$\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())
    
\$\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\$
    – ggorlen
    Commented Jul 5 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
    Commented Jul 5 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
    Commented Jul 5 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.

\$\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\$
    – Greedo
    Commented Jul 6 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
    Commented Jul 7 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")
    
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.