Use enum flags instead of an int bit operation

[hansmbakker]

The code becomes more readable and better typed if you use enum flags instead of the int values construction.

See https://msdn.microsoft.com/en-us/library/cc138362.aspx#Anchor_0 for more info.

[larsblumberg]

I want to stick with the "one write options argument eats it all" approach. While fade transition and writing w/o response are from two different domains, it's an SDKs job to keep things simple and not over-engineered.

[hansmbakker]

Ok, I can do it for the sake of getting this branch merged.

I'm really not meaning to be rude, but I still do not believe in this "keeping things simple" because I believe programmers do not need to be treated as living in a world where maths doesn't get more difficult than 1+1. I do agree that an SDK should be as simple as possible, but it should not be oversimplified to a level where there is a risk of resulting in odd design choices.

A very concrete example is that for me as a programmer it is counter-intuitive to mix variables from different domains. So for me having the combined write-options is not simpler.

On the other hand - I think that restructuring these parameters (for example like in my example) improves the readability of the code. Not only by grouping the parameters by their purpose, but also by reducing the number of parameters.

There is a good practice to keep the number of parameters in a function under a certain level (see this stackoverflow) for the sake of readability and code structuring. I won't say that 4 is too much and 3 is not; however it is something to keep in mind.

[hansmbakker]

I committed the name change because having a plural name for a bitflag enum is a good practice.

I decided to not inline the variables since I think this makes the code very difficult to read (for many people lines 165-167 probably are magic already and if we mix the flag reading there, a lot of things will happen in a single line.

[SeidChr]

Just 2 Points:

I like when things are named after what they are, but i usually avoid putting the project name again in front of it. The typename of the write options seems to be quite long due to this.

Secondly it is not a write option when you control the fade transition of leds. It's display options and should go together with the intervall. Alternatively combine it with the other options and remove the “write“ from the name.

[larsblumberg]

@wind-rider Thanks!

_nuimoController?.DisplayLedMatrixAsync(new NuimoLedMatrix(matrixString), displayInterval, (int)NuimoLedMatrixWriteOption.WithFadeTransition);

in the readme needs to be changed to

_nuimoController?.DisplayLedMatrixAsync(new NuimoLedMatrix(matrixString), displayInterval, NuimoLedMatrixWriteOptions.WithFadeTransition);

with this PR, right?

@Vittel: Do you suggest to rename it to DisplayOptions? Wouldn't it be better to tell where this enum comes from? E.g. NuimoDisplayOptions?

[hansmbakker]

@Vittel you said:

It's display options and should go together with the intervall.

To me this sounds like you support the idea that these parameters should be restructured. If I misunderstand what you meant then please let us know

[SeidChr]

• Yes they should be restructured. Just correct me if i got it wrong that those single Option class controls the fade transition on the device as well as the answer behavior of the device back to the host. Then it's a clear restructure candidate.
• No, they do not necessarily need to be renamed. I've had a quick look at the rest of your classes, and i have seen the Nuimo prefix in most places. This seems to be a common pattern in your library, so i wouldn't touch it for now. Just in general, it's a duplication of the namespace. The (classically spoken) ideal namespace would be something like “Senic.Nuimo“. If you add the type name it becomes more obvious: “Senic.Nuimo.NuimoDisplayOptions“ or “NuimoSDK.NuimoDisplayOptions“.

Just to make it clear: this is not a general rule, just my practice. I will still call a controller StuffController. Just normally the context is clear enough to know which project is used, and if not, you can always prefix the typename with your namespace.

TL;DR: keep the Nuimo prefix.

[larsblumberg]

Thanks for this PR! Does the change suggest to update the README.md

from

_nuimoController?.DisplayLedMatrixAsync(new NuimoLedMatrix(matrixString), displayInterval, (int)NuimoLedMatrixWriteOption.WithFadeTransition);

to

_nuimoController?.DisplayLedMatrixAsync(new NuimoLedMatrix(matrixString), displayInterval, NuimoLedMatrixWriteOption.WithFadeTransition);

[hansmbakker]

@larsblumberg it does now.

[larsblumberg]

Great, thanks!