5
\$\begingroup\$

I am working with an SQLite database. Is this code written the best way it can be and correct? The code:

  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

from datetime import datetime as dt

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) or amount < 500 and percent_diff < int(flag_lower_budget):
                flag=1

        write_cursor.execute('INSERT OR REPLACE INTO employee_overall_spend VALUES (?,?,?,?,?,?,?,?)',
                         (employee_id, amount, spend, date, spend_left, percent_diff, dt.now().strftime("%d/%m/%Y"), flag)) 
\$\endgroup\$
  • \$\begingroup\$ I'm unsure why you care about the flag variable. The code that you've posted doesn't use it... Please add more code so we can actually comment on what your code is doing. \$\endgroup\$ – Peilonrayz Mar 1 '18 at 12:17
  • \$\begingroup\$ @Peilonrayz okay thanks will add it it is needed. \$\endgroup\$ – Sam Mar 1 '18 at 12:25
  • \$\begingroup\$ @Peilonrayz i've added it basically takes the values and inserts them into a new db \$\endgroup\$ – Sam Mar 1 '18 at 12:28
  • 1
    \$\begingroup\$ I have some concerns with how far write_cursor is indented. Please post your actual code. \$\endgroup\$ – Peilonrayz Mar 1 '18 at 12:30
  • 1
    \$\begingroup\$ I've attempted to summarize what this code accomplishes. If I have misinterpreted it, please correct the title. \$\endgroup\$ – 200_success Mar 1 '18 at 13:07
6
\$\begingroup\$

I would say being a small code, you can change a couple things to make it a bit more readable

Specially the part with the conditionals, is a bit hard to follow

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) or amount < 500 and percent_diff < int(flag_lower_budget):
        flag=1

You could try extracting all into smaller functions:

is_past_amount_date = dt.strptime(date, "%d/%m/%Y") <= dt.now()
should_flag_amount = (amount > 500 and percent_diff < int(flag_higher_amount)) or (amount < 500 and percent_diff < int(flag_lower_budget))

if is_past_amount_date and should_flag_amount:
    flag = 1

As well, try not to use date as a variable number since it shadows the date name from datetime, it can have some undesired effects later

To end with it, the execution of the cursor can take place outside the function, so as a first parameter you receive employee_budget_cursors (which is the result of executing the select Then, you iterate with it in your function directly

Also easier for testing

\$\endgroup\$
  • \$\begingroup\$ Would be interesting if the = in the <= dt.now() triggered. \$\endgroup\$ – Ev. Kounis Mar 1 '18 at 13:44
  • \$\begingroup\$ @Ev.Kounis what do you mean? \$\endgroup\$ – Sam Mar 1 '18 at 13:59
  • \$\begingroup\$ @Sam try dt.strptime('01/03/2018', "%d/%m/%Y")==dt.now(). This can be True only and precisely at midnight (00:00:00.000000). This amount of precision would be extraordinary \$\endgroup\$ – Ev. Kounis Mar 1 '18 at 14:08
  • \$\begingroup\$ @Ev.Kounis but that works as expected right as midnight tonight would return true for '01/03/2018'? \$\endgroup\$ – Sam Mar 1 '18 at 14:18
  • \$\begingroup\$ @Sam Yeah it does, but not checking for equality would not make you miss much unless you are extremely unlucky. That is my point. \$\endgroup\$ – Ev. Kounis Mar 1 '18 at 14:22
8
\$\begingroup\$

There's two ways you can go about this. Stick with your current way, or use SQL more.

Current way

  • I'd change your for loop to assign to one value.
  • I'd make a function that tells you if the values have the flag.
  • I'd use *value to tuple unpack into the write_cursor tuple.

And so would get something like:

from datetime import datetime as dt


def get_flag(employee_spend, higher, lower):
    _, amount, _, date, _, percent_diff = employee_spend
    return int(
        dt.strptime(date, "%d/%m/%Y") <= dt.now()
        and (
            amount > 500
            and percent_diff < int(higher)
            or amount < 500
            and percent_diff < int(lower)
        )
    )


def calculateEmployeeSpend(read_cursor, flag_higher_amount, flag_lower_budget):
    higher, lower = int(flag_higher_amount), int(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_spend in read_cursor:
        flag = get_flag(employee_spend, higher, lower)
        write_cursor.execute(
            'INSERT OR REPLACE INTO employee_overall_spend VALUES (?,?,?,?,?,?,?,?)',
            (*employee_spend, dt.now().strftime("%d/%m/%Y"), flag)
        )

Use SQL

I'm don't know SQL, but you could use something like:

CALCULATE_EMPLOYEE_SPEND_READ = '''\
DECLARE @curr_date DATETIME;
SET @curr_date = GETDATE();

SELECT
    e.employee_id,
    e.amount,
    e.spend,
    e.date,
    e.spend_left,
    e.percent_diff,
    @curr_date as curr_date,
    case when
        e.date <= @curr_date
        and (
            e.amount > 500
            and e.percent_diff < ?
            or e.amount < 500
            and e.percent_diff < ?
        )
        then 1
        else 0 
    end
    as flag
FROM (
    SELECT distinct
        b.employee_id,
        b.amount,
        s.spend,
        b.date,
        b.amount - s.spend as spend_left,
        100.0 - 100.0*s.spend / b.amount as percent_diff
    FROM employee_budget_upload As b
    JOIN employee_budget_spent As s
    ON  b.employee_id = s.employee_id
    WHERE b.amount != 0
) as e
'''
CALCULATE_EMPLOYEE_SPEND_WRITE = 'INSERT OR REPLACE INTO employee_overall_spend VALUES (?,?,?,?,?,?,?,?)'


def calculateEmployeeSpend(read_cursor, higher, lower):
    read_cursor.execute(CALCULATE_EMPLOYEE_SPEND_READ, (int(higher), int(lower)))
    for employee_spend in read_cursor:
        write_cursor.execute(CALCULATE_EMPLOYEE_SPEND_WRITE, employee_spend)
\$\endgroup\$
  • \$\begingroup\$ I like your answer but I would need to change so much for the SQL way. The method for the flagged value is a good idea but the way I have written it within classes for me it just cleans it up using the below. \$\endgroup\$ – Sam Mar 1 '18 at 14:48
  • 3
    \$\begingroup\$ @Sam I don't know anything about your environment, as you wouldn't tell us. Doing everything in SQL IMO is better, as then you can just use stored procedures. But the choice is yours. \$\endgroup\$ – Peilonrayz Mar 1 '18 at 14:53
  • \$\begingroup\$ Yes sorry I understand but I just have a small sqlite db I do like your answer. I wouldn't need the stored procedures though. I would mark it as right but I used the other guys code to refactor mine. \$\endgroup\$ – Sam Mar 1 '18 at 15:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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