Paul Kraus wrote: > So now for all my reports all I have to write are little 5 or 6 line scripts > that take a text file split the fields and format them before basing them off > into my custom object. Very slick and this is my first python project. Its > cluttered and messy but for about 1 hours worth of work on a brand new > language I am impressed with the usability of this language.
I'm impressed with how fast you have learned it! > > Current Script - Critique away! :) A number of comments below... > =-=-=-=-=--=-= > #!/usr/bin/python > import string > import re > > class Tbred_24Months: > def __init__(self,currentyear,currentmonth): ### Takes Ending Year and > Ending Month Inits List > guide = [] > self.results = {} > self.guide = {} > self.end_month = currentmonth > self.end_year = currentyear You never use these two attributes ^^ > self.start_month = currentmonth > self.start_year = currentyear start_month and start_year can be local variables, you don't use them outside this method. > for count in range(24): > guide.append((self.start_month,self.start_year)) > self.start_month -= 1 > if self.start_month < 1: > self.start_month = 12 > self.start_year -= 1 > guide.reverse() > count = 0 > for key in guide: > self.guide[key[1],key[0]]=count > count += 1 You can unpack key in the for statement, giving names to the elements: for start_month, start_year in guide: self.guide[start_year, start_month] = count You can generate count automatically with enumerate(): for count, (start_month, start_year) in enumerate(guide): self.guide[start_year, start_month] = count If you built guide with tuples in the order you actually use them (why not?) you could build self.guide as easily as self.guide = dict( (key, count) for count, key in enumerate(guide) ) For that matter, why not just build self.guide instead of guide, in the first loop? for index in range(23, -1, -1): self.guide[start_month,start_year] = index start_month -= 1 if start_month < 1: start_month = 12 start_year -= 1 I found the use of both guide and self.guide confusing but it seems you don't really need guide at all. > self.sortedkeys = self.guide.keys() > self.sortedkeys.sort() You don't use sortedkeys > > def insert(self,key,year,month,number): > if self.guide.has_key((year,month)): > if self.results.has_key(key): > seq = self.guide[(year,month)] > self.results[key][seq] += number > else: > self.results[key] = [] > for x in range(24):self.results[key].append(0) Could be self.results[key] = [0*24] Is there a bug here? You don't add in number when the key is not there. I like to use dict.setdefault() in cases like this: if self.guide.has_key((year,month)): seq = self.guide[(year,month)] self.results.setdefault(key, [0*24])[seq] += number though if performance is a critical issue your way might be better - it doesn't have to build the default list every time. (Don't try to build the default list just once, you will end up with every entry aliased to the same list!) (I hesitate to include this note at all - you will only notice a difference in performance if you are doing this operation many many times!) > > def splitfields(record): > fields = [] > datestring='' > ### Regular Expr. > re_negnum = re.compile('(\d?)\.(\d+)(-)') > re_date = re.compile('(\d\d)/(\d\d)/(\d\d)') > for element in record.split('|'): > element=element.strip() # remove leading/trailing whitespace > ### Move Neg Sign from right to left of number > negnum_match = re_negnum.search( element ) > if negnum_match: > if negnum_match.group(1):element = "%s%d.%02d" > %(negnum_match.group(3),int(negnum_match.group(1)),int(negnum_match.group(2))) > else:element = "%s0.%02d" > %(negnum_match.group(3),int(negnum_match.group(2))) This is a lot of work just to move a - sign. How about if element.endswith('-'): element = '-' + element[:-1] > ### Format Date > date_match = re_date.search(element) > if date_match: > (month,day,year) = > (date_match.group(1),date_match.group(2),date_match.group(3)) > ### Convert 2 year date to 4 year > if int(year) > 80:year = "19%02d" %int(year) > else:year = "20%02d" %int(year) > element = (year,month,day) You go to a lot of trouble to turn your year into a string, then you convert it back to an int later. Why not just keep it as an int? Do you know that Python data structures (lists, dicts, tuples) are heterogeneous? You can have a list that contains strings, ints, tuples, etc. > if element == '.000': element = 0.00 > fields.append( element ) > return fields This whole splitfields() function is kind of strange. It seems you know what order the fields are in since the caller assumes an order. So why not something like elements = record.split('|') if len(elements) != 7: return None vendor,otype,oreturn,discountable,discperc,amount,date discperc = processNumber(discperc) amount = processNumber(amount) date = processDate(date) return vendor,otype,oreturn,discountable,discperc,amount,date then all the ugly regex stuff goes into processNumber() and processDate() and the intent of the code is much clearer. processNumber() can return a float and processDate() can return the (year, month, day) tuple you need. > > > ### Build Vendor Sales > sales = Tbred_24Months(2005,11) > vendorsales = open('vendorsales.txt','r') > for line in vendorsales: > fields = splitfields( line ) > if len(fields) == 7: > (vendor,otype,oreturn,discountable,discperc,amount,date) = fields > amount = float(amount);discperc = float(discperc) > #if discperc and discountable == 'Y': amount = amount - ( amount * > (discperc/100) ) > if otype == 'C' or oreturn == 'Y':amount = amount * -1 > sales.insert(vendor,int(date[0]),int(date[1]),amount) > result = '' > > for key in sales.results: > sum = float(0) could be sum = 0.0 > result = str(key) > for amount in sales.results[key]: > sum += amount > result += "|" + str(amount) You can iterate keys and values together with for key, amounts in sales.results.iteritems(): then it would be for amount in amounts: > print str(key),sum > #print result,sum HTH Kent _______________________________________________ Tutor maillist - Tutor@python.org http://mail.python.org/mailman/listinfo/tutor