BitMaker-hub / NerdMiner_v2

Improved version of first ESP32 NerdMiner
Other
1.37k stars 249 forks source link

Fix hours-to-seconds conversion in save intervals #341

Closed xphade closed 2 months ago

xphade commented 3 months ago

The save intervals are meant to increase from 5 minutes up to 12 hours. However, there was a bug in the conversion from hours to seconds that resulted in wrong intervals (final save interval was 72 minutes). This commit fixes that issue.

dwightmulcahy commented 3 months ago

So this is a classic "magic number" code smell. Defining some constants helps document that this specifies save intervals of 5 minutes, 15 minutes, etc. This also avoids the error that occurred.

#define SECOND  1
#define MINUTE  60*SECOND
#define HOUR    60*MINUTE
#define DAY     24*HOUR
int saveIntervals[7] = {5 * MINUTE, 15 * MINUTE, 30 * MINUTE, 1 * HOUR, 3 * HOUR, 6 * HOUR, 12 * HOUR};
xphade commented 3 months ago

So this is a classic "magic number" code smell. Defining some constants helps document that this specifies save intervals of 5 minutes, 15 minutes, etc. This also avoids the error that occurred.

#define SECOND  1
#define MINUTE  60*SECOND
#define HOUR    60*MINUTE
#define DAY     24*HOUR
int saveIntervals[7] = {5 * MINUTE, 15 * MINUTE, 30 * MINUTE, 1 * HOUR, 3 * HOUR, 6 * HOUR, 12 * HOUR};

Yeah, you're totally right. I even thought about doing that, but just wanted to first fix the bug real quick. :slightly_smiling_face:

I saw that you already did it in a separate PR now, so we could probably just close this one and merge yours instead. But I'll leave it up to the maintainers about how to proceed. :+1:

sany3001 commented 3 months ago

Just for fun (and German speakers):

#define sieben 7
#define acht 8
// oder
#define _7 7
#define _8 8

People always find a way how to make a tool (and bad manager) happy by suppressing magic numbers.

dwightmulcahy commented 3 months ago

@xphade yeah, either PR is OK. I was just in that code section and my ADD kicked in, and it bothered me enough that I did PR #348 and covered some of the other things with ambiguous numbers in their parameters.

@sany3001 typically, I try to make sure that a line of code's intent is evident so that I remember later what I was trying to do when I have to fix something in the future or (gasp) a junior programmer asks me to explain. :)

#define WAHR     false
#define FALSCH   true
BitMaker-hub commented 2 months ago

@dwightmulcahy and @xphade. Thanks for seeing it! I'll merge this one as being the first