pinToggle library - comments please

Thanks Paul. I just knew that you would comment :slight_smile:

Every class I've ever developed has a name that starts with a capital letter.

I'm glad that you started with an easy one !

Seeing these, I went looking for the corresponding end variables. None to be seen...

There are no end variables, because they are not needed. The _start* variables hold the values as they were when toggling started and are used when restarting the toggling (as opposed to resuming it from where it was stopped)

There's no _startStartStartState...

I don't much like the names of the _start* variables either. Perhaps _initial would be better than _start

I would really expect the constructor to take a pin number, and the on and off periods. I would not expect it to do anything with the data, except for storing it.

That is how I originally wrote it but when experimenting with an array of pinToggle objects I decided to change it so that the pin number could be allocated in a for loop using the init() function. Perhaps I went too far by removing both the pin number and the periods from the constructor. Maybe pin number in the constructor and periods and count in the start() function, which is where I think that they belong so that toggling can be started anywhere in the program.

Someone will think that the current and previous variables can be used to hold all the current and previous counts.

Very true and sloppy variable naming on my part.

Having an array of periods AND two scalar variables holding the same values is just asking for them to get out of sync.

The array is only populated with values by the start() function and the scalar variables are not used in the library so there is little chance that they will get out of step. I could, of course, introduce a _currentPeriod variable and lose the array and index method of managing the current period at the expense of an if/else test when it needs to be changed.

Once again, thanks for the comments