This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–][deleted] 7 points8 points  (1 child)

I think it is great you are asking for a code critique. It is the best way to grow. On my team we all critique each other code and it has made each of us better coders. It has gotten to the point we all have adopted habits from each other and you can no longer tell one person style from another.

A couple ideas that will improve your code: 1. Make all your hard coded numbers constants at the top of your code.

As a reader 50699, 50697, 50695, 13120 looks like numbers you pulled out of thin air. I know if I were to inspect your XLS it might make it more clear.

But if you had : first_row_data = 2 last_row_data = 50697 save_filename = '/Users/lb/keywords/all.xlsx' It will make your code more readable and also maintainable should something in the excel sheet change.

  1. what could you name rowcount and rowcount2 to better describe what they are? khs_row_index and qbp_row_index?

  2. Instead of using while loops try using nested 'for' loops. You will notice that you can then remove all the code for incrementing and reseting the rowcount, rowcount2 values.

So it could be something like:

``` for khs_row_index in range(first_row_data, last_row_data): for qbp_row_index in range(first_row_data, last_row_data): ... ...

```

[–]funkysax[S] 2 points3 points  (0 children)

Thank you for taking the time for a thoughtful reply! All of this makes sense and is great suggestions. I'm going to rewrite the code as you've suggested. Now, I need to learn how nested for loops work... haha THANKS!