Child pages
  • Clean up your code with the Hanson Loop
Skip to end of metadata
Go to start of metadata

Do me a favor, would you? Have a quick look at this code and tell me what you think it does...

MysteryProcedureUsingCodeBlock  (long StudentID)!,long ! Declare Procedure


FilesOpened     BYTE(0)
Result                                              long
StepNumber                                          long
MostAvoidedTeacher                                  cstring(100)
CourseTakenQ                                        Queue
CourseID                                                long
TeacherID                                               long
                                                    end
CourseNotTakenQ                                     Queue
CourseID                                                long
TeacherID                                               long
                                                    end
CourseQ                                             Queue
CourseID                                                long
Taken                                                   BOOL
                                                    end
TeacherQ                                            Queue
TeacherID                                               Long
ClassesTaught                                           Long
                                                    end
x                                                   long
WasTaken                                            bool

    CODE
    do OpenFiles
    STU:StudentID = StudentID
    get(Student,STU:pkStudentID)
    if errorcode()
        do CloseFiles
        return FALSE
    end
    set(Course)
    loop
        next(Course)
        if errorcode() then break.
        clear(CRSI:Record)
        CRSI:CourseID = CRS:CourseID
        set(CRSI:kCourseIDTeacherID,CRSI:kCourseIDTeacherID)
        WasTaken = false
        loop 
            next(CourseInstance)
            if errorcode() or CRSI:CourseID <> CRS:CourseID then break.
            ENR:StudentID = StudentID
            ENR:CourseInstanceID = crsi:CourseInstanceID
            get(Enrollment,ENR:kCourseInstanceUDStudentID)
            if not errorcode()
                WasTaken = TRUE
                break
            end
        end
        if WasTaken
            clear(CourseTakenQ)
            CourseTakenQ.CourseID = CRS:CourseID
            CourseTakenQ.TeacherID = CRSI:TeacherID
            add(CourseTakenQ)
        else
            clear(CourseNotTakenQ)
            CourseNotTakenQ.CourseID = CRS:CourseID
            CourseNotTakenQ.TeacherID = CRSI:TeacherID
            add(CourseNotTakenQ)           
        end
    end
    set(Teacher)
    loop
        next(Teacher)
        if errorcode() then break.
        TeacherQ.TeacherID = TEA:TeacherID
        add(TeacherQ)
    end
    loop x = 1 to records(CourseTakenQ)
        get(CourseTakenQ,x)
        TeacherQ.TeacherID = CourseTakenQ.TeacherID
        get(TeacherQ,TeacherQ.TeacherID)
        if not errorcode() ! should always find a record
            TeacherQ.ClassesTaught += 1
            put(TeacherQ)
        end
    end
    sort(TeacherQ,-TeacherQ.ClassesTaught)
    loop while not errorcode()
        get(TeacherQ,6)
        delete(TeacherQ)
    end
    x = 1
    loop 
        get(CourseNotTakenQ,x)
        if errorcode() then break.
        TeacherQ.TeacherID = CourseNotTakenQ.TeacherID
        get(TeacherQ,TeacherQ.TeacherID)
        if not errorcode()
            x += 1
        else
            delete(CourseNotTakenQ)
        end
    end
    TeacherQ.TeacherID = CourseNotTakenQ.TeacherID
    get(TeacherQ,TeacherQ.TeacherID)
    get(CourseNotTakenQ,random(1,records(CourseNotTakenQ)))
    do CloseFiles
    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):

MysteryProcedureUsingLocalProcedures  (long StudentID)!,long ! Declare Procedure

                                        map
                                            GetStudentRecord(),long
                                            BuildListsOfCoursesTakenAndNotTaken(),long
                                            CourseWasTaken(),long
                                            BuildListOfTeachers(),long
                                            CountCoursesTaughtToStudent(),long
                                            SortTeacherListByCountOfCoursesTaughtToStudent(),long
                                            NarrowTeacherListToFiveMostAvoided(),long
                                            NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers(),long
                                            CourseIsTaughtByOneOfFiveMostAvoidedTeachers(),long
                                            RegisterStudentForRandomlyChosenCourse(),long
                                        end
FilesOpened                                         BYTE(0)
Result                                              long
StepNumber                                          long
MostAvoidedTeacher                                  cstring(100)
CourseTakenQ                                        Queue
CourseID                                                long
TeacherID                                               long
                                                    end
CourseNotTakenQ                                     Queue
CourseID                                                long
TeacherID                                               long
                                                    end
CourseQ                                             Queue
CourseID                                                long
Taken                                                   BOOL
                                                    end
TeacherQ                                            Queue
TeacherID                                               Long
ClassesTaught                                           Long
                                                    end

    CODE
    do OpenFiles
    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
    
GetStudentRecord                        procedure()!,long
    code
    STU:StudentID = StudentID
    get(Student,STU:pkStudentID)
    if not errorcode() then return level:benign.
    return level:notify
    
BuildListsOfCoursesTakenAndNotTaken     procedure()!,long
    code
    set(Course)
    loop
        next(Course)
        if errorcode() then break.
        if CourseWasTaken() = Level:Benign
            clear(CourseTakenQ)
            CourseTakenQ.CourseID = CRS:CourseID
            CourseTakenQ.TeacherID = CRSI:TeacherID
            add(CourseTakenQ)
        else
            clear(CourseNotTakenQ)
            CourseNotTakenQ.CourseID = CRS:CourseID
            CourseNotTakenQ.TeacherID = CRSI:TeacherID
            add(CourseNotTakenQ)           
        end
    end
    return Level:Benign
    
CourseWasTaken                          procedure()!,long
WasTaken                                    bool
    code
    clear(CRSI:Record)
    CRSI:CourseID = CRS:CourseID
    set(CRSI:kCourseIDTeacherID,CRSI:kCourseIDTeacherID)
    loop 
        next(CourseInstance)
        if errorcode() or CRSI:CourseID <> CRS:CourseID then break.
        ENR:StudentID = StudentID
        ENR:CourseInstanceID = crsi:CourseInstanceID
        get(Enrollment,ENR:kCourseInstanceUDStudentID)
        if not errorcode()
            WasTaken = TRUE
            break
        end
    end
    if WasTaken then return level:benign.
    return level:notify
    
BuildListOfTeachers                     procedure()!,long
    code
    set(Teacher)
    loop
        next(Teacher)
        if errorcode() then break.
        TeacherQ.TeacherID = TEA:TeacherID
        add(TeacherQ)
    end
    return Level:Benign
            
CountCoursesTaughtToStudent             procedure()!,long
x                                           long
    code
    loop x = 1 to records(CourseTakenQ)
        get(CourseTakenQ,x)
        TeacherQ.TeacherID = CourseTakenQ.TeacherID
        get(TeacherQ,TeacherQ.TeacherID)
        if not errorcode() ! should always find a record
            TeacherQ.ClassesTaught += 1
            put(TeacherQ)
        end
    end
    return Level:Benign
SortTeacherListByCountOfCoursesTaughtToStudent  procedure()!,long
    code
    sort(TeacherQ,-TeacherQ.ClassesTaught)
    return Level:Benign
    
NarrowTeacherListToFiveMostAvoided      procedure()!,long
x                                           Long
    code
    loop while not errorcode()
        get(TeacherQ,6)
        delete(TeacherQ)
    end
    return Level:Benign
NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers  procedure()!,long
x                                                                   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. 

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:

    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:

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:

 

Level:Benign (0)

no action, returns Level:Benign

Level:User (1)

displays message, returns Level:Benign or Level:Cancel

Level:Notify (5)

displays message, returns Level:Benign

Level:Fatal (3)

displays message, halts the program

Level:Program (2)

treated as Level:Fatal

Level:Cancel (4)

used to confirm no action taken by User

any other value

treated as Level:Program

 

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. 

  • No labels

40 Comments

  1. Thanks for the honor, Dave.  I'll do my best to live up to it. (smile)

  2. Great article thanks.

    One minor change I would make is to have "else" before the break in the Execute:

     

        execute StepNumber
            Result = GetStudentRecord()
            Result = BuildListsOfCoursesTakenAndNotTaken()
            Result = BuildListOfTeachers()
            Result = CountCoursesTaughtToStudent()
            Result = SortTeacherListByCountOfCoursesTaughtToStudent()
            Result = NarrowTeacherListToFiveMostAvoided()
            Result = NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers()
            Result = RegisterStudentForRandomlyChosenCourse()
            else
            break
        end
    This is defensive programming against StepNumber being accidently/incorrectly changed elsewhere which could put it into an (almost) infinite loop if it was then out of the expected range Eg. StepNumber = 10.  IOW do the break if not in the range 1 to 8, rather than just breaking on 9.
    cheers
    Geoff R 
  3. Good point, Geoff.  When I use this myself, I often put that processing loop itself into a separate routine or local procedure, so that the StepNumber is local to that scope.  Then there's very little chance for it to be misused.  If it has the wrong value, then it's a bug that should be noticed and fixed.  Therefore, I probably wouldn't use the ELSE myself.

  4. yes fair call Mike.

    perhaps we could settle on something like

     

        execute StepNumber
            Result = GetStudentRecord()
            Result = BuildListsOfCoursesTakenAndNotTaken()
            Result = BuildListOfTeachers()
            Result = CountCoursesTaughtToStudent()
            Result = SortTeacherListByCountOfCoursesTaughtToStudent()
            Result = NarrowTeacherListToFiveMostAvoided()
            Result = NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers()
            Result = RegisterStudentForRandomlyChosenCourse()
            break
            else
                    stop('Houston, we've got a problem!')  ! should never happen
        end

    ** of course many will realise the accurate quote is "Houston we've had a problem here" https://en.wiktionary.org/wiki/Houston,_we_have_a_problem

    cheers

    Geoff R

     

    1. Agreed!  That's definitely the best solution.  Even better would be to log that somewhere, so that when your user calls, you have record of it happening. (smile)

       

  5. That is great stuff, guys. Thanks for the enhancement, Geoff!

    Dave

  6. The "Brian Staff Construct" is also worth mentioning.

    Case Level:Notify
    of GetStudentRecord()
    orof BuildListsOfCoursesTakenAndNotTaken()
    orof BuildListOfTeachers()
    orof CountCoursesTaughtToStudent()
    orof SortTeacherListByCountOfCoursesTaughtToStudent()
    orof NarrowTeacherListToFiveMostAvoided()
    orof NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers()
    orof RegisterStudentForRandomlyChosenCourse()
        Message('Oops')
    else
       Cowabunga()
     
    end

    "We don't need no stinkin' loops!"<g>

    1. Jeff,

      Brian is one of the most inventive Clarion developers I know. I don't find that construct all that readable, but it's pretty cool anyway and extremely clever. 

      1. Not disagreeing, but just throwing that out there as a possibility. (smile)

  7. That is a nifty loop, and quite efficient as EXECUTE is very efficient.  However, since there is no compelling reason for any of the called procedures to understand what the step value is, there is really nothing to be gained here over calling each procedure in turn - once.  The cleanup of the code from the original is spot on.

    I use a LOOP like Mike's when one or more of my EXECUTE statements (even if procedure calls) need to know something about the LOOP.  For example, I may be looping through a master file and I need a value from a unique field to fetch matching values from other files (which the procedures fetch for me).  In this case, they need to know what value they must fetch.  In short, gather a lot of related data.

    Or I could simply make a local class using the template interface and get it that way.  This may be my first approach as like Mike, I like my code a bit close to the main action, but too much of that and you are in the same boat - code becomes hard to read.  Which is why I refrain from using MAP structures inside a procedure. That is just too much clutter.

    But what we are really talking about is differences in "code penmanship", so this is all really in the eyes of the beholder. (smile)

  8. Eliminating extra initialization.

    Loop StepNumber = 1 TO 1000

        execute StepNumber

            Result = GetStudentRecord()

            Result = BuildListsOfCoursesTakenAndNotTaken()

            Result = BuildListOfTeachers()

            Result = CountCoursesTaughtToStudent()

            Result = SortTeacherListByCountOfCoursesTaughtToStudent()

            Result = NarrowTeacherListToFiveMostAvoided()

            Result = NarrowCourseNotTakenListToThoseTaughtByFiveMostAvoidedTeachers()

            Result = RegisterStudentForRandomlyChosenCourse()

            break

        end

    while Result = Level:Benign

     

    1. Hey Charlie, why stop at a thousand?  (just joking)

      I must admit I rarely use while or until at the bottom of a loop as I miss having the "end" to terminate the loop but I guess it all comes down to what you are used to and I have used it on occasion.

      As well as eliminating the initialisation code, doing it this way would be marginally more efficient but so small you wouldn't notice it.

      cheers

      Geoff R 

       

       

  9. Russ - The point isn't to tell the called procedure what the step value is. It's to provide order. 

    The thing to be gained (where you say nothing is to be gained) is that the code will stop executing if any of the chain of calls within the EXECUTE fail. The WHILE keeps you from having to do IF/THEN/BREAK constructs for each procedure calll within the code.

  10. I have mixed feelings about the Hanson loop. I absolutely understand what its doing, but it wouldn't be my first choice as loops (to me) imply you might need to or intend repeating an activity or series of actions. This usage is always intended to be passing through the steps once and then out. It's just not "obvious" coding (to me) although I won't swear I haven't done it before.

    This line of code appears in both samples and jumped out at me when I first scanned through the code:  get(TeacherQ,6)

    --David

    1. David -

      I often do 1 iteration loops in order to create a block of logic that can be easily aborted.

      As ugly as it is, here's how I would have coded that:

      Loop 1 time
         If GetStudentRecord() <> Level:Benign
                 !Do Something if desired
                 Break
            end
         If BuildListsOfCoursesTakenAndNotTaken() <> Level:Benign
                 !Do Something if desired
                 Break
            end
             Yada Yada Yada
      end
      In my opinion, this is more maintainable than dancing around EXECUTE when you want to fix something in 2 years.
      EXECUTE is something I never use because stuff doesn't always fit that mold forever. And if you have to change it to use BEGIN/END, you are setting yourself (and the next person to look at it) up for hair pulling.  
      I always select CASE instead of EXECUTE. And I love 1 LOOP loops.
      1. Jeff,  I agree.  I find that the LOOP 1 TIMES cleans up the code considerably because it removes a lot of nested IF statements and their associated indentation.  It is also useful to ensure that the initialization and cleanup in a procedure are always executed by putting them before and after the loop, respectively, and makes it easier to use a single RETURN from a function.

      2. Jeff,

        Seeing other people's coding style is always interesting...even if I would never think of doing it that way. (smile)

        I use EXECUTE sparingly and almost entirely with a POPUP() or backing into something related to day-of-week.

        I'd likely do something similar to what you wrote, but instead of an always-1-Loop construct I'd do a routine or call a procedure with the exits/returns being equivalent to breaking the loop.

        To me, the value of the loop and EXECUTE being used like this is to show all the steps needed in order to fulfill some requirement. In that sense, I like it. But you can accomplish the same thing without the loop and execute scheme.  And if the error handling needs are more complex or perhaps some steps are optional based on prior results, then I think this loop/execute scheme doesn't work as well.

        Hammers for nails, screwdrivers for screws, wrenches for bolts...but that doesn't mean you can't open a can of paint with a screwdriver if you want to.

        --David

        1. If everything was etched in stone, then what fun would that be? (smile)  

      3. Jeff, I also use that sometimes too.

        BTW, the EXECUTE loop is also handy, as sometimes there are a few of those steps that need to be repeated, and I adjust the Stage accordingly to do that.

      4. Hi Jeff

        see this article from Dave some 14+ years ago

        http://archive.clarionmag.com/cmag/v3/v3n3advisor.html

        I use this construct fairly often although I must admit it is largely through fear of using a GOTO.  I think Dave hits the nail on the head where he says I guess I could use a GOTO to jump to the end of the code, but I really don't want a reputation as a GOTO coder.  (If you'll excuse a bad pun I thought Dave's reluctance may have been heightened due to some concern that "Goto considered harmful" was in fact singling him out by referring to his surname).  Anyway I was taught years ago that goto's were verboten.  I have never used a GOTO in Clarion even though I have seen them in other's code (was it in Todd Seidel's PowerBrowse?) and once I read a spirited defence by Carl Barnes

        http://clarion-software.com/index.php?group=13&id=13281

        cheers

        Geoff

        1. The problem I have with Carl's argument is that it focuses on the speed of the code, not on its readability.  Computers are very fast, and it's quite rare for the code to be "too slow".  Most of the time computers are waiting for people, and not vice versa.

          The first goal should always be code readability, because the cost of the software developer is high, and if code is not readable, then it cannot be maintained, and if it cannot be maintained, then it needs to be rewritten.

          That's why I write my code to be conceptually clear and visually clean.  When it's revisited (by me or someone else), I want its intent to be just as clear then as it was when I first wrote it.

          Reusability is my second goal, which is why I break my code into chunks.  Once it's separated like that, I'm more likely to realize that it's something that can be called everywhere.

          Predictability is another goal.  If I have a chunk of code that works dependably, then that's a good thing.  My other procedures can depend on it, so I don't have to repeat those ideas (and potentially do them wrong) in a bunch of place.  It's done once, and hopefully never again.

          Hence the overarching goal of all programmers should be DRY: Don't Repeat Yourself.

          1. GOTO can in fact aid readability.

            I've used GOTO but it's almost always in a source procedure and almost always at the top and used in a series of IFs to check for validity

            IF A=B

                GOTO end: ! No good get out of here

            end

             

            And end: closes files etc.  The alternative is setting a variable and wrapping the main code in an IF or something or using procedures like the loop here. Which is overkill.

            Now this is also a carry over from my COBOL days, and is really just a question of style.

            1. In that situation, I use LOOP 1 TIMES having BREAKs within in it.  Of course, we could argue all day about which is better. (wink)

              1. I just read an article about 10 NASA commandments

                https://jaxenter.com/power-ten-nasas-coding-commandments-114124.html

                where the first is 

                Restrict all code to very simple control flow constructs – do not use goto statements, setjmp or longjmp constructs, and direct or indirect recursion.

                So goto is the first thing banned.

                However now we could start a new conversation because recursion is also banned and sometimes recursion really simplifies things.  I have written some code that I would hate to try to re-write without recursion!  Sure you could could use iteration simulating the stack using a queue (and I recall doing that once in 1987 using a COBOL that did not support recursion) but it is messy and not nearly as clean.

                anyway as Mike says we could all argue about these things until the cows come home

                cheers

                Geoff R

                 

        2. Thanks for the trip back through time, Geoff! I'd completely forgotten about that article. 

          Yes, PowerBrowse made extensive use of GOTOs (none of which would have been necessary had virtual methods been available at that time). I've had to do some work on the PowerBrowse library for a client and it's a momentarily terrifying experience to select everything, hit Ctrl-I, and get a page full of compile errors because all your GoTo labels have left their proper position at column 1! (smile)

  11. Fascinating discussion! I like the idea of a one-iteration loop, and I can see the benefit of it for larger code blocks within the loop. On the other hand, the thing I've found the most useful about the Hanson Loop is it forces me to think in terms of a series of single lines of code expressing intent. That helps me break up my code into smaller chunks and it makes for the shortest possible high-level description of the overall process. 

    As for efficiency, for the kinds of applications I write the speed of basic language constructs is almost never an issue. Those CPU cycles are typically dwarfed by IO or idle user time. My aim is to minimize my personal CPU cycles, which means writing for readability, re-usability, and maintainability. 

    Dave

    1. Yup, primary focus should be on code readability and organization.  Optimization comes later, and only if it's truly too slow.

  12. Where I've had a problem with the one-iteration loop is with code spanning multiple pages/screens.

    To me the loop structure implies the contained code being executed 0 to n times and so it's confusing when I see initialization or cleanup code or whatever that would only make sense executing once or has to always be executed. And then after all the pages/screens I get to a BREAK as the last line of the loop.  I feel cheated that I didn't get to loop back to the beginning. (smile)  Using a WHILE can be a signal its a one-iteration loop with a nicely named variable, but a comment explicitly indicating the looping structure should only be executed once would always be nice.

    If the full one-iteration loop fits on my screen like this example and I can "see" it's totality then I've never had a problem with it...although if it was my code I would still be tempted to convert it to a routine or procedure with the individual calls listed there.

    The "stepping" EXECUTE loop feels a little overly clever to me for just doing a logical set of procedure calls when a bit more code would make the intent obvious. If I used it regularly in this manner it would be obvious, but I'm not seeing an advantage for changing (either to or from.)

    --David

  13. You should never have a chunk of code that spans multiple screens.  Generally my procedures are not more that a dozen lines long.  If it hits 25, then it feels onerously big.  I heard of one fellow who doesn't like his procedures to be more than 4 lines long, although I personally think that's a bit extreme.

    The whole point of the LOOP with the EXECUTE StepNumber is that it forces you to think of your various operations as separate pieces in a whole.  You can glance down that EXECUTE statement, and quickly see the "story" of everything that's going to happen.  Sometimes those procedures could contain a single line of code, but the intent of the code is clear from the procedure name.  It separates the idea from the nitty-gritty details.

    I don't care whether people choose to use LOOP+EXECUTE, or LOOP 1 TIMES, or some other organization construct.  What's important is that they break their code into small, understandable chunks, so that there's not too much noise at the same level.

    Remember that each bite-sized piece tells a part of the story, whether you're at 40,000 feet or examining the individual blades of grass.

    1. The stepping execute does force you to line things out in hopefully logical and obvious steps, but you "should" already be approaching things in this manner. And it does eliminate some code for handling a bad result and I like that side of it.

      I fully understand and agree with the point of having the code in logical and understandable units. But a stepping execute used mainly to order procedure calls is a gray area–the code has been simplified and organized, but at the expense of clarity. >For me< (smile)

      What I would add is that a smaller amount of code does not necessarily equate to better understanding. We tend to recognize the extremes in code size (like the no-more-than-4-lines-per-procedure or a one-iteration loop spread over multiple pages), but increasing or decreasing complexity is more individualistic.  Hitting what I see as a "proper" balance of reducing code size while maximizing understanding (within the constraints of what I have to accomplish here) is part of the fun for me at least.

      --David

    2. >  I heard of one fellow who doesn't like his procedures to be more than 4 lines long, although I personally think that's a bit extreme.

      yes there are extremists everywhere.  We are told we need to be alert but not alarmed.

      I wonder how many levels deep his call structure must be?  

      In a similar vein I once had a friend who worked in a COBOL shop in the 80's that had a standard that IF's could be nested no deeper than three levels.  On one program she was working on she came across a three deep IF statement that on one branch said something like PERFORM 3000-MORE-IFS.  Now that was a useful section name! But it did (superficially) comply with the standards... 

      cheers

      Geoff R

       

       

      1. I know there are devs out there would like to have the entire process laid out as a single long story.  Having to go "somewhere else" blows up their brain.  The problem having everything in one place, though, is that it discourages code reuse.  Many of those inner-snippets of code are potentially independent concepts, that can be managed in a separate library, and not be repeated each time it needs to be used.

        I break my code into small "atomic" chunks, not only so the story makes sense in each place, but also to lead me onto (perhaps not so obvious) opportunities to create those independent callable libraries.

        In the end, I think of my programs like stories, where you have chapters in a table of contents.  Each chapter might be broken up into sections, each section into paragraphs, and each paragraph into cohesive sentences.  Sometimes I'll have footnotes with digressions that can be ignored.  Etc.  Each of these entities could be summarized by a simple description: Why is this here?

        This way you can focus on the relevant task at hand, yet you always realize that you're working in a larger framework of ideas.

  14. Granted.  For me, each story must be at the same level of abstraction within itself.  Mixing error checking code with processing code usually clouds the waters, which is why TRY/CATCH is such a great thing in other languages.

    So the EXECUTE structure is the "list of things that are going to happen", with minimal error checking code to confuse things.  Then each step is in another place with a story of its own.  No story contains too many details to overwhelm your short term memory.

    We all know how frustrating it is when someone tells a story, but keeps digressing to present seemingly irrelevant details.  As my father often said to me (before I learned to self-edit), "I asked you for the time, not how to build the watch!" (smile)

  15. "Mixing error checking code with processing code usually clouds the waters..." is interesting as I tend to notice its absence more than its presence and start wondering if an error has been accounted for.

    Some people prefer seeing more trees in the forest, some want to see fewer trees. What's cool as programmers is we often get to decide how much we see based on how we lay out the design.

    --David

  16. I'm more of a routine guy myself... I find the calling of the procedures interesting though. Normally I'm doing all of this in the same procedure.. calling a routine that executes the routines....

    builddata routine

     

    do GetStudentRecord;if bad;exit.
    do BuildListsOfCoursesTakenAndNotTaken;if bad;exit.
       
    GetStudentRecord routine
    bad=false
    !do something
    !if error();bad=true;exit.

    Something like that.. However, I may try something using multiple procedures... I like the concept. Plus it being in multiple procedures would allow the code to be called from anywhere.
    Ray
    1. Hi Ray

      yes there are certainly numerous ways to do things, including using routines as you outline.

      One thing I would tend to do slightly differently is set Bad to TRUE (rather than FALSE) at the top of the GetStudentRecord routine and then only set it to FALSE as the last line when everything has gone well.  Then you can exit from anywhere in the routine on error without needing to set Bad.  Effectively "guilty until proven innocent".

      >Plus it being in multiple procedures would allow the code to be called from anywhere.

      well not in this case.  It all comes down to scope and these local procedures are defined and visible within the calling procedure only.  You could move them to be a global procedure however then they would not be able to "see" the local data of the calling procedure.  Best to think of them as similar to your routines except 

      • you define a prototype in a map
      • they can accept parameters
      • they can return a value
      • they are called by name rather than DO whatever
      • you RETURN rather than EXIT

      just as with a local routine they can directly use the data in the calling procedure without having to pass it as a parameter.

      cheers

      Geoff R

    2. I've got two problems with that, Ray.

      • Your "Bad" variable is almost like a global within that procedure.  A routine always says "We didn't work", when it might actually want to say "I didn't work, but I'm not failing everything".  These local variables are often confused and abused, which is why I've stopped using them.  Instead, I prefer to declare variables as far down in the chain as possible, close to where they are used.
      • All those ";if bad;exit." lines add lots of noise to the code.  At the very least, I would push those segments over, so that they all line up visually.
  17. I have used the "Hanson Loop" for multi-step web processes. It nicely lays out the logic in one place. I like to show a progress bar because each step can take some time. I use a local Class and each Method updates the progress window text to say what it is doing The class maintains its own Abort flag and other state info, e.g. if the user Cancel then in UpLd. Finally there is no need to tell him we aborted.

        UpLd.Abort=0
        ?Prog{RangeHigh} = 8
        LOOP Prog=1 TO 6
            EXECUTE Prog
            UpLd.PrepareFile()
            UpLd.CheckInet()
            UpLd.Login()
            UpLd.OpenUploadPage()
            UpLd.UploadFile()
            UpLd.TakeResponsePage()
            BREAK      !#7 ? should not get here
            END
        WHILE ~UpLd.Abort
        Prog = 7 ; UpLd.Cleanup()
        Prog += 1 ; UpLd.Done()  !show 100% on window, tell user if failed

    Will I ever figure out this control? Had to restore to html editing.

  18. Carl, I edited your comment to use a code block for the source. Just type left brace then code and choose Code Block from the popup. 

     

  19. I don’t use GOTO very often, but I will use it. I cannot understand the thinking that GOTO makes code very unreadable or untraceable. If I see “GOTO RetryDeleteLabel” usually the label is in view. It is easy to see over there in column 1. I can easily search for it and be 100% sure I have traced where the code is going, not as easy with LOOP.

    Using a GOTO with a Message Retry button is usually faster to code than a LOOP as there is no indenting the code and there is also no worry about making the other buttons BREAK else you get stuck. The desire is to add branching for just one button and GOTO does that.

    Coding a LOOP is just a way of using GOTO (branching) without labels. It can be hard to trace through unless the developer indented the code nicely, and some comments help. If the developer didn’t do that he should be fired.

    Doing CYCLE is a GOTO “Top of Loop” so now you search UP in the code for “LOOP” and then consider the condition “TO, Times, While, Until”. Doing BREAK is a GOTO END+1 so you are looking for the END, or period (you’re fired!) , and there are a lot of ENDs. Or you may be looking for a conditional loop end using UNTIL or WHILE. When you run into the loop’s END (are you sure you have the right end?) that is a GOTO “Top” so hunt down the prior “LOOP”. I’m not anti-loop at all , but tracing a nested Loop with nested IF or CASE can be tedious.

    So IMO LOOP can have GOTO issues of a different type. Mike’s idea of limiting code blocks to less than a visible page makes good sense.

    When you get your ENDs or periods a little off up the compiler is not too helpful. Clarion really could use an “Explicit End” option so it could work like TPLs and require ENDIF ENDCASE ENDLOOP. If they are used the compiler should check them. Be nice if that was a Pragma that could be turned On and Off so if a block of code was screwed up you could turn it On. My usual method is to Comment it all out, compile, uncomment 25% to 50% and retry.

    BTW the ELSE on EXECUTE was my request. I recall it announced by BB at Orlando DevCon which I think was C4 and ABC release. I just checked CW20 help and EXECUTE did not have ELSE.

    Jeff thanks for reminding me about LOOP 1 TIMES, I just used it where I had 6 possible failures!

     

    RetryRemoveLabel
        REMOVE(FileName)
        IF ERRORCODE() > 3 THEN
            CASE MESSAGE('Error while removing ... etc close it ... etc ', |
                  'FILE REMOVE ERROR', ICON:Exclamation, |
                   BUTTON:Abort+BUTTON:Retry+BUTTON:Ignore, BUTTON:Retry)
            OF BUTTON:Abort  ; RETURN Errorcode()
            OF BUTTON:Retry  ; Goto RetryRemoveLabel
            OF BUTTON:Ignore ; Message('warning! ...')
            END !CASE
        END !IF ERROR
    
    !Use loop indents and risks getting stuck
        LOOP
            REMOVE(FileName)
            IF ERRORCODE() > 3 THEN
                CASE MESSAGE('Error while removing ... etc close it ... etc ', |
                      'FILE REMOVE ERROR', ICON:Exclamation, |
                       BUTTON:Abort+BUTTON:Retry+BUTTON:Ignore, BUTTON:Retry)
                OF BUTTON:Abort  ;  RETURN Errorcode()
                OF BUTTON:Retry  ;  CYCLE 
                OF BUTTON:Ignore ;  Message('warning! ...') !fall to break
                END !CASE
            END !IF ERROR
            BREAK
        END !LOOP Remove
    
    

     

    The LOOP does look prettier, but it did take longer to write as I thought about 4 "conditions" and trying to have the fewest breaks possible, plus not get stuck in the loop. I got it down to one BREAK at the end so it is kind of a LOOP 1 TIMES that that allows cycle The GOTO was simpler because I only had to think about one branch. I also consider that I never intend this code to loop, so the odd GOTO is ok.