Confused compare function :)

[Anatoli Hristov]

On Thu, Dec 6, 2012 at 1:31 AM, Steven D'Aprano
<steve+comp.lang.python at 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