Follow

Bad Code 

Now I'm no professional programmer but even I know that this code I wrote is rough. But it works so ¯\_(ツ)_/¯

Bad Code 

@cyberfarmer What language is this? C++?

Bad Code 

@urusan @cyberfarmer
It's embedded C++. Like for an arduino or some other microcontroller.

Bad Code 

@urusan @cyberfarmer
Arduino-flavored C++ is bonkers. Anything undefined is just 0.

Call an uninitialized variable? It's 0.

Divide by 0? The result is 0.

Bad Code 

@cyberfarmer @preflex
So, here's some suggestions, starting with little things that just tidy things up:
* You should create some static constant strings for your menus. You can join the multiple print statements together by just adding more newlines to your longer string. In the code you can then print the whole thing with display.println(MENU1);
* Similarly, 60*1000UL is a constant you can reuse, MILISECONDS_IN_MINUTE perhaps?
* Put the repetitive display code in a separate function

Bad Code 

@cyberfarmer @preflex The biggest change though would be to add another variable to track your page number, instead of using nested while loops to track which page you're on.

So it'd be something like:
page = 1;
while(true){
display_page(page);
//check each button
//either update minutes or change the page
}

Bad Code 

@cyberfarmer @preflex As preflex pointed out elsewhere, delay is a bad idea.

I mean, you have the right instinct, because without any delay it would be incredibly CPU intensive to loop as fast as possible.

The lazy way to do this is to have the lowest delay that doesn't cause resource usage problems. The problems won't be as severe if any delay would be too fast to perceive. Humans can easily perceive anything longer than 100 ms.

The right way to do it is outside my area of expertise.

Bad Code 

@urusan @cyberfarmer

To keep CPU usage down, you can keep shorter delays, but attach an interrupt before the delay and detach it either on the click or at the end of the delay.

Bad Code 

@urusan @cyberfarmer

Here's a fairly simple program that uses the OneButton library and state tracking to handle a multi-level menu.

It's not worried about CPU usage though.

github.com/jakeatcnm/SmartRoom

Bad Code 

@urusan @cyberfarmer

That code is also not good, because of the 32-bit time_t.

But it handles the menus reasonably well.

Bad Code 

@preflex @urusan damn apparently I got somr homework to do. Thanks for the suggestions.

Bad Code 

@urusan @cyberfarmer

Do _not_ attach interrupts to clicks in setup() unless you want everything to go haywire.

Bad Code 

@urusan @cyberfarmer

See:
arduino.cc/reference/en/langua

in short:
attachInterrupt(BUTTON_A, ClickedA, HIGH);
delay(100);
detachInterrupt(BUTTON_A);
if(AButtonClicked){
//do stuff
}
...

void ClickedA(){
AButtonClicked = true;
}

Bad Code 

@cyberfarmer @preflex One more habit you should form is that if you write the same code twice (and especially if you write it 3+ times), then you should break that code off into a function. It's essential for readability.

If you have reservations about the performance of functions, remember that the compiler will often optimize them out of existence, creating the hard to read code you would have written to maximize performance from the clean, easy to understand code you actually wrote.

Bad Code 

@urusan @cyberfarmer
What Urusan said.

Also track the state of your buttons and your depth in the code.

The OneButton library is handy for this if you want to be lazy while having better code.
github.com/mathertel/OneButton

re: Bad Code 

@cyberfarmer why? is wuttwe babby scawed of whiwe twue statements uwu. this wooks funky wunky

if i had more time i would bother to see what it's actually doing

re: Bad Code 

@meowski @cyberfarmer

It's not bad because "while(true)". It's bad because "delay".

re: Bad Code 

@meowski @cyberfarmer
Why is he using it? I dunno. Because he didn't want to write proper timers and state tracking.

Unless he set some interrupt (also a bad idea), the system is non-responsive during delay(). He has to be holding down the button when the delay ends in order to get to the next step.

re: Bad Code 

@preflex @cyberfarmer i don't think we have enough information to tell just how bad this code is. it's out of context

re: Bad Code 

@meowski @cyberfarmer

No. I think we do have enough information. No external context is necessary. There isn't any context where this is good, but there are some contexts where it can work but be finicky and waste lots of time.

Sign in to participate in the conversation
Fosstodon

Fosstodon is an English speaking Mastodon instance that is open to anyone who is interested in technology; particularly free & open source software.