Those constants (
macron
,equalSign
,underscore
) are a bit uselessly named. It's no use havingconst int TEN = 10
const int TEN = 10
; use names that reflect what the constants are for. (This also helps avoid the analogic case ofconst int TEN = 11
when you need to change the constant in a new version...) Better yet, put the constants into predicate functions that encapsulate all the checking in one easy-to-use package.The nested loops over your file handle may work, but they're confusing as heck. I recommend an outer
while True
loop and manual calls tof.next()
.Your code is very nested. Try splitting it into multiple functions.
It's often preferable to return early (
if not tempList: break
) instead of nesting (if tempList: ...
).Your inner loop could be improved to handle an entire "record" (begin day, stuff happens, end day) at a time, rather than taking each line and clumsily maintaining state. (All this really entails is changing the structure to remove the loops and clearly define a sequence of processing; moving the code into its own function is just bonus points.)
Some bits of code common to all branches of execution (
d.month_year(month_year)
is one spot,d.activity_name(tempList[0])
is another) can be moved out of the branching constructs.If a day is started without ending the previous day, the code throws the previous day away. You might want to change that.
You're not awfully clear about whether the macron to start a new day gets its own line; your code acts as if it does (by throwing away the rest of that line), so I'm going with that.
Your terminating characters (macron, equals, underscore) - you say they have to be at the beginning of the line, but then test if they're anywhere in the line! Combined with the above leap of logic, you can simplify those tests to checking if the character is the only one on its line.
Other-activity sections (terminated with
_
) don't terminate a record when they end. I'l assume it's impermissible to have anything between the end of an other-section and the end of the record.
Those constants (
macron
,equalSign
,underscore
) are a bit uselessly named. It's no use havingconst int TEN = 10
; use names that reflect what the constants are for. (This also helps avoid the analogic case ofconst int TEN = 11
when you need to change the constant in a new version...) Better yet, put the constants into predicate functions that encapsulate all the checking in one easy-to-use package.The nested loops over your file handle may work, but they're confusing as heck. I recommend an outer
while True
loop and manual calls tof.next()
.Your code is very nested. Try splitting it into multiple functions.
It's often preferable to return early (
if not tempList: break
) instead of nesting (if tempList: ...
).Your inner loop could be improved to handle an entire "record" (begin day, stuff happens, end day) at a time, rather than taking each line and clumsily maintaining state. (All this really entails is changing the structure to remove the loops and clearly define a sequence of processing; moving the code into its own function is just bonus points.)
Some bits of code common to all branches of execution (
d.month_year(month_year)
is one spot,d.activity_name(tempList[0])
is another) can be moved out of the branching constructs.If a day is started without ending the previous day, the code throws the previous day away. You might want to change that.
You're not awfully clear about whether the macron to start a new day gets its own line; your code acts as if it does (by throwing away the rest of that line), so I'm going with that.
Your terminating characters (macron, equals, underscore) - you say they have to be at the beginning of the line, but then test if they're anywhere in the line! Combined with the above leap of logic, you can simplify those tests to checking if the character is the only one on its line.
Other-activity sections (terminated with
_
) don't terminate a record when they end. I'l assume it's impermissible to have anything between the end of an other-section and the end of the record.
Those constants (
macron
,equalSign
,underscore
) are a bit uselessly named. It's no use havingconst int TEN = 10
; use names that reflect what the constants are for. (This also helps avoid the analogic case ofconst int TEN = 11
when you need to change the constant in a new version...) Better yet, put the constants into predicate functions that encapsulate all the checking in one easy-to-use package.The nested loops over your file handle may work, but they're confusing as heck. I recommend an outer
while True
loop and manual calls tof.next()
.Your code is very nested. Try splitting it into multiple functions.
It's often preferable to return early (
if not tempList: break
) instead of nesting (if tempList: ...
).Your inner loop could be improved to handle an entire "record" (begin day, stuff happens, end day) at a time, rather than taking each line and clumsily maintaining state. (All this really entails is changing the structure to remove the loops and clearly define a sequence of processing; moving the code into its own function is just bonus points.)
Some bits of code common to all branches of execution (
d.month_year(month_year)
is one spot,d.activity_name(tempList[0])
is another) can be moved out of the branching constructs.If a day is started without ending the previous day, the code throws the previous day away. You might want to change that.
You're not awfully clear about whether the macron to start a new day gets its own line; your code acts as if it does (by throwing away the rest of that line), so I'm going with that.
Your terminating characters (macron, equals, underscore) - you say they have to be at the beginning of the line, but then test if they're anywhere in the line! Combined with the above leap of logic, you can simplify those tests to checking if the character is the only one on its line.
Other-activity sections (terminated with
_
) don't terminate a record when they end. I'l assume it's impermissible to have anything between the end of an other-section and the end of the record.
Also taking into account Christoph Hegemann's pointers Christoph Hegemann's pointers, your revised code:
Also taking into account Christoph Hegemann's pointers, your revised code:
Also taking into account Christoph Hegemann's pointers, your revised code:
Those constants (
macron
,equalSign
,underscore
) are a bit uselessly named. It's no use havingconst int TEN = 10
; use names that reflect what the constants are for. (This also helps avoid the analogic case ofconst int TEN = 11
when you need to change the constant in a new version...) Better yet, put the constants into predicate functions that encapsulate all the checking in one easy-to-use package.The nested loops over your file handle may work, but they're confusing as heck. I recommend an outer
while True
loop and manual calls tof.next()
.Your code is very nested. Try splitting it into multiple functions.
It's often preferable to return early (
if not tempList: break
) instead of nesting (if tempList: ...
).Your inner loop could be improved to handle an entire "record" (begin day, stuff happens, end day) at a time, rather than taking each line and clumsily maintaining state. (All this really entails is changing the structure to remove the loops and clearly define a sequence of processing; moving the code into its own function is just bonus points.)
Some bits of code common to all branches of execution (
d.month_year(month_year)
is one spot,d.activity_name(tempList[0])
is another) can be moved out of the branching constructs.If a day is started without ending the previous day, the code throws the previous day away. You might want to change that.
You're not awfully clear about whether the macron to start a new day gets its own line; your code acts as if it does (by throwing away the rest of that line), so I'm going with that.
Your terminating characters (macron, equals, underscore) - you say they have to be at the beginning of the line, but then test if they're anywhere in the line! Combined with the above leap of logic, you can simplify those tests to checking if the character is the only one on its line.
Other-activity sections (terminated with
_
) don't terminate a record when they end. I'l assume it's impermissible to have anything between the end of an other-section and the end of the record.
Also taking into account Christoph Hegemann's pointers, your revised code:
def isStartWorkDay(line):
return line == " ̄\n"
def isEndWorkDay(line):
return line == "=\n"
def isEndOtherActivitySection(line):
return line == "_\n"
def processRecord(f, d):
# This line should just start a record
line = f.next()
if line == '\n':
return False
# Start of record should be start of record
if not isStartWorkDay(line):
raise Exception("You missed the ̄ at the start of the day")
# First line always starts with a day or a special identifier ("sick" etc.)
lineList = f.next().rsplit()
d.activity_name(lineList[0])
if isWorkingDay(lineList[0]):
# Line 1 is day of week, day of month, [boring]
d.date_of_month(lineList[1])
# Line 2 is [boring], hours worked, [boring], pay, [boring]
lineList = f.next().rsplit()
d.hours_worked(lineList[1])
d.pay(lineList[3])
else:
# Something special happened; go through lines describing it
# I know this is potentially confusing (as I said above), but it's the "least bad" way
for line in f:
if isEndOtherActivitySection(line):
# That's it for this section
break
elseif "Credit:" in line:
# It's a pay credit, we need to record it
d.pay(line[8:12])
else:
pass # Move along, nothing to see here
# All right, this record had better be ending now
line = f.next()
if isEndWorkDay(line):
return True
raise Exception("You missed the = at the end of the day")
def readBodyIntoDb(path, month_year):
'''Reads schedule detail text file and generates pairing data objects for each work day.
path: file to open 'monthYEAR.txt'
date: MonthYEAREMPNO (month, year, employee number)
from file header'''
d = None # WorkDay object; required in outer scope
with open(path) as f:
try:
while True:
d = WorkDay()
d.month_year(month_year)
if processRecord(f, d): # d modified by side effect
dbWrite(d)
except StopIteration:
# EOF. I filled this pseudocode in for you
if isEmptyRecord(d):
pass # Nothing to see here
elseif isCompleteRecord(d):
dbWrite(d) # Someone left off the last `=` or something
else:
raise Exception("incomplete record")
return True
(My Python's a bit rusty, so there may be latent syntax or logic errors in there.)