7
\$\begingroup\$

I try to program a stopwatch/countdown clock in FORTH with Gforth (and using Gforth-specific words).

I'm a complete beginner and the following code is the basic stuff (going to add an alarm function / countdown option and more).

For now the stopwatch counts from 00:00:00 (hh:mm:ss) up to 24:00:00 and

  • pressing space pauses the clock, pressing it again resumes the clock
  • pressing j/k sets the clock back/ahead one minute
  • pressing J/K sets the clock back/ahead one hour
  • pressing q quits the program

Does my code follow best practices? Is it bad style for words like move-clock-seconds-back to return flags because they will be used in a begin ... until statement?

1000000 constant million

: sextal  ( -- )
   6 base ! ;

: hhmmss.  ( ud --  )
   drop million /  0 <# decimal # sextal # [char] : hold decimal # sextal # [char] : hold decimal # sextal # decimal #> TYPE ;

: pause-clock  ( ud -- ud f )
   utime
   begin
      50 ms
      key? if
         key bl = ( pause is released )
      else
        false ( clock keeps pausing )
      then
   until
   utime d- d- 
   false ;

: move-clock-seconds-ahead  ( u ud -- ud f )
    million * 0 d-
    false ;

: move-clock-seconds-back  ( u ud -- ud f )
    million * 0 d+
    utime dmin
    false ;

: get-elapsed-time  ( ud -- ud )
    2dup
    utime 
    2swap d- ;

: 24-hours-elapsed?  ( ud -- f )
    get-elapsed-time ( elapsed time fits in single integer )
    drop 24 60 * 60 * million * u> ;

: run-clock  ( -- )
    page ( clears the terminal )
    utime ( returns a ud timestamp in microseconds )
    begin
       50 ms ( sleep for 50 ms )
       get-elapsed-time
       5 0 at-xy ( coordinates where to print output )
       hhmmss.
       key? if
       key case
       bl of pause-clock endof
          [char] j of 60 move-clock-seconds-back endof
          [char] k of 60 move-clock-seconds-ahead endof
          [char] J of 60 60 * move-clock-seconds-back endof
          [char] K of 60 60 * move-clock-seconds-ahead endof
          [char] q of true endof
          false swap ( the char is now on top of the stack, will be dropped by endcase )
       endcase
       else 24-hours-elapsed? if
          CR
          ." 24 hours elapsed."
          true
       else
          false
       then then
    until ;

run-clock

bye
\$\endgroup\$
2
\$\begingroup\$

The other review covers some valuable points, so I'll just concentrate on adding additional suggestions here.

Consider portability

There does not really seem to be a need for microsecond precision for this timer, so I'd suggest that rather than using the non-standard utime, perhaps time&date (which is standard) could be used.

Consider restoring the number base

When I program in Forth, I'm often using it in hex mode. If I used your hhmmss. word, I'd be annoyed that it didn't restore the original base. It only takes a few extra words to store and restore base.

Consider explicitly setting the base

When the program defines million it will be using whatever base had previously been set, which will result in very strange behavior if it's not decimal. Good practice is to explicitly set the base so that this can be a standalone module.

Add comments

There are a few comments within the code, but I'd suggest that each function could have a comment immediately above the definition to describe what it does. The usual convention is to use \ for such comments and to use ( -- ) for stack comments as you're already doing.

Make comments count

Describing what page does in an associate comment isn't very useful, since page is a standard word and should be well understood. Better would be to document what's assumed to be on the stack (semantically, not just how big and how many)

Have each word do just one thing

It's already been mentioned that the drop that begins hhmmss. is very odd and not a good idea. Similarly, ending a number of the other words with false is at best counterintuitive. Generally, I follow the guideline that the only things on the stack are things that are required for the particular word being defined. This makes it up the caller to do whatever stack manipulations are required to put things in the right place. It might make the program slightly longer, but it is very likely to make it a lot easier to interactively debug.

Use more constants

The number of milliseconds in 24 hours could be calculated once, put into a constant and then used instead of calculating it each time through 24-hours-elapsed?.

Fix the stack comments

The stack comments for move-clock-seconds-ahead and move-clock-seconds-back are not correct. Instead of ( u ud -- ud f ), they should both read ( ud u -- ud f ).

Fix minor typos

Correcting the error in one of the comments (pring -> print) and fixing the formatting of the first case (it should be indented with the other cases).

Simplify the logic

Right now, the code contains these lines:

   else 24-hours-elapsed? if
      CR
      ." 24 hours elapsed."
      true
   else
      false
   then then

I think it would be a little more concise to write it instead like this:

   else 24-hours-elapsed? dup if
      CR
      ." 24 hours elapsed."
   then then
|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

A comment in

: get-elapsed-time  ( ud -- ud )

seems wrong. The code leaves the elapsed time on top of the otherwise unchanged stack, so the action is really (-- ud).

The way get-elapsed-time leaves a double results in very unnerving drop in hhmmss. and 24-hours-elapsed?. I strongly recommend to make it leave a single integer instead.

I also recommend to reduce stack manipulations with

: get-elapsed-time
    utime
    2over
    d- drop ;

Is it bad style for words like move-clock-seconds-back to return flags

Yes. It makes it very hard to reuse them in other contexts. Consider (untested)

    begin key dup [char] q <> while
        case
            ....
        endcase
    repeat
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ It probably would be good to rename get-elapsed-time into get-elapsed-seconds because it's the first time seconds are used instead of microseconds, right? \$\endgroup\$ – wolf-revo-cats Feb 22 '17 at 3:07
  • \$\begingroup\$ it's true, that get-elapsed-time leaves the stack unchanged, yet for an error not to occur, it needs the starting time as an ud on the stack. The commenting style in Leo Brodie's Starting FORTH is so that in such a case ud would be stated before --. If it's about change, it would also be 2DUP ( -- d d ) (because 2DUP does nothing to the d-number on the stack) but in reality it's 2DUP ( d -- d d ) in the book. \$\endgroup\$ – wolf-revo-cats Feb 22 '17 at 3:09
  • \$\begingroup\$ I think that : get-elapsed-time ( ud -- ud ud ) would be correct? \$\endgroup\$ – wolf-revo-cats Feb 22 '17 at 3:18
  • \$\begingroup\$ I agree that it should be ( ud -- ud ud ) unless you rewrite it to do something more like ( ud -- ud u ) since the code that uses it always drops the high half anyway. \$\endgroup\$ – Edward Feb 22 '17 at 3:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.