Advice Criticism on Python App

[MRAB]

Jimbo wrote:
> I have made a Python App(really script) that will check a stocks
> current values from a website & save that data to a SQLite 3 database.
> 
> I am looking for any suggestions & criticisms on what I should do
> better or anything at all but mainly in these areas:
> [QUOTE]
> - Correct Python Layout of code
> - Correct error checking: Am I catching all my errors or are my
> exceptions not specific enough? Should I be using more try excepts
> inside my functions?
> - Are there any areas where huge errors, bugs etc could occur that I
> have not compensated for?
> - I am also looking for suggestions on how to write a function better,
> so if you think that function is really bad & should be rewritten
> completely or something I would really like to hear it.
> - Anything else that you see
> - Is python meant to be used in the way I have used it? To make a
> 'semi detailed' app?[/QUOTE]
> 
> Any advice criticism would be really helpful.
> 
> App:
> [CODE]"""
>  *Stock Data Builder*
>    Algorithm:
>       - Search website for stock
>       - Get website HTML source code
>       - Search code for target stock data(price,dividends,etc..)
>       - Add data to text file
> """
> 
> import sys
> import os
> import sqlite3
> import datetime
> import time
> import urllib2
> 
> ### Global Variables ###
> menu = "***Stock Program*** \n\n1. Add a Stock to track \n2. Get
> Todays Tracking Data \n3. Exit \nEnter decision: "
> target = '<th scope="row" class="row"><a href="/asx/research/
> companyInfo.do?by=asxCode&asxCode=%s">%s</a>'
> ASXurl = 'http://www.asx.com.au/asx/markets/priceLookup.do?
> by=asxCodes&asxCodes='
> 
> class stock:
>     code             = ""
>     purchasePrice    = 0
>     purchaseQuantity = 0
>     price            = []  # list of recent prices
>     recentBid        = []  # list of recent bids for stock
>     recentOffer      = []  # list of recent offers for stock
>     stockVol         = []  # list of stock quantity available on
> market

This will be variables belonging to the class itself, not instances of
it.

>     def __init__(self):
>         """ Default Constructor """
>         self.code             = ""
>         self.purchasePrice    = 0
>         self.purchaseQuantity = 0
> 
>     def constructor(self, stockCode, purPrice, purQuant):
>         """ Constructor """
>         self.code             = stockCode
>         self.purchasePrice    = purPrice
>         self.purchaseQuantity = purQuant
> 
>     def setData(self, stockCode, purPrice, purQuant, priceList,
> reBidList, reOffList, popList):
>         """ Defines & implements the objects' public variables """
>         self.code             = stockCode
>         self.purchasePrice    = purPrice
>         self.purchaseQuantity = purQuant
>         self.price            = priceList
>         self.recentBid        = reBidList
>         self.recentOffer      = reOffList
>         self.stockVol         = popList
> 
>         self.printStats()
> 
>     def updateData(self, priceEle, bidEle, offerEle, populEle):
>         """ Adds data to stock object's lists """
>         self.price.append(priceEle)
>         self.recentBid.append(bidEle)
>         self.recentOffer.append(offerEle)
>         self.stockVol.append(populEle)
> 
>     def printStats(self):
>         """ Output Stock attributes """
> 
>         print("Stock Code: "+self.code)

In Python 2 'print' is a statement, so it doesn't need its arguments to
be enclosed in (). You could also use Python's string formatting:

         print "Stock Code: %s" % self.code

>         print("Stock Purchase Price: "+str(self.purchasePrice))
>         print("Stock Quantity Owned: "+str(self.purchaseQuantity))
>         print("***Initial Investment Value:
> "+str(self.purchasePrice*self.purchaseQuantity))
>         if not(len(self.price) <= 0):

'not' has a lower priority than '<=', and this can be simplified anyway:

         if len(self.price) > 0:

or even:

         if self.price:

because empty containers (eg lists) are treated as False, non-empty ones
as True, by 'if' and 'while' statements.

>             print("Stock Current Price: "+str(self.price[-1]))
>             print("Recent Bid: "+str(self.recentBid[-1]))
>             print("Recent Offer: "+str(self.recentOffer[-1]))
>             print("Total Stock Volume in market:
> "+str(self.stockVol[-1]))
>             print("***Present Investment Value:
> "+str(self.price[-1]*self.purchaseQuantity))
>         print("\n")
> 
> 
> ### Functions ###
> def connectDatabase(dbLocation, dbName, tableName):
>     """ Establish & Return connection to SQLite Database """
> 
>     try:
>         if not (os.path.exists(dbLocation)):
>             os.mkdir(dbLocation) # create folder/dir
> 
>         os.chdir(dbLocation)        # change directory focus to
> dbLocation

It's normally easier to use absolute paths instead of changing the
current directory.

>         conn = sqlite3.connect(dbLocation+dbName)

It's better to join paths using os.path.join() because that will insert
any directory separators for you.

>         cur = conn.cursor()
>         try:
>             createTableQ = "CREATE TABLE IF NOT EXISTS "+tableName
> +" (code varchar PRIMARY KEY, purchase_price float, purchase_quantity
> float, purchase_date varchar);"
>             cur.execute(createTableQ)
>             conn.commit()
>         except:
>             pass

DON'T USE BARE EXCEPTS, especially if you're just going to ignore the
exception. Exceptions can be raised for a variety of reasons, including
one you didn't expect due to bugs, so catch only what you expect.

>         return conn
>     except IOError or OSError:

The result of:

     IOError or OSError

is:

     IOError

What you want is:

     except (IOError, OSError):

Note: the parentheses are necessary here, otherwise it it would mean
"catch an IOError exception and then bind it to the name 'OSError'".

>         print "Connection to database failed"
>         return False
> 
Some times you return the connection, and other times False. A more
usual value than False in such cases is None (unless you're returning
True or False).

> def retrieveStockDatabase(conn, tableName):
>     """ Read SQLite3 database & extract stock data into StockList """
> 
>     stockList  = []
>     stockQuery = "select recent_price, recent_offer, recent_bid,
> stock_volume from ? ;"
>     cur = conn.cursor()
>     cur.execute("select code, purchase_price, purchase_quantity from
> "+tableName+";")
> 
>     for row in cur.fetchall():
>         newStock = stock()
>         newStock.code             = row[0]
>         newStock.purchasePrice    = row[1]
>         newStock.purchaseQuantity = row[2]
>         cur.execute(stockQuery,[newStock.code])
>         for rw in cur.fetchall():
>             newStock.price.append(rw[0])
>             newStock.recentOffer.append(rw[1])
>             newStock.recentBid.append(rw[2])
>             newStock.stockVol.append(rw[3])
>         stockList.append(newStock)
> 
>     return stockList
> 
> def getDate():
>     """ Return todays date in format DD:MM:YYYY """
>     time = datetime.datetime.now()
>     date = time.strftime("%d:%m:%Y") # string format time (%y)
>     return date
> 
> def newStockDatabase(conn, stockTable, stock):
>     """ Add a new stock to SQLite database if not already there
>         We save the stocks code, purchase price, quantity purchased
>         & date of purchase.                                       """
>     cur = conn.cursor()
>     try:
>         createTableQ = "create table "+stock.code+" (date varchar
> PRIMARY KEY, recent_price float, recent_offer float, recent_bid float,
> stock_volume double);"
>         stockQuery   = "insert into "+stockTable+"
> values(?, ?, ?, ?);"
>         cur.execute(createTableQ)
>         cur.execute(stockQuery,
> [stock.code,stock.purchasePrice,stock.purchaseQuant,getDate()])
>         conn.commit()
>     except IOError or OSError:
>         print "Table may already exist or bad SQLite connection."
>         return False
> 
> def webFormat(URL):
> 
>     if (URL.startswith("http://")==False):
>         URL = "http://"+URL
> 
Better as:

     if not URL.startswith("http://"):

The conditions of 'if' and 'while' statements don't need to enclosed in
parentheses like in C, Java, etc.

>     return URL
> 
> def getSource(URL):
>     """ Retrieve HTML source code from website URL &
>         save in sourceBuffer                       """
> 
>     try:
>         URL = webFormat(URL) # make sure URL contains essential
> "http://"
>         sourceBuffer = urllib2.urlopen(URL)
>         print '\nResponse code = ',sourceBuffer.code
>         print 'Response headers = ',sourceBuffer.info()
>         print 'Actual URL = ',sourceBuffer.geturl()
>         sourceCode = sourceBuffer.read()
>         sourceBuffer.close()
>         return sourceCode
> 
>     except IOError:  # URLError
>         print "Function Failed: Reasons could be invalid URL name \nOR
> \nHTML protocol message transfer failure."
>         return False # function failed
> 
> def getTargetText(targetStrtData, targetEndData, dataBuffer):
>     """ Grabs target text that lies inside 'dataBuffer' string
>         between targetStrtData & targetEndData                """
> 
>     try:
>         result = dataBuffer.split(targetStrtData)
>         result.pop(0)
>         result = result[0].split(targetEndData)
>         result.pop(1)
>         print result
>         return result
>     except IOError:
>         print "Function Failed: Reasons could be targetStrtData and/or
> targetEndData is not present in dataBuffer."
> 
You haven't given a return statement for when there's an error, so it'll
return None. It's probably clearer if you write a function in Python
either as a _function_ which _always_ returns a value or as a
_procedure_ which _never_ returns a value.

In Python the preference is for a function to raise an exception to
indicate an error instead of returning a flag.

> def getStockData(htmlText, selectedStock):
>     """ Extract stock data(stock code,price,etc) from htmlText """
>     try:
>         # Here I extract my number data from HTML text
>         tempList = []
>         for string in htmlText:
>             for i in string.split('>'):
>                 for e in i.split():
>                         if ('.' in e and e[0].isdigit()):
>                             tempList.append(float(e))
>                         elif (',' in e and e[0].isdigit() ):
>                             # remove ',' chars
>                             e = e.replace(',','')
>                             tempList.append(float(e))
> 
>  
> selectedStock.updateData(tempList[0],tempList[2],tempList[3],tempList[7])
> 
>     except IOError:  # is this the correct error I should be trying to
> catch here??
>         print "Function Failed: Reasons could be: sites HTML data has
> changed. Consult author of program."
>         return False
> 
> def createStockTracker(stockCode,stockPrice,stockQuant, stockList):
>     """ Add a new stock to the database to track """
>     newStock = stock()
>     newStock.constructor(stockCode,stockPrice,stockQuant)
>     stockList.append(newStock)
>     return stockList
> 
> def writeStockToDatabase(conn, stock):
>     """ Write ONLY this Stock's attributes to SQLite Database """
> 
>     cur = conn.cursor()
>     date = getDate()
> 
>     tableName = stock.code
>     stockquery = "insert into "+tableName+" values(?, ?, ?, ?, ?);"
>     cur.execute(query,[date,stock.price[-1], stock.recentOffer[-1],
> stock.recentBid[-1], stock.stockVol[-1]])
>     conn.commit()
> 
> def writeAllToDatabase(conn, stockList):
>     """ Enter recent Stock attributes into SQLite Database """
> 
>     cur = conn.cursor()
>     date = getDate()
> 
>     for stock in stockList:
>         tableName = stock.code
>         stockquery = "insert into "+tableName+"
> values(?, ?, ?, ?, ?);"
>         cur.execute(query,[date,stock.price[-1],
> stock.recentOffer[-1], stock.recentBid[-1], stock.stockVol[-1]])
>         conn.commit()
> 
> ### Input Functions ###
> def inputNewStock():
>     """ """
>     print "*Please note only an Australian Securities Exchange(ASX)
> listed stock can be tracked in Version 1.0."
>     badInput = True
> 
>     while (badInput == True):

Better as:

     while badInput:

>         try:
>             code     = raw_input("Please enter the ASX code for the
> stock you wish to track: ")

If you assign to a name in a function then that name will be treated as
a local name (variable) unless you say it's global in the function,
therefore 'code', etc, won't been seen outside this function.

>             price    = input("Please enter the individual stock value
> for "+code+": ")
>             quantity = input("Please enter the number/quantity of
> stocks purchased: ")
>             if (len(code)>3):
>                 badInput = True
>             else : badInput = False
>             return True

This will check the length, setting badInput accordingly, and then
return True.

>         except IOError:

I don't see anything in the 'try' block that will raise IOError.

>             if (raw_input("Incorrect input. Note: ASX code cannot be
> more than 3 chars. Press 'x' to exit or anything else to try
> again")=='x'):
>                 return False
>         badInput = True # this I am not sure if necessary to loop
> correctly
> 
You're not explicitly returning anything from the function when the loop
exits, so it would return None.

> 
> ### Main program loop ###
> def main():
>     programEnd = False;
>     dbLocation = "C:\Users\Sam\Desktop\StockApp/"

If your string contains literal backslashes then use raw strings
instead. Raw strings can end in a backslash, but that shouldn't matter
in the script if you're joining paths together using os.path.join().

>     dbName     = "stockData.db"
>     stockTable = "stocks"
> 
>     conn = connectDatabase(dbLocation,dbName,stockTable)
>     stockList = retrieveStockDatabase(conn,stockTable)
> 
>     for s in stockList:
>         s.printStats()
> 
>     while (programEnd == False):

Better as:

     while not programEnd:

> 
>         decision = input(menu) # Print Menu

input() is often considered a bad function to use because it's
equivalent to eval(raw_input()), so the user could enter any expression.
It's better to use int(raw_input()) in this case and then catch the
ValueError if what was entered isn't an int.

> 
>         if (decision==1):
> 
>             if not(inputNewStock()==False):

A double-negative, equivalent to:

             if inputNewStock() != False:

It would be more Pythonic if you had inputNewStock() return the code,
etc, or raise an exception if there was an error. You could then catch
that exception in a 'try' block.

>                 stockList =
> createStockTracker(acode,price,quantity,stockList)
>                 newStockDatabase(conn,stockTable,stockList[-1])
>                 print("\n** New Stock **")
>                 stockList[-1].printStats()
>                 print "The stock "+code+" was successfully added to
> our database. \nNow every time the program runs it will automatically
> track this stock & obtain its stock attributes\n\n"
>                 # TO DO:
>                 # get new stock recent Data from internet etc.
>                 # add stock data to data base;
>         elif (decision==2):
>             if (len(stockList)>0):

Better as:

             if stockList:

>                 for Stock in stockList:
>                     URL = ASXurl+Stock.code
>                     sourceCode = getSource(URL)
>                     targetData = getTargetText(target %
> (Stock.code,Stock.code),"</tr>",sourceCode)
>                     getStockData(targetData,Stock)
>                     Stock.printStats()
>                     #writeStockToDatabase(conn,Stock)
>             else:
>                 print "You must first identify a stock to follow.
> \nPlease select option 1."
>         elif (decision==3):
>             print "Thank you for using Stock Program. Goodbye.\n"
>             programEnd = True; # Exit program
> 
>     conn.close()
>     return 0 # End program
> 
'main' is called by the main program and its return value is never used,
so just omit this return statement.

> main()[/CODE]

The recommended standard in Python is for names of classes to be
CamelCase, names of constants to be UPPERCASE_WITH_UNDERSCORES, and
names of everything else to be lowercase_with_underscores.