Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #34338 > unrolled thread

Confused compare function :)

Back to article view | Back to comp.lang.python

Contents

  Confused compare function :) Anatoli Hristov <tolidtm@gmail.com> - 2012-12-05 23:50 +0100
    Re: Confused compare function :) Steven D'Aprano <steve+comp.lang.python@pearwood.info> - 2012-12-06 00:31 +0000
      Re: Confused compare function :) Anatoli Hristov <tolidtm@gmail.com> - 2012-12-06 10:36 +0100

#34338 — Confused compare function :)

Hi all,

I'm confused again with a compare update function. The problem is that
my function does not work at all and I don't get it where it comes
from.

in my DB I have total of 754 products. when I run the function is says:
Total updated: 754
Total not found with in the distributor: 747
I just don't get it, can you find my  mistake ?

Thanks in advance

def Change_price():
    total = 0
    tnf = 0
    for row in DB: # DB is mySQL DB, logically I get out 1 SKU and I
compare it with next loop
        isku = row["sku"]
        isku = isku.lower()
        iprice = row["price"]
        iprice = int(iprice)
        found = 0
        try:
            for x in PRICELIST:# here is my next loop in a CSV file
which is allready in a list PRICELIST
                try:
                    dprice = x[6]
                    dprice = dprice.replace(",",".") # As in the
PRICELIST the prices are with commas I replace the comma as python
request it
                    dprice = float(dprice)
                    newprice = round(dprice)*1.10
                    dsku = x[4]
                    dsku = dsku.lower()
                    stock = int(x[7])
                    if isku == dsku and newprice < int(iprice):# If
found the coresponded SKU and the price is higher than the one in the
CSV I update the price
                        print dsku, x[6], dprice, newprice
                        Update_SQL(newprice, isku)# goes to the SQL Update
                        print isku, newprice
                        if isku == dsku:# Just a check to see if it works
                            print "Found %s" %dsku
                            found = 1
                        else:
                            found = 0
                except IndexError:
                    pass
                except ValueError:
                    pass
                except TypeError:
                    pass
        except IndexError:
            pass
        if found == 1:
            print "%s This is match" % isku
        if found == 0:
            print "%s Not found" % isku
            tnf = tnf +1
        total = total +1
    print "Total updated: %s" % total
    print"Total not found with in the distributor: %s" % tnf

[toc] | [next] | [standalone]

#34348

On Wed, 05 Dec 2012 23:50:49 +0100, Anatoli Hristov wrote:


> def Change_price():

Misleading function name. What price does it change?


>     total = 0
>     tnf = 0

"tnf"? Does that mean something?


>     for row in DB:  # DB is mySQL DB, logically I get out 
>                     # 1 SKU and I compare it with next loop

Use of global variables, yuck. What happens if some day you need two 
databases at the same time?


>         isku = row["sku"]
>         isku = isku.lower()

Hungarian Notation? This is better written as:

sku = row["sku"].lower()


>         iprice = row["price"]
>         iprice = int(iprice)

And likewise price = int(row["price"]). Or better still, "oldprice", or 
"price_in_database", or something that actually describes what it is.


>         found = 0

found = False


>         try:
>             for x in PRICELIST:  # here is my next loop in a CSV file
>                                  # which is allready in a list PRICELIST
>                 try:
>                     dprice = x[6]

"dprice"? D-for-database price? But this is the price from the CSV file, 
not from the database. Another misleading name, leading to confusion.


>                     dprice = dprice.replace(",",".")
>                     # As in the PRICELIST the prices are with 
>                     # commas I replace the comma as python request it
>                     dprice = float(dprice)
>                     newprice = round(dprice)*1.10
>                     dsku = x[4]
>                     dsku = dsku.lower()

And again, what's "dsku" mean? Database-SKU? But it's the CSV SKU.


>                     stock = int(x[7])

I don't believe that this is used at all. Get rid of it.


>                     if isku == dsku and newprice < int(iprice):
>                     # If found the coresponded SKU and the price is
>                     # higher than the one in the CSV I update the price

I think your logic is wrong here. You aren't comparing the price in the 
CSV here at all. You compare two prices, neither of which is the price in 
the CSV file:

newprice = round(price in CSV) * 1.10
iprice = price from the database

(which is already an int, no need to call int *again* -- if you're going 
to use Hungarian Notation, pay attention to it!)

So you have THREE prices, not two, and it isn't clear which ones you are 
*supposed* to compare. Either the code is wrong, or the comment is wrong, 
or possibly both.

iprice = price from the database
dprice = price from the CSV
newprice = calculated new price

I'm going to GUESS that you actually want to compare the new price with 
the price in the database.

if newprice > iprice:  # horrible name! no wonder you are confused
    # Update the database with the new price


>                         print dsku, x[6], dprice, newprice
>                         Update_SQL(newprice, isku)
>                         # goes to the SQL Update

Really? Gosh, without the comment, how would anyone know that Update_SQL 
updates the SQL? :-P

Seriously, the comment is redundant. Get rid of it.


>                         print isku, newprice
>                         if isku == dsku:
>                         # Just a check to see if it works
>                             print "Found %s" %dsku
>                             found = 1
>                         else:
>                             found = 0

found = True or False is better.

But this code cannot do anything but print Found, since above you already 
tested that isku == dsku. So this check is pointless.

The reason is, your code does this:

if isku == dsku and (something else):
    # Inside this block, isku MUST equal dsku
    blah blah blah
    if isku == dsku:
        print "Found"
        found = 1
    else:
        # But this cannot possibly happen
        print "not found"
        found = 0


>                 except IndexError:
>                     pass
>                 except ValueError:
>                     pass
>                 except TypeError:
>                     pass

Why are you hiding errors? You should not hide errors unnecessarily, that 
means there are bugs in either the CSV or your code, you should fix the 
bugs.

However, if you really must, then you can replace all of those with:

              except (IndexError, ValueError, TypeError): 
                  pass


>         except IndexError:
>             pass

And hiding more errors?


>         if found == 1:
>             print "%s This is match" % isku
>         if found == 0:
>             print "%s Not found" % isku
>             tnf = tnf +1
>         total = total +1

Better to write this as:

if found:
   print "%s This is match" % isku
else:
   print "%s Not found" % isku
   tnf = tnf + 1  # What does this mean?
total += 1


>     print "Total updated: %s" % total

That's wrong. total is *not* the number updated. It is the total, updated 
or not updated. This should say:

print "Total records inspected: %s" % total


>     print"Total not found with in the distributor: %s" % tnf

Ah-ha! It means "Total Not Found"! I shouldn't have to read all the way 
to the end of the code to discover this. Instead of "tnf", you should 
count the total number of SKUs, and count how many times you call 
Update_SQL. Then you can report:

Total records inspected: 754
Total records updated: 392

(or whatever the values are).



Simplify and clean up your code, and then it will be easier to find and 
fix the problems in it. Good luck!



-- 
Steven

[toc] | [prev] | [next] | [standalone]

#34371

On Thu, Dec 6, 2012 at 1:31 AM, Steven D'Aprano
<steve+comp.lang.python@pearwood.info> wrote:
> On Wed, 05 Dec 2012 23:50:49 +0100, Anatoli Hristov wrote:
>
>
>> def Change_price():
>
> Misleading function name. What price does it change?
>
>
>>     total = 0
>>     tnf = 0
>
> "tnf"? Does that mean something?
>
>
>>     for row in DB:  # DB is mySQL DB, logically I get out
>>                     # 1 SKU and I compare it with next loop
>
> Use of global variables, yuck. What happens if some day you need two
> databases at the same time?
>
>
>>         isku = row["sku"]
>>         isku = isku.lower()
>
> Hungarian Notation? This is better written as:
>
> sku = row["sku"].lower()
>
>
>>         iprice = row["price"]
>>         iprice = int(iprice)
>
> And likewise price = int(row["price"]). Or better still, "oldprice", or
> "price_in_database", or something that actually describes what it is.
>
>
>>         found = 0
>
> found = False
>
>
>>         try:
>>             for x in PRICELIST:  # here is my next loop in a CSV file
>>                                  # which is allready in a list PRICELIST
>>                 try:
>>                     dprice = x[6]
>
> "dprice"? D-for-database price? But this is the price from the CSV file,
> not from the database. Another misleading name, leading to confusion.
>
>
>>                     dprice = dprice.replace(",",".")
>>                     # As in the PRICELIST the prices are with
>>                     # commas I replace the comma as python request it
>>                     dprice = float(dprice)
>>                     newprice = round(dprice)*1.10
>>                     dsku = x[4]
>>                     dsku = dsku.lower()
>
> And again, what's "dsku" mean? Database-SKU? But it's the CSV SKU.
>
>
>>                     stock = int(x[7])
>
> I don't believe that this is used at all. Get rid of it.
>
>
>>                     if isku == dsku and newprice < int(iprice):
>>                     # If found the coresponded SKU and the price is
>>                     # higher than the one in the CSV I update the price
>
> I think your logic is wrong here. You aren't comparing the price in the
> CSV here at all. You compare two prices, neither of which is the price in
> the CSV file:
>
> newprice = round(price in CSV) * 1.10
> iprice = price from the database
>
> (which is already an int, no need to call int *again* -- if you're going
> to use Hungarian Notation, pay attention to it!)
>
> So you have THREE prices, not two, and it isn't clear which ones you are
> *supposed* to compare. Either the code is wrong, or the comment is wrong,
> or possibly both.
>
> iprice = price from the database
> dprice = price from the CSV
> newprice = calculated new price
>
> I'm going to GUESS that you actually want to compare the new price with
> the price in the database.
>
> if newprice > iprice:  # horrible name! no wonder you are confused
>     # Update the database with the new price
>
>
>>                         print dsku, x[6], dprice, newprice
>>                         Update_SQL(newprice, isku)
>>                         # goes to the SQL Update
>
> Really? Gosh, without the comment, how would anyone know that Update_SQL
> updates the SQL? :-P
>
> Seriously, the comment is redundant. Get rid of it.
>
>
>>                         print isku, newprice
>>                         if isku == dsku:
>>                         # Just a check to see if it works
>>                             print "Found %s" %dsku
>>                             found = 1
>>                         else:
>>                             found = 0
>
> found = True or False is better.
>
> But this code cannot do anything but print Found, since above you already
> tested that isku == dsku. So this check is pointless.
>
> The reason is, your code does this:
>
> if isku == dsku and (something else):
>     # Inside this block, isku MUST equal dsku
>     blah blah blah
>     if isku == dsku:
>         print "Found"
>         found = 1
>     else:
>         # But this cannot possibly happen
>         print "not found"
>         found = 0
>
>
>>                 except IndexError:
>>                     pass
>>                 except ValueError:
>>                     pass
>>                 except TypeError:
>>                     pass
>
> Why are you hiding errors? You should not hide errors unnecessarily, that
> means there are bugs in either the CSV or your code, you should fix the
> bugs.
>
> However, if you really must, then you can replace all of those with:
>
>               except (IndexError, ValueError, TypeError):
>                   pass
>
>
>>         except IndexError:
>>             pass
>
> And hiding more errors?
>
>
>>         if found == 1:
>>             print "%s This is match" % isku
>>         if found == 0:
>>             print "%s Not found" % isku
>>             tnf = tnf +1
>>         total = total +1
>
> Better to write this as:
>
> if found:
>    print "%s This is match" % isku
> else:
>    print "%s Not found" % isku
>    tnf = tnf + 1  # What does this mean?
> total += 1
>
>
>>     print "Total updated: %s" % total
>
> That's wrong. total is *not* the number updated. It is the total, updated
> or not updated. This should say:
>
> print "Total records inspected: %s" % total
>
>
>>     print"Total not found with in the distributor: %s" % tnf
>
> Ah-ha! It means "Total Not Found"! I shouldn't have to read all the way
> to the end of the code to discover this. Instead of "tnf", you should
> count the total number of SKUs, and count how many times you call
> Update_SQL. Then you can report:
>
> Total records inspected: 754
> Total records updated: 392
>
> (or whatever the values are).
>
>
>
> Simplify and clean up your code, and then it will be easier to find and
> fix the problems in it. Good luck!
>
>
>
> --
> Steven
> --
> http://mail.python.org/mailman/listinfo/python-list

Thank you Steven for your help. Now I renamed all the variables and it
seems working I didn't found the mistake, but it seems working :) Here
is the new code:

def Change_price(): # Changes the price in the DB if the price in the
CSV is changed
    TotalUpdated = 0 # Counter for total updated
    TotalNotFound = 0 # Counter for total not found
    for row in PRODUCTSDB:
        db_sku = row["sku"].lower()
        db_price = int(row["price"])
        found = False
        try:
            for x in pricelist:
                try:
                    csv_price = x[6]
                    csv_price = csv_price.replace(",",".")
                    csv_price = float(csv_price)
                    csv_new_price = round(csv_price)*1.10
                    csv_sku = x[4].lower()
                    csv_stock = int(x[7]) # I used this as normally I
used  stock in the condition
                    if len(db_sku) != 0 and db_sku == csv_sku and
csv_new_price < db_price and csv_stock > 0:
                        print db_sku, csv_price, db_price, csv_new_price
                        Update_SQL(csv_new_price, db_sku)
                        found = True
                        TotalUpdated += 1
                    else:
                        found = False

                except IndexError: # I have a lot of index error in
the CSV (empty fields) and the loop gives "index error" I  don't care
about them
                    pass
                except ValueError:
                    pass
                except TypeError:
                    pass
        except IndexError:
            pass
        if found:
            TotalNotFound += 1
            print "%s This is match" % db_sku
        else:
            print "%s Not found" % db_sku
            TotalNotFound += 1

    print "Total updated: %s" % TotalUpdated
    print"Total not found with in the distributor: %s" % TotalNotFound

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web