6
\$\begingroup\$

I have written the following function which works as expected but I still see there is some room for improving its readability

def get_claims_to_search():
    claims = []
    database_priority = DatabasePriority.objects.first()

    low_priority_claims_databases = ClaimsDatabase.objects.filter(
        deleted=False, priority="low"
    )
    normal_priority_claims_databases = ClaimsDatabase.objects.filter(
        deleted=False, priority="normal"
    )
    high_priority_claims_databases = ClaimsDatabase.objects.filter(
        deleted=False, priority="high"
    )

    low_priority_count = database_priority.low
    normal_priority_count = database_priority.normal
    high_priority_count = database_priority.high

    if not low_priority_claims_databases.count():
        low_priority_count = 0
        if normal_priority_claims_databases.count():
            normal_priority_count += int(
                database_priority.normal
                / (database_priority.normal + database_priority.high)
                * database_priority.low
            )
        if high_priority_claims_databases.count():
            high_priority_count += int(
                database_priority.high
                / (database_priority.normal + database_priority.high)
                * database_priority.low
            )
    if not normal_priority_claims_databases.count():
        normal_priority_count = 0
        if low_priority_claims_databases.count():
            low_priority_count += int(
                database_priority.low
                / (database_priority.low + database_priority.high)
                * database_priority.normal
            )
        if high_priority_claims_databases.count():
            high_priority_count += int(
                database_priority.high
                / (database_priority.low + database_priority.high)
                * database_priority.normal
            )

    if not high_priority_claims_databases.count():
        high_priority_count = 0
        if low_priority_claims_databases.count():
            low_priority_count += int(
                database_priority.low
                / (database_priority.low + database_priority.high)
                * database_priority.normal
            )
        if normal_priority_claims_databases.count():
            normal_priority_count += int(
                database_priority.normal
                / (database_priority.normal + database_priority.high)
                * database_priority.high
            )

    priority_databases = {
        "low": low_priority_claims_databases,
        "normal": normal_priority_claims_databases,
        "high": high_priority_claims_databases,
    }
    priority_count = {
        "low": low_priority_count,
        "normal": normal_priority_count,
        "high": high_priority_count,
    }

    for priority in priority_count:
        if priority_count[priority]:
            priority_count[priority] = int(
                (
                    (priority_count[priority] / 100)
                    / priority_databases[priority].count()
                )
                * settings.DEBUNKBOT_SEARCHEABLE_CLAIMS_COUNT
            )

            for claim_database in priority_databases[priority]:
                claims.append(
                    claim_database.claims.filter(processed=False, rating=False).values(
                        "claim_first_appearance"
                    )[: priority_count[priority]]
                )
    return claims

any suggestions on how I can improve/rewrite it?

Improve this question
\$\endgroup\$
1

1 Answer 1

Reset to default
5
\$\begingroup\$

The function seems to be inconsistent; I don't know what is it intended to do, so if you say it works as expected, probably you can lose some functionality on unnecessary "beautification" of the code.

What inconsistency am I talking about: the central (and the longest) part of the code transforms 6 input values, *_priority_claims_databases.count() and database_priority.* (where * is low, normal and high) into 3 output values, *_priority_count, using something that seems to be one formula. I'll use short names dl for database_priority.low and pl for low_priority_count etc. to make formulas more readable. So we have (omitting conditions):

pn += int( dn / (dn + dh) * dl )
ph += int( dh / (dn + dh) * dl )
pl += int( dl / (dl + dh) * dn )
ph += int( dh / (dl + dh) * dn )
pl += int( dl / (dl + dh) * dn ) #<--!!!!!
pn += int( dn / (dn + dh) * dh )

At this point it is clear that first two lines (in if low_priority_claims_databases.count() == 0:) end with dl, second two lines - with dn, 6th line - with dh, which corresponds with condition, but 5th line stands out. If we change dh+dh into s-dl (where s=dh+dn+dl), the problem will get even worse:

pn += int( dn / (s - dl) * dl )
ph += int( dh / (s - dl) * dl )
pl += int( dl / (s - dn) * dn )
ph += int( dh / (s - dn) * dn )
pl += int( dl / (s - dn) * dn ) #<--!!!!!
pn += int( dn / (s - dl) * dh ) #<--!!!!!

Now I see two lines are out of pattern. Sorry, can't help it here without a description.

Maybe last two lines should be

pl += int( dl / (s - dh) * dh )
pn += int( dn / (s - dh) * dh )

i.e.

low_priority_count  += int( 
    database_priority.low 
    / (database_priority.low + database_priority.normal) 
    * database_priority.high )
normal_priority_count += int( 
    database_priority.normal 
    / (database_priority.low + database_priority.normal) 
    * database_priority.high )

If so, fix the code first.

Still we can do something with the last portion of the code. Let's create dicts with keys "low", "normal" and "high" (or list and constants) instead of groups of variables; so we'll have

if priority_count["low"]:
    priority_count["low"] = (
        priority_count["low"] // priority_claims_databases["low"].count()
    ) * settings.DEBUNKBOT_SEARCHEABLE_CLAIMS_COUNT
    for claim_database in priority_claims_databases["low"]:
        claims.append(
            claim_database.claims.filter(processed=False, rating=False).values(
                "claim_first_appearance"
            )[:priority_count["low"]]

and two other code chunks with the only change "low" to "normal" and "high". This can be changed into the loop:

for priority in ["low","normal","high"]:
    if priority_count[priority]:
        priority_count[priority] = (
            priority_count[priority] // priority_claims_databases[priority].count()
        ) * settings.DEBUNKBOT_SEARCHEABLE_CLAIMS_COUNT
        for claim_database in priority_claims_databases[priority]:
            claims.append(
                claim_database.claims.filter(processed=False, rating=False).values(
                    "claim_first_appearance"
                )[:priority_count[priority]]

This is at least as readable as your code but almost 3 times shorter.

One more question: what happens if all *_priority_claims_databases.count() are greater then zero? Is it intended that the code return empty list?

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for this. For some more details, the function is supposed to build a list of claims based on different priorities. I have 3 database types (low, normal and high) and every level has a specific percentage e.g low might be 15%, normal 35% e.t.c. Incase we don't have a database with low priority, we want to re distribute the 15% to both Normal and High i.e Normal will now be 35+ (35/(35+50)*15) where 50 is the %age of the High priority. If all counts are greater than 0 then all if *_priority_count: will be true and thus not an empty list will be returned. \$\endgroup\$
    – E_K
    Aug 19, 2021 at 1:08
  • \$\begingroup\$ First, we make *_priority_count = 0. Next, there are three ifs for each *_priority_claims_databases.count() == 0. If all three will be False, *_priority_count will stay 0 and next three ifs on *_priority_count would not execute and claims will be empty. \$\endgroup\$
    – Pavlo Slavynskyy
    Aug 19, 2021 at 5:23
  • \$\begingroup\$ The case we don't have a database with low priority, i.e. low_priority_claims_databases.count() == 0, is the first two formulas. Why 5th and 6th are different? Also add this into the description. \$\endgroup\$
    – Pavlo Slavynskyy
    Aug 19, 2021 at 5:25
  • \$\begingroup\$ I've updated the code to include some of your suggestions and it's looking good so far. \$\endgroup\$
    – E_K
    Aug 19, 2021 at 7:40

Your Answer

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

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