Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • Those constants (macron, equalSign, underscore) are a bit uselessly named. It's no use having const int TEN = 10 const int TEN = 10; use names that reflect what the constants are for. (This also helps avoid the analogic case of const 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 to f.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 having const int TEN = 10; use names that reflect what the constants are for. (This also helps avoid the analogic case of const 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 to f.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 having const int TEN = 10; use names that reflect what the constants are for. (This also helps avoid the analogic case of const 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 to f.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.

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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:

Source Link
  • Those constants (macron, equalSign, underscore) are a bit uselessly named. It's no use having const int TEN = 10; use names that reflect what the constants are for. (This also helps avoid the analogic case of const 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 to f.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.)

lang-py

AltStyle によって変換されたページ (->オリジナル) /