Replaced assert statment with error check and AssertionError throw

[topherbuckley]

I noticed that sending incorrect values to the setDutyCycle method would fail silently and just hang at that point if called within an IOIOLooper instance. The error would not print to the logcat until the app was closed. As this took a fair bit of time to figure out, I figured I'd save others the time by replacing the silent assertion errors with a throw that should pop up in the logcat as soon as the error is encountered.

Note, this is not the only location for these assert statements that I think should be replaced, but it is a start. I'm happy to make updates to some of the other assert statements I think could cause confusion for devs, but will wait to hear back on if this is a desired approach or not prior to putting in the effort.

@hannesa2 as you are seem to know a lot more about formal coding practices than I, is this a legitimate method for solving these silent errors or do you recommend another way of approaching this? I'm essentially just following advice given here.

If you think this is a valid approach, I can continue on this branch before merging and add a few more commits with other methods that rely on this silent assert keyword.

[hannesa2]

Thank you for sharing your improvements ! I'm fine with it like it is. Just if you had done it I would

• use a RuntimeException
• a new line for if
• show immediate the wrong value

    public void setDutyCycle(float dutyCycle) throws ConnectionLostException, RuntimeException {
        if (dutyCycle > 1 || dutyCycle < 0)
            throw new RuntimeException("dutyCycle must be between 0 and 1 (current " + dutyCycle + ")");
        setPulseWidthInClocks(period_ * dutyCycle);
    }

Ok, I will wait to merge for more changes as you wrote

[topherbuckley]

Thanks for the tips. I'm working on implementing them now. May I ask why you recommend a RuntimeException over the AssertionError?

Also it is the first I have seen an if statement without curly braces. Are you against them for some reason? I only did a one-liner to make the diff look cleaner, but in retrospective that wasn't very logical.

So this:

    public void setDutyCycle(float dutyCycle) throws ConnectionLostException, RuntimeException {
        if (dutyCycle > 1 || dutyCycle < 0){
            throw new RuntimeException("dutyCycle must be between 0 and 1 (current " + dutyCycle + ")");
        }
        setPulseWidthInClocks(period_ * dutyCycle);
    }

[topherbuckley]

I recall you liking a clean branch history as well, so if you'd like me to rebase and squash/fixup anything after I've made the next few commits, please let me know how you'd like it to look and I can rework it.

[topherbuckley]

Also not sure if I should be including the RuntimeException in the method signatures, javadocs, and or interface method signatures.

Some discussion here which seems to conclude that adding them to the javadoc is a good idea whereas putting them in method signatures just clutters up code. Your thoughts?

[topherbuckley]

I've added a few more commits with the changes that I thought would be useful. There are other assert statements in the codebase, but I'm not familiar enough with some of them, such that I wouldn't want internal functionality to throw RuntimeExceptions if unnecessary. I just made changes to methods I knew a typical dev would use and avoided ones that looked to be for internal use only.

[hannesa2]

Thank you for this pull request !

May I ask why you recommend a RuntimeException over the AssertionError?

Just a personal flavor.

in the method signatures, javadocs

Javadoc is for other people, I never read it. That's why I don't care about it. For me only the code it the source of truth

Are you against them for some reason?

No, you did all fine