Arduino Forum

Using Arduino => Programming Questions => Topic started by: jgoulder on Feb 07, 2012, 07:43 pm

Title: Seeking constructive criticism on first Arduino library
Post by: jgoulder on Feb 07, 2012, 07:43 pm
Hello, all.

I am new to the world of Arduino.  I picked one up this weekend and have been having fun playing with it.  I decided to create my own library for the MAX517 8-bit DAC.

Since I have never done this before, I was hoping to get some constructive criticism on the libraries' implementation.

It is hosted on GitHub: https://github.com/JordanGoulder/Max517Dac

Thanks for the help!
Title: Re: Seeking constructive criticism on first Arduino library
Post by: dc42 on Feb 07, 2012, 08:19 pm
Hi,

Your code looks generally good to me. You have declared a class so that you can support multiple instances with different addresses (if that is possible for that device), which is good. I have just a few minor comments:

1. The static storage class on the declarations of constants CMD_RESET etc. is redundant. In C++ (although not in C), non-local constant declarations have file scope unless they are declared extern.

2. You are using type int everywhere, however type uint8_t or byte is more appropriate and more efficient for parameters and constants that represent 8-bit values to be transmitted or received. This applies to constants like CMD_RESET and parameters like the ones to mSendCommand.

3. You don't actually need to indicate an empty parameter list using (void), that is how you do it in C. In C++ it is sufficient and more usual to use just () instead. But it really doesn't matter.

4. You're making the assumption that either the user will never want to call setOutput with a bit pattern of all 1s, or that if he does, he will pass it as value 255 not -1. Better I think to make the parameter uint8_t or byte so that this issue doesn't arise.
Title: Re: Seeking constructive criticism on first Arduino library
Post by: jgoulder on Feb 07, 2012, 08:44 pm
Thanks for the very helpful advice dc42.  I have made changes to fix this issues that you mentioned.