Go Down

Topic: Trying to make code beginner-friendly -- how's this for style? (Read 773 times) previous topic - next topic

odometer

Some time ago, I made a two-button clock project:
https://forum.arduino.cc/index.php?topic=408565.0
I used a coding style (numbers in BCD, lots of bit-masks) which "feels" right to me, but is very beginner-unfriendly. So, now I wish to build up essentially the same project, in smaller steps, and with a simpler, more beginner-friendly coding style.

This first stage just counts time since the Arduino was most recently powered on (or reset).

Code: [Select]

// robs_clk_pt1.ino
// by odometer  2018-12-06

#include <LiquidCrystal.h>

// declare and initialize variables
uint16_t dd = 0; // number of days
uint8_t hh = 0; // number of hours
uint8_t mi = 0; // number of minutes
uint8_t ss = 0; // number of seconds
uint8_t ticks = 0; // number of ticks
uint32_t lastTickMicros = 0UL; // value of micros() at most recent tick
uint8_t updateFlag = 1; // whether to update the display (0 means no, 1 means yes)
char buf[30]; // buffer for storing a string

// a pretty important constant
const uint32_t ONE_TICK = 100000UL; // this number controls the speed of our clock

// here we specify what pins our LCD is on
//                RS  EN  D4  D5  D6  D7
LiquidCrystal lcd(12, 11,  5,  4,  3,  2);

void setup() {
  // start the LCD going
  lcd.begin(16, 2);
}

void loop() {
  // check if it is time for the clock to tick
  if ((micros() - lastTickMicros) >= ONE_TICK) {
    // make the clock tick forward
    ticks++;
    // space the clock ticks exactly 1/10 second apart
    lastTickMicros += ONE_TICK;
    // take care of all of the timekeeping math
    if (ticks >= 10) {
      // trade 10 ticks for 1 second
      ticks -= 10;
      ss++;
    }
    if (ss >= 60) {
      // trade 60 seconds for 1 minute
      ss -= 60;
      mi++;
    }
    if (mi >= 60) {
      // trade 60 minutes for 1 hour
      mi -= 60;
      hh++;
    }
    if (hh >= 24) {
      // trade 24 hours for 1 day
      hh -= 24;
      dd++;
    }
    // remind ourselves that it is time to update the display
    updateFlag = 1;
  }
 
  // check if it is time to update the display
  if (updateFlag == 1) {
    // update the display
    //
    //   Position: 01234567890123456
    //     Line 0: robs_clk_pt1.ino
    // or, Line 0: So far it's been
    //     Line 1: 00000d00h00m00s0
    lcd.setCursor(0,0);
    // top line of display: alternate messages every 5 seconds
    if ((ss % 10) < 5) {
      lcd.print("robs_clk_pt1.ino");
    }
    else {
      lcd.print("So far it\'s been");
    }
    lcd.setCursor(0,1);
    // bottom line of display: show elapsed time
    sprintf(buf, "%5dd%02dh%02dm%02ds%d", dd, hh, mi, ss, ticks);
    lcd.print(buf);
    updateFlag = 0;
  } 
}



I am not 100% sure about my explanatory comments, though. I want them to be short enough to hold the reader's attention, but not so short as to leave her scratching her head.

The main thing I am having trouble with is that big ol' sprintf() statement. I am not sure how to comment it, or even whether to try.

I am aware that, instead of the sprintf() statement, I could use lcd.print() to print one number at a time. But, let's face it: lcd.print() leaves something to be desired. More details on that "something" here: http://forum.arduino.cc/index.php?topic=334205.0

What should I do?

Robin2

What should I do?
The very first thing is to click Report to Moderator and ask to have your Thread moved to a more appropriate section. The Introductory Tutorials section clearly says that questions should NOT be posted in it.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

WattsThat

Seeing how sprintf is a core function, are you writing a demo program or are you explaining core functionality? Make a decision and stick to it. Trying to be all things to all people is a receipe for ineffective tutorials.

There are tons of tutorials available for core functions. Provide some links and leave it there.

Code: [Select]
uint16_t dd = 0; // number of days
uint8_t hh = 0; // number of hours
uint8_t mi = 0; // number of minutes
uint8_t ss = 0; // number of seconds

IMO, that's not a good way to introduce newbies to variable naming...
Vacuum tube guy in a solid state world

westfw

It needs an introductory big comment at the top saying what the purpose of the program is.

I also don't like your variable names.

Robin2

Some time ago, I made a two-button clock project:

I used a coding style (numbers in BCD, lots of bit-masks) which "feels" right to me, but is very beginner-unfriendly. So, now I wish to build up essentially the same project, in smaller steps, and with a simpler, more beginner-friendly coding style.
What exactly do you want the student to gain from your tutorial?

Is it "how to use an LCD"?

OR,

Is it "how to write code to calculate time"

Whichever it is, it seems to me that the other part is just a confusion.

And I, also, don't like your abbreviated variable names. IMHO long descriptive names make it possible to read a program like story. For example secondsValue rather than ss

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

odometer

What exactly do you want the student to gain from your tutorial?

Is it "how to use an LCD"?

OR,

Is it "how to write code to calculate time"

Whichever it is, it seems to me that the other part is just a confusion.
It's "how to make a rudimentary clock". It's not much of a clock if you can't see what time it is.
Actually, now that I think about it, maybe I should pull out the "display the time" part into its own function.

I know that the code I posted is for a count-up timer rather than a clock. The first step in my tutorial will be to make a count-up timer. In the next step, I will add buttons to allow the time to be set, thus turning it into a clock.

Quote
And I, also, don't like your abbreviated variable names. IMHO long descriptive names make it possible to read a program like story. For example secondsValue rather than ss

...R
I see your point. But I don't want my code to end up like this, either: https://www.youtube.com/watch?v=nJ8Tq5EotaE

Would day, hour, minute, and second work? Or would they conflict with some reserved words or something? I know that min for minutes wouldn't work, because min is used for something else in Arduino-land.

Robin2

I see your point. But I don't want my code to end up like this, either:
Isn't that a bit of an exaggeration?

I see no value in trying to make variable names short - especially as programs get longer.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

Deva_Rishi

I usually try and not reserve separate lines for comments but put them after the condition or the statement
Code: [Select]
    if (mi >= 60) {      // trade 60 minutes for 1 hour
      mi -= 60;
      hh++;
    }
    if (hh >= 24) {            // trade 24 hours for 1 day
      hh -= 24;
      dd++;
    }   
    updateFlag = 1;            // remind ourselves that it is time to update the display
  }
as to make it clear what is the code and that the comments are just that.
To 'Correct' you have to be Correct. (and not be condescending..)

Robin2

Reply #7 brings to mind a preference of mine for indenting comments more than the code - like this
Code: [Select]
     // space the clock ticks exactly 1/10 second apart
lastTickMicros += ONE_TICK;
    // take care of all of the timekeeping math
if (ticks >= 10) {

because then I can easily read the code while ignoring the comments.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

odometer

OK, this is starting to turn into something like this: https://www.bartleby.com/17/1/62.html

Which is why I've settled on this code:
Code: [Select]

// how_long_running.ino
// by odometer  2018-12-10

// This sketch answers the question: "How long have I been running?"
// The answer is shown on a 16x2 LCD character display.

#include <LiquidCrystal.h>

// declare and initialize variables
uint16_t nowDay = 0; // "days" part of elapsed time
uint8_t  nowHr  = 0; // "hours" part of elapsed time
uint8_t  nowMin = 0; // "minutes" part of elapsed time
uint8_t  nowSec = 0; // "seconds" part of elapsed time
uint8_t  nowTick = 0; // "ticks" part of elapsed time
uint32_t lastTickMicros = 0UL; // value of micros() at most recent tick
char buf[30]; // buffer for storing a string

// a pretty important constant
const uint32_t ONE_TICK = 100000UL; // this number controls the speed of our clock

// here we specify what pins our LCD is on
//                RS  EN  D4  D5  D6  D7
LiquidCrystal lcd(12, 11,  5,  4,  3,  2);

void setup() {
  // start the LCD going
  lcd.begin(16, 2);
}

void loop() {
  // check if it is time for the clock to tick
  if ((micros() - lastTickMicros) >= ONE_TICK) {
    // make the clock tick forward
    nowTick++;
    // space the clock ticks exactly 1/10 second apart
    lastTickMicros += ONE_TICK;
    // take care of all of the timekeeping math
    if (nowTick >= 10) {
      // trade 10 ticks for 1 second
      nowTick -= 10;
      nowSec++;
    }
    if (nowSec >= 60) {
      // trade 60 seconds for 1 minute
      nowSec -= 60;
      nowMin++;
    }
    if (nowMin >= 60) {
      // trade 60 minutes for 1 hour
      nowMin -= 60;
      nowHr++;
    }
    if (nowHr >= 24) {
      // trade 24 hours for 1 day
      nowHr -= 24;
      nowDay++;
    }
    // update the display so we can see the new time
    updateTimeDisplay();
  }
}


void updateTimeDisplay () {
  // function to update the display to show the current elapsed time
  //
  // we want the display to look like this
  //   Position: 01234567890123456
  //     Line 0:  days  h  m  s 
  //     Line 1:     0 00:00:00.0
  //
  // on the top line of display, show the units
  lcd.setCursor(0,0);
  lcd.print(" days  h  m  s  ");
  // on the bottom line of the display, we show the elapsed time
  // first, we compose a string showing the elapsed time
  sprintf(buf, "%5d %02d:%02d:%02d.%1d", nowDay, nowHr, nowMin, nowSec, nowTick);
  // then we display that string on the bottom line of the display   
  lcd.setCursor(0,1);   
  lcd.print(buf);
}


(Maybe I'll put in one or two more lines of comments explaining that sprintf statement, but that's about it.)

In which sub-forum should this project go? This sketch is going to be the first in a series in which I build up to making a clock with calendar, hourly chime, and buttons for setting the time, as well as an option to use an external RTC.

Robin2

Which is why I've settled on this code:
You are quite correct to realize that YOU must the final decision.

Nevertheless I find it strange that an item named ONE_TICK actually has a value of 100,000  :)

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

odometer

You are quite correct to realize that YOU must the final decision.

Nevertheless I find it strange that an item named ONE_TICK actually has a value of 100,000  :)

...R
That's because the clock is to tick 10 times per second. Actually, I might be able to justify making it 100 times per second (and changing ONE_TICK to 10000), but well, 10 is what I settled on.

At the very least, the finished clock will need to keep track of half-seconds. When I get to the code for making it chime, you will see why.

Robin2

That's because the clock is to tick 10 times per second. Actually, I might be able to justify making it 100 times per second (and changing ONE_TICK to 10000), but well, 10 is what I settled on.
You missed my point completely - which probably reinforces my point.

It was the name of the constant that I was commenting on, not the chosen value. In my universe calling it millisForOneTick would be more meaningful.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

wildbill

Why is buf global? Shouldn't it be a local in updateTimeDisplay?

ChrisTenone

This just doesn't look like good beginner's code.

"Unix style" types such as uint8_t are more intimidating than byte, and unsigned long. Any abbreviations in variable or function names can be ambiguous, especially for non-English as a first people.

And maybe make the entire sprintf thing something for lesson 2. The F macro might be a good thing to introduce if you do explicit print statements. (Memory management is a beggining Arduino topic.)

And a bit of white space always seems to friendly the structure up a bit. For example:

Code: [Select]

void loop() {

  // check if it is time for the clock to tick
  if ((micros() - lastTickMicros) >= ONE_TICK) {

    // make the clock tick forward
    nowTick++;

    // space the clock ticks exactly 1/10 second apart
    lastTickMicros += ONE_TICK;

    // take care of all of the timekeeping math
    if (nowTick >= 10) {
      // trade 10 ticks for 1 second
      nowTick -= 10;
      nowSec++;
    }

    if (nowSec >= 60) {
      // trade 60 seconds for 1 minute
      nowSec -= 60;
      nowMin++;
    }

    if (nowMin >= 60) {
      // trade 60 minutes for 1 hour
      nowMin -= 60;
      nowHr++;
    }

    if (nowHr >= 24) {
      // trade 24 hours for 1 day
      nowHr -= 24;
      nowDay++;
    }

    // update the display to see the new time
    updateTimeDisplay();
  }


This allows the student to see every part of the code independently from the rest, while maintaining the sequential nature of the flow. I also am not a big fan of using "we" in comments. It's kitschy.
Atmosphere carries combustion vapors to places where they will do good instead of harm - Mike Faraday's 'History of a Candle': https://www.youtube.com/watch?v=6W0MHZ4jb4A

Whoops ::)

Go Up