Do me a favor, would you? Have a quick look at this code and tell me what you think it does...
...
Do me a favor, would you? Have a quick look at this code and tell me what you think it does...
Code Block |
---|
MysteryProcedureUsingCodeBlock (long StudentID)!,long ! Declare Procedure FilesOpened BYTE(0) Result long long StepNumber end CourseQ long MostAvoidedTeacher Queue CourseID cstring(100) CourseTakenQ Queue CourseID long Taken long TeacherID BOOL long end TeacherQ end CourseNotTakenQ Queue TeacherID Queue CourseID Long ClassesTaught long TeacherID Long long end x end CourseQ long WasTaken Queue CourseID bool CODE do OpenFiles STU:StudentID = StudentID get(Student,STU:pkStudentID) if errorcode() long Taken do CloseFiles return FALSE end set(Course) loop next(Course) if errorcode() then break.BOOL clear(CRSI:Record) CRSI:CourseID = CRS:CourseID set(CRSI:kCourseIDTeacherID,CRSI:kCourseIDTeacherID) WasTaken = false loop end TeacherQ next(CourseInstance) if errorcode() or CRSI:CourseID <> CRS:CourseID then break. ENR:StudentID = StudentIDQueue TeacherID ENR:CourseInstanceID = crsi:CourseInstanceID get(Enrollment,ENR:kCourseInstanceUDStudentID) if not errorcode() Long ClassesTaught WasTaken = TRUE break endLong end if WasTaken clear(CourseTakenQ) CourseTakenQ.CourseID = CRS:CourseID end x CourseTakenQ.TeacherID = CRSI:TeacherID add(CourseTakenQ) else clear(CourseNotTakenQ) long WasTaken CourseNotTakenQ.CourseID = CRS:CourseID CourseNotTakenQ.TeacherID = CRSI:TeacherID add(CourseNotTakenQ) bool CODE end do OpenFiles end STU:StudentID = set(Teacher)StudentID loopget(Student,STU:pkStudentID) if nexterrorcode(Teacher) if errorcode() then break.do CloseFiles TeacherQ.TeacherIDreturn =FALSE TEA:TeacherID end addset(TeacherQCourse) endloop loop x = 1 to recordsnext(CourseTakenQCourse) get(CourseTakenQ,xif errorcode() TeacherQ.TeacherID = CourseTakenQ.TeacherIDthen break. get(TeacherQ,TeacherQ.TeacherIDclear(CRSI:Record) ifCRSI:CourseID not errorcode() ! should always find a record = CRS:CourseID set(CRSI:kCourseIDTeacherID,CRSI:kCourseIDTeacherID) WasTaken TeacherQ.ClassesTaught += 1false loop put(TeacherQ) end next(CourseInstance) end sort(TeacherQ,-TeacherQ.ClassesTaught) loop while not if errorcode() or CRSI:CourseID <> CRS:CourseID then break. get(TeacherQ,6) delete(TeacherQ)ENR:StudentID = StudentID end x = 1ENR:CourseInstanceID = crsi:CourseInstanceID loop get(CourseNotTakenQEnrollment,xENR:kCourseInstanceUDStudentID) if not errorcode() then break. TeacherQ.TeacherID = CourseNotTakenQ.TeacherID WasTaken = TRUE get(TeacherQ,TeacherQ.TeacherID) if not errorcode() break x += 1 end else end if WasTaken delete(CourseNotTakenQ) end clear(CourseTakenQ) end TeacherQ.TeacherID = CourseNotTakenQ CourseTakenQ.CourseID = CRS:CourseID CourseTakenQ.TeacherID = CRSI:TeacherID get(TeacherQ,TeacherQ.TeacherID) get(CourseNotTakenQ,random(1,records(CourseNotTakenQ)))add(CourseTakenQ) do CloseFiles else return false ! because this code is not yet complete |
How are you making out? Do you have a clear idea of what this code does?
If you study the code long enough you should be able to figure out what it does, but you probably still won't know why it's doing what it does.
Now consider this version of the same code (I've omitted the OpenFiles/CloseFiles routines in both cases):
Code Block |
---|
MysteryProcedureUsingLocalProcedures (long StudentID)!,long ! Declare Procedureclear(CourseNotTakenQ) CourseNotTakenQ.CourseID = CRS:CourseID CourseNotTakenQ.TeacherID = CRSI:TeacherID add(CourseNotTakenQ) map end end set(Teacher) loop next(Teacher) GetStudentRecord(),long if errorcode() then break. TeacherQ.TeacherID = TEA:TeacherID add(TeacherQ) end loop x = 1 to BuildListsOfCoursesTakenAndNotTakenrecords(CourseTakenQ),long get(CourseTakenQ,x) TeacherQ.TeacherID = CourseTakenQ.TeacherID get(TeacherQ,TeacherQ.TeacherID) if not CourseWasTakenerrorcode(),long ! should always find a record TeacherQ.ClassesTaught += 1 BuildListOfTeachers(),longput(TeacherQ) end end sort(TeacherQ,-TeacherQ.ClassesTaught) loop while not errorcode() get(TeacherQ,6) CountCoursesTaughtToStudentdelete(TeacherQ),long end x = 1 loop get(CourseNotTakenQ,x) if errorcode() then break. SortTeacherListByCountOfCoursesTaughtToStudent(),long TeacherQ.TeacherID = CourseNotTakenQ.TeacherID get(TeacherQ,TeacherQ.TeacherID) if not errorcode() x += 1 NarrowTeacherListToFiveMostAvoided(),long else delete(CourseNotTakenQ) end end TeacherQ.TeacherID = CourseNotTakenQ.TeacherID NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers(),longget(TeacherQ,TeacherQ.TeacherID) get(CourseNotTakenQ,random(1,records(CourseNotTakenQ))) do CloseFiles return false ! because this code is not CourseIsTaughtByOneOfFiveMostAvoidedTeachers(),long yet complete |
How are you making out? Do you have a clear idea of what this code does?
If you study the code long enough you should be able to figure out what it does, but you probably still won't know why it's doing what it does.
Now consider this version of the same code (I've omitted the OpenFiles/CloseFiles routines in both cases):
Code Block |
---|
MysteryProcedureUsingLocalProcedures (long StudentID)!,long ! Declare Procedure RegisterStudentForRandomlyChosenCourse(),long map end FilesOpened GetStudentRecord(),long BYTE(0) Result BuildListsOfCoursesTakenAndNotTaken(),long long StepNumber CourseWasTaken(),long long MostAvoidedTeacher BuildListOfTeachers(),long cstring(100) CourseTakenQ CountCoursesTaughtToStudent(),long Queue CourseID SortTeacherListByCountOfCoursesTaughtToStudent(),long long TeacherID NarrowTeacherListToFiveMostAvoided(),long long NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers(),long end CourseNotTakenQ CourseIsTaughtByOneOfFiveMostAvoidedTeachers(),long Queue CourseID RegisterStudentForRandomlyChosenCourse(),long long TeacherID end FilesOpened long BYTE(0) Result end CourseQ long StepNumber Queue CourseID long MostAvoidedTeacher cstring(100) CourseTakenQ long Taken Queue CourseID BOOL long TeacherID end TeacherQ long Queue TeacherID end CourseNotTakenQ Long ClassesTaught Queue CourseID Long long TeacherID end CODE do OpenFiles StepNumber = 0 long Result = Level:Benign Loop while Result = Level:Benign StepNumber += 1 execute StepNumber Result = GetStudentRecord() end CourseQ Result = BuildListsOfCoursesTakenAndNotTaken() Result = BuildListOfTeachers() Result = CountCoursesTaughtToStudent() Queue CourseID Result = SortTeacherListByCountOfCoursesTaughtToStudent() Result = NarrowTeacherListToFiveMostAvoided() Result = NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers() long Taken Result = RegisterStudentForRandomlyChosenCourse() break end end do CloseFiles Return result GetStudentRecordBOOL procedure()!,long code STU:StudentID = StudentID get(Student,STU:pkStudentID) if not errorcode() then return level:benign. end TeacherQ return level:notify BuildListsOfCoursesTakenAndNotTaken procedure()!,long code set(Course) loop next(Course) Queue TeacherID if errorcode() then break. if CourseWasTaken() = Level:Benign clear(CourseTakenQ) CourseTakenQ.CourseID = CRS:CourseID Long ClassesTaught CourseTakenQ.TeacherID = CRSI:TeacherID add(CourseTakenQ) else Long clear(CourseNotTakenQ) CourseNotTakenQ.CourseID = CRS:CourseID CourseNotTakenQ.TeacherID = CRSI:TeacherID add(CourseNotTakenQ) end CODE do OpenFiles end StepNumber = 0 end Result return= Level:Benign Loop CourseWasTakenwhile Result = Level:Benign StepNumber += 1 execute StepNumber procedure()!,long WasTaken Result = GetStudentRecord() Result = BuildListsOfCoursesTakenAndNotTaken() bool Result code= BuildListOfTeachers() clear(CRSI:Record) CRSI:CourseID = CRS:CourseID Result = set(CRSI:kCourseIDTeacherID,CRSI:kCourseIDTeacherIDCountCoursesTaughtToStudent() loop Result = nextSortTeacherListByCountOfCoursesTaughtToStudent(CourseInstance) if errorcode Result = NarrowTeacherListToFiveMostAvoided() or CRSI:CourseID <> CRS:CourseID then break. Result ENR:StudentID = StudentID= NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers() ENR:CourseInstanceID = crsi:CourseInstanceIDResult = RegisterStudentForRandomlyChosenCourse() get(Enrollment,ENR:kCourseInstanceUDStudentID) break if not errorcode() end end WasTaken =do TRUECloseFiles Return result GetStudentRecord break end end if WasTaken then return level:benign. procedure()!,long returncode level:notify STU:StudentID = BuildListOfTeachersStudentID get(Student,STU:pkStudentID) if not errorcode() then return level:benign. procedure(return level:notify BuildListsOfCoursesTakenAndNotTaken procedure()!,long code set(TeacherCourse) loop next(TeacherCourse) if errorcode() then break. TeacherQ.TeacherID if CourseWasTaken() = TEA:TeacherIDLevel:Benign addclear(TeacherQCourseTakenQ) end CourseTakenQ.CourseID return= LevelCRS:BenignCourseID CourseTakenQ.TeacherID = CountCoursesTaughtToStudentCRSI:TeacherID procedureadd(CourseTakenQ)!,long x else clear(CourseNotTakenQ) CourseNotTakenQ.CourseID = CRS:CourseID long code CourseNotTakenQ.TeacherID = CRSI:TeacherID loop x = 1 to records(CourseTakenQ) add(CourseNotTakenQ) get(CourseTakenQ,x) TeacherQ.TeacherID = CourseTakenQ.TeacherID end get(TeacherQ,TeacherQ.TeacherID) end return Level:Benign if not errorcode() ! should alwaysCourseWasTaken find a record TeacherQ.ClassesTaught += 1 procedure()!,long WasTaken put(TeacherQ) end end return Level:Benign SortTeacherListByCountOfCoursesTaughtToStudent procedure()!,long code bool sort(TeacherQ,-TeacherQ.ClassesTaught) code return Level:Benignclear(CRSI:Record) CRSI:CourseID NarrowTeacherListToFiveMostAvoided= CRS:CourseID procedure()!,long xset(CRSI:kCourseIDTeacherID,CRSI:kCourseIDTeacherID) loop next(CourseInstance) if errorcode() or CRSI:CourseID <> CRS:CourseID then break. ENR:StudentID = StudentID Long code ENR:CourseInstanceID = loopcrsi:CourseInstanceID while not errorcode() get(TeacherQEnrollment,6ENR:kCourseInstanceUDStudentID) if not deleteerrorcode(TeacherQ) end return Level:Benign NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers procedure()!,long xWasTaken = TRUE break end end if WasTaken then return level:benign. return level:notify BuildListOfTeachers procedure()!,long code xset(Teacher) = 1 loop get(CourseNotTakenQ,xnext(Teacher) if errorcode() then break. if CourseIsTaughtByOneOfFiveMostAvoidedTeachers() TeacherQ.TeacherID = LevelTEA:BenignTeacherID add(TeacherQ) x +=end 1 return Level:Benign else CountCoursesTaughtToStudent delete(CourseNotTakenQ) end procedure()!,long x end return Level:Benign CourseIsTaughtByOneOfFiveMostAvoidedTeachers procedure()!,long code TeacherQ.TeacherID = CourseNotTakenQ.TeacherID get(TeacherQ,TeacherQ.TeacherID) if not errorcode()long then return Level:Benign. code return Level:Notify loop x = 1 RegisterStudentForRandomlyChosenCourse procedure()!,long to records(CourseTakenQ) code get(CourseNotTakenQ,random(1,records(CourseNotTakenQ)))CourseTakenQ,x) return Level:Notify ! becauseTeacherQ.TeacherID this= codeCourseTakenQ.TeacherID is not yet complete get(TeacherQ,TeacherQ.TeacherID) |
Code Block |
---|
if donot OpenFileserrorcode() ! should always find StepNumbera =record 0 Result = Level:Benign Loop while Result = Level:BenignTeacherQ.ClassesTaught += 1 StepNumber += 1 put(TeacherQ) execute StepNumber end end return Level:Benign ResultSortTeacherListByCountOfCoursesTaughtToStudent = GetStudentRecordprocedure()!,long code sort(TeacherQ,-TeacherQ.ClassesTaught) Result = BuildListsOfCoursesTakenAndNotTaken() return Level:Benign NarrowTeacherListToFiveMostAvoided Result = BuildListOfTeachersprocedure()!,long x Result = CountCoursesTaughtToStudent() Result = SortTeacherListByCountOfCoursesTaughtToStudent() Result = NarrowTeacherListToFiveMostAvoided() Long code loop while not errorcode() get(TeacherQ,6) delete(TeacherQ) end Result = return Level:Benign NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers procedure()!,long x Result = RegisterStudentForRandomlyChosenCourse() break end end do CloseFiles Return result |
long
code
x = 1
loop
get(CourseNotTakenQ,x)
if errorcode() then break.
if CourseIsTaughtByOneOfFiveMostAvoidedTeachers() = Level:Benign
x += 1
else
delete(CourseNotTakenQ)
end
end
return Level:Benign
CourseIsTaughtByOneOfFiveMostAvoidedTeachers procedure()!,long
code
TeacherQ.TeacherID = CourseNotTakenQ.TeacherID
get(TeacherQ,TeacherQ.TeacherID)
if not errorcode() then return Level:Benign.
return Level:Notify
RegisterStudentForRandomlyChosenCourse procedure()!,long
code
get(CourseNotTakenQ,random(1,records(CourseNotTakenQ)))
return Level:Notify ! because this code is not yet complete
|
I'm pretty confident that you'll find the second version a lot easier to understand, because it breaks up all those blocks of code into procedures with descriptive names.
Note |
---|
While this code does compile, and does use a real dictionary, I make no claims as to whether it works. Certainly it is incomplete, because I haven't yet written the final local procedure. In any case the purpose is not to register students for courses taught by teachers they avoid but to demonstrate how complicated code can be improved. |
There are a couple of things you may find unfamiliar about this code. One is the use of a local map and local procedures. Just as you can (and almost always do) have a map in a program module, you can have maps within modules and within procedures.
Also each procedure is quite short because short blocks of code are generally easier to understand than long blocks of code.
The Hanson Loop
But the real beauty of this procedure is a construct Mike Hanson recently showed me, and which I've christened the Hanson Loop:
Code Block |
---|
StepNumber = 0
Result = Level:Benign
Loop while Result = Level:Benign
StepNumber += 1
execute StepNumber
Result = GetStudentRecord()
Result = BuildListsOfCoursesTakenAndNotTaken()
Result = BuildListOfTeachers()
Result = CountCoursesTaughtToStudent()
Result = SortTeacherListByCountOfCoursesTaughtToStudent()
Result = NarrowTeacherListToFiveMostAvoided()
Result = NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers()
Result = RegisterStudentForRandomlyChosenCourse()
break
end
end
do CloseFiles
Return result |
The Hanson Loop is a while loop - it keeps executing as long as the Result variable is equal to Level:Benign. You could write a similar loop testing for Result = True, but I think there are some important benefits to using the Clarion return levels, which I'll get to in a moment.
Within the Hanson Loop there's an Execute structure. When the StepNumber variable has a value of 1, the first line in the Execute structure is executed; when the StepNumber variable has a value of 2, the second line executes, and so on.
The very last statement in the Execute structure is simply a break command, which exits the loop.
The beauty of the Hanson Loop is it provides a clear overview of the code being executed by the main procedure, while allowing for clean and efficient error handling. At any time in the sequence of procedure calls a non-benign result will terminate the loop. Do you need to wrap this code in a transaction? It's as easy as doing a logout before the loop, and either a commit or a rollback depending on the value of Result.
Now, about those Clarion error levels.
The Clarion error levels
It has taken me years, nearly two decades, to adopt Clarion error levels in my own coding.
I'm almost completely convinced.
The Clarion help has this to say about error levels, in the context of the ABC ErrorManager object:
Panel | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Six Levels of Treatment By default, the error manager recognizes six different levels of error severity. The default actions for these levels range from no action for benign errors to halting the program for fatal errors. The error manager also supports the intermediate actions of simply notifying the user, or of notifying the user and letting the user decide whether to continue or abort. Customizable Treatments These various levels of treatment are implemented with virtual methods so they are easy to customize. The error manager calls a different virtual method for each severity level, so you can override the default error actions with your own application specific error actions. See the various Take methods for examples. The recognized severity EQUATEs are declared in EQUATES.CLW. These severity levels and their default actions are:
You may define your own additional severity levels and their associated actions. |
These values specifically indicate actions the ErrorManager may take. But there aren't quite as many of them as there may seem to be at first. Level:User appears nowhere in the ABC class library or the templates, other than as a definition. Level:Program is the same as Level:Fatal, and Level:Fatal means exit the program, so it's not something you're likely to use. So that leaves Benign, Notify, and Cancel.
Over the years I've written may procedures and methods that returned a true/false value, and in those circumstances the Clarion error levels seemed overkill. Of the three really usable levels I seem to only need Benign and Notify, to signify True and False. So why not just use True and False?
In part I've adopted the Clarion error levels to be more in sync with the ABC classes, and in part because using levels leaves open the possibility of indicating different levels of success or failure, even if it's not a situation I come across often. But as soon as I do have one of those situations I suddenly become inconsistent, returning True/False in most of my code and three or more values in other code.
As you can see in the example I'm even using Level:Benign when there is only one potential return value, although I've mainly done that to make the code line up neatly and not because I suspect I will need a different value at some point. I could just as easily not assign a value to ReturnValue in any of the lines in the Execute statement - the loop will continue as before and will always move beyond those lines where ReturnValue is never assigned.
Summary
As I said you can always adapt the Hanson Loop to work with True/False values. That's not so important; what does matter is the Execute nested inside the Loop, and the use of local procedures to break up long blocks of code.
If you have long blocks of code that are difficult to understand, I highly recommend the Hanson Loop. Be descriptive in your procedure names, keep your procedures short, and your code will become easier to read and easier to maintain.