Confused compare function :)

[Steven D'Aprano]

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