Refactoring if statements and other code in python

Question

Whats the best way to refactor this code to clean it up:

1) Selects from the db adding a column for the percentage difference between two columns

2) Loops through the values of the columns

3) If the date is in the past

4) If the price is greater than 500 and the percentage difference is less than 1st argument set flag to 1

5) Else if the price is less than 500 and the percentage difference is less than 2nd argument set flag to 1

6) Otherwise keep the flag as 0

def calculateEmployeeSpend(read_cursor, flag_higher_amount, flag_lower_budget):

    read_cursor.execute("SELECT distinct b.employee_id, b.amount, "
                "s.spend, b.date, b.amount - s.spend as spend_left,  "
                "100.0*(b.amount - s.spend) / b.amount As PercentDiff FROM employee_budget_upload "
                "As b JOIN employee_budget_spent As s ON  b.employee_id = s.employee_id where b.amount != 0")

    for employee_id, amount, spend, date, spend_left, percent_diff in read_cursor:
        flag=0
        date_of_amount = dt.strptime(date, "%d/%m/%Y")
        if date_of_amount <= dt.now():
            if amount > 500 and percent_diff < int(flag_higher_amount):
                flag=1

            if amount < 500 and percent_diff < int(flag_lower_amount):
                flag=1

Edit:

I have changed the ifs to one if:

if amount > 500 and percent_diff < int(flag_higher_amount) or amount < 500 and percent_diff < int(flag_lower_amount):
                    flag=1

what is the point of the last check if flag becomes 1 anyway? — Ma0
– Ma0, Commented Mar 1, 2018 at 12:10
@Ev.Kounis its for checking the amount is less than 500 and the percentage difference is on the flag for the lower amount. Think that needs to be in a separate if rather than elif actually thanks. — Sam
– Sam, Commented Mar 1, 2018 at 12:12

Eran Moshe · Accepted Answer · 2018-03-01 12:12:58Z

1

Extract out the SQL command into a file.sql. Give the function either the path to the sql file or the text of the sql file.
Rename the flag to its purpose.
your if and elif both sets the flag to 1 so why the differences? Combine it to one condition
'500' should be a variable of the function.
Write a short description of what the function does in """ """. You can specify what every parameter is if you want.

answered Mar 1, 2018 at 12:12

Eran Moshe

3,2382 gold badges26 silver badges45 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Sam Over a year ago

There are many SQL commands in the full code and I read it's more efficient to have them in the same file as the class. The differences between the if's are that one checks for lower amount with a lower flag of percent diff and the other checks for higher amount with higher of percent diff.

Eran Moshe Over a year ago

Lets say the code is going to be operative and in order to change it you'll need to "PAY" a lot. changing the SQL from an outsource file is much easier and correct, putting your code in a much more flexible, maintainable and scale-able position.

Eran Moshe Over a year ago

You can also write a function that will handle your "execute" / "read_sql" assignment and add there error handling.

solstice333 · Accepted Answer · 2018-03-01 12:23:08Z

0

Since there seems to be several instances of keeping track of state, it might help in the long run to decouple the logic into classes and methods.

answered Mar 1, 2018 at 12:23

solstice333

3,8091 gold badge37 silver badges35 bronze badges

Comments

Recontemplator · Accepted Answer · 2018-03-01 12:46:35Z

First of all the definition of 'best' depend on your goal, like: readability, efficiency, performance and so on.

In many cases I wold prefer to solve task like this by reading the whole dataset in pandas DataFrame and utilise one (or set off) convenient and expressive pandas idioms. Or, by writing more sophisticated SQL statement which allow to solve the end to end task on database side.

As for common best practice for refactoring, I would recommend externalise the magic values like "500" or "%d/%m/%Y" to a constant or method parameter. Give a "flag" more self-spoken name. If case with amount exactly equal to 500 is purposely should lead to flag equal to zero, then it should be better explicitly reflected in comments. In order to avoid code duplication (flag=1) it is better to combine the if statements, like this:

if amount > 500 and percent_diff < int(flag_higher_amount) or \
   amount < 500 and percent_diff < int(flag_lower_amount):
    flag=1

You also can create a function with self-spoken name, and move whole condition inside such a function:

if is_percent_inside_amount_bounds(
     percent_diff, amount, flag_lower_amount, flag_higher_amount):
    flag = 1

or just

flag = is_percent_inside_amount_bounds(
     percent_diff, amount, flag_lower_amount, flag_higher_amount)

In case of amount equal exactly 500 could be interpreted like amount<=500 the condition could be transformed to more laconic:

flag = percent_diff < int(
    flag_lower_amount if amount>500 else flag_higher_amount)

but I would not recommend to use ternary operator in production code in cases like this, because it usually reduce readability.

Collectives™ on Stack Overflow

Refactoring if statements and other code in python

3 Answers 3

3 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related