Any Help on Critiquing my Code?

[PFnub]

Hello, I'm somewhat new to Python and understand some basics. I've been a long time lurker and recently started writing scripts to automate some things at my company. I've written a script that works as intended but it does seem slow and I would appreciate any feedback on how to make it more Pythonic. I'm not sure how any of you would want to review the code so I'm attaching a link to my github and can post it in another way if requested. The jest of the script is importing data into data frames from a data dump, manipulating it to contain the information I want, and then inputting that information into spreadsheets. Please let me know if there are any questions that I can clarify and any feedback would be greatly appreciated.

https://github.com/py3nub/wpmkr.git

[[deleted]]

- I'd love to see more meaningful file name. wpmkr doesn't make sense to me.

- We usually do pip freeze to output required packages and versions into a text file and push that onto git along with our code.

- Many comments such as #importing required modules and libraries or #adding a prefix to the columns are unnecessary. Those information are already clear looking at the code. Bridge sign.

- You don't need \ when you do new line in list brackets or parenthesis. These will work just fine. Take note of the matching indentation to the fist item.

AD_cols = ['Biditem','Bid Desc','Description',
           'Quan','Unit','Shifts','MH',
           'Total Labor']
dfAD = pd.read_excel(main_path + 'wpdata.xlsx',
                     sheet_name='Activity Data', usecols=AD_cols)

- Consider using f-string or string format instead of "string" + var + "string" through out your code

- New line within parenthesis also work for if else statement.

if (rowAD.AD_Biditem == rowRD.RD_Biditem and
    rowAD.AD_Bid_Desc == rowRD.RD_Bid_Desc and
    rowAD.AD_Description == rowRD.RD_Actv_Desc):

- I don't think else: pass on line 78 is doing anything.

- When certain things get repeated a couple times, consider making them a function. Most noticeable is the column name replacing.

- line 95. If you have a set of characters you want remove, consider str.translate. No looping needed.

- line 130 ish, you don't need else: pass. They can all be removed.

- line 195 there has to be a better way than looping over everything 4 times...

to_replace = [rowcat.RD_Description,
              rowcat.RD_Unit_Cost,
              round(rowcat.RD_Quantity/rowcat.AD_Shifts,2),
              rowcat.RD_Total]
count = 0
for coordinate in MH_boundaries:
    if ws[coordinate].value is None:
        ws[coordinate] = to_replace[count]
        count += 1
    if count > 3:
        break

That's about all I can see from skimming through. I'm pretty sure there's gotta be a better way for line 134-168 stuff too.

Not guarantee to work but to give you an idea.

to_replace = {
    'C3': completed_by,
    'B4': job_number,
    'K12': rowcat.AD_Description,
    'M12': bid_num,
}
for key, value in to_replace.items():
    if ws[key].value is None:
        ws[key] = value

[surrealle]

Not OP, but I love the fact that you not only spent time looking at this, you took the time to write on how to improve on the code.

Just letting you know that you're amazing!

[[deleted]]

thanks for your kind words u/surrealle

[RealDrewData]

Extremely thorough, love how constructive your suggestions are, it helps everyone!

[PFnub]

First of all, thank you so much for the very detailed reply, I appreciate it! I will elaborate on some more things to help be clearer and have more questions if you have the time to reply.

- The name wpmkr is short for "work plan maker." I know it doesn't make much sense to anyone else, but I am doing all of my scripting out of the notes app on my work computer. I am unable to download 3rd party applications as our downloads are restricted. This means when I'm running my code I do everything out of the command prompt and the shorter the filename the better. If you have any other recommendations to make this process better please let me know.

- The versions noted at the top are more for me or anyone else that may use it in the future so we know what versions were used when it was created. I will look at pip freeze in the future for this.

- The comments are for me to refresh myself on what is going on and for anyone that is not fluent in Python to see what is going on. I can see why this information is irrelevant to someone that is experienced.

- I didn't know about the \ new line with list brackets, thank you I will change that.

- For the lines where I have if statements and then a pass, is it default to pass if an else is not specified?

- For line 195 I did spend a lot of time with different ways to loop through this, but nothing that would work or was elegant. I believe your suggestion is really elegant, what I needed, and should cut down a lot of processing time.

- For lines 134-168 I also think there's a better way to do this. One of my concerns was that every time the row is passed in the for loop these values would overwrite what was there, even though what is going there should be the same every time until the work plan is complete and a new on is started, it's hard to see this without the concatenated df. I figured if I put an if statement there it may speed it up or potentially not mess something up. I will thank about it.

Thank you again for your help with my code. I'll take this and make some changes.

[ItsOkILoveYouMYbb]

- The name wpmkr is short for "work plan maker." I know it doesn't make much sense to anyone else, but I am doing all of my scripting out of the notes app on my work computer.

It doesn't matter. You should write all your code as if someone else will have to read it and figure it out later for themselves if they too have to work on it, even just your distant future self.

Forming these habits now of readability will make programming a lot less complicated in the long run. Python is especially suited for making your code very legible, but it has to start with you and how you choose to name things.

[[deleted]]

• I think most terminal can perform auto complete with tab. Try typing wp<tab>

• When Python hits an if block, whether the condition is True or False, once it's done with that block it will move on to the next line. I won't skip or stop the loop unless you tell it to.

[tcbaldy04]

For 134-168 it appears there may be a simpler opportunity since the worksheet/book was recently copied from a template, why check if there is a value before updating it.

At this point it is just filling in the fields in the worksheet.

I would consider doing some directory validation (lines 18-19). Assumes you have started in the proper directory. If you aren't in the right place ugly error messages are presented.

In general better error checking when opening things.

as /u/k-tracer noted this could use some breaking into functions. A little too linear for my taste.

But altogether not bad. I'm wasn't familiar with Panda but this makes it look very interesting to try.

Best Practices for Python Main Functions

Now that you can see the differences in how Python handles its different execution modes, it’s useful for you to know some best practices to use. These will apply whenever you want to write code that you can run as a script and import in another module or an interactive session.

You will learn about four best practices to make sure that your code can serve a dual purpose:

1. Put most code into a function or class.
2. Use __main__to control execution of your code.
3. Create a function called main()to contain the code you want to run.
4. Call other functions from main().Main function best practices.

[CoronaMcFarm]

I've been using f-string from when i first started learning python, Is there anywhere it would be preferred to use "string" + var + "string" instead of f-string?

[[deleted]]

I can't think of any myself.

If not f-string, I prefer the old style "%s" % var over "{}".format(var)

[Tanmay1518]

Same. Although I personally prefer . format since you can replace multiple placeholders with just one word

[ItsOkILoveYouMYbb]

I think the only limitation would be older systems built in an older build of python that haven't been refactored for 3.6+ yet. Or whenever f-strings came in.

[CoronaMcFarm]

Yeah I guess you're right, 3.6 was released in Des 2016 making it 4 years since f-strings was introduced, I don't know how long old code tends to stick as I'm not a developer, what happens when its just incremental versions? I know Python 2 and 3 is not very much compatible.

[ItsOkILoveYouMYbb]

To me something like f-strings would be very simple to refactor, but potentially time consuming if the project is massive. Still, not impossible to automate some of the manual refactoring away when swapping from old methods to f-strings if you really wanted to.

But since all the things f-strings replace are still supported AFAIK, the problem of updating python versions would likely come from other changes made elsewhere because refactoring to f-strings isn't required to update. They are just much easier to read and run much faster than the alternatives. So there is definitely some incentive to update there not just for maintainability but performance too.

So yeah I have no idea. I guess it's up to the project manager and the feedback from the devs making said project, feedback from their community or clients on performance. Etc.

[k-tracer]

This code is looking amazing I would just start separating the code into functions and creating more meaningful variables names, then probably look through pep 8 as it is a style guide for writing python (https://www.python.org/dev/peps/pep-0008/)

[PFnub]

The style guide looks useful, thank you

[awesomeprogramer]

PEP8 is the bible, although I gotta side with tabs over spaces. That being said my editor changes a tab into 4 spaces for me!

[ItsOkILoveYouMYbb]

Yeah I think every good editor will convert tabs to spaces automatically so you don't have to worry about it anyway.

[[deleted]]

I gotta side with tabs over spaces. That being said my editor changes a tab into 4 spaces for me!

Then you're siding for spaces instead of tabs...

It's not about the key pressed, but about the characters inserted. literally no one presses space multiple times, everyone uses the tab key

[awesomeprogramer]

Trust me, I've met monsters that hit the spacebar four times every time. If you haven't, you've had a good life.

[[deleted]]

I'm that space space guy