Return to Answer

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (say up to 10 LOC), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables.

Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw better to uniquely identify your variables).

This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

Your main() function is too long and doing too much. You should probably have another class, Sample that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

 std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

 // Length in elements of type 'T' of statically allocated C++ arrays.
 template<class T, std::size_t N>
 constexpr std::size_t arrayLength(const T (&)[N])
 {
     return N;
 }

 ...

 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const auto count = arrayLength(octaves);

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. That would be less boilerplate and easier to maintain. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (like get/set accessors), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables. I don't recommend it as an optimization either, the gain is in reducing redundant/boilerplate code implicit in the prototype/implementation setup.

Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely identify your variables).

This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

Your main() function is too long and doing too much. You should probably have another class, Sample perhaps, that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

 std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

 // Length in elements of type 'T' of statically allocated C++ arrays.
 template<class T, std::size_t N>
 constexpr std::size_t arrayLength(const T (&)[N])
 {
     return N;
 }

 ...

 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const auto count = arrayLength(octaves);

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (say up to 10 LOC), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables.

Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access.

This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

Your main() function is too long and doing too much. You should probably have another class, Sample that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

 std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

 // Length in elements of type 'T' of statically allocated C++ arrays.
 template<class T, std::size_t N>
 constexpr std::size_t arrayLength(const T (&)[N])
 {
     return N;
 }

 ...

 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const auto count = arrayLength(octaves);

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (say up to 10 LOC), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables.

Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw better to uniquely identify your variables).

This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

Your main() function is too long and doing too much. You should probably have another class, Sample that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

 std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

 // Length in elements of type 'T' of statically allocated C++ arrays.
 template<class T, std::size_t N>
 constexpr std::size_t arrayLength(const T (&)[N])
 {
     return N;
 }

 ...

 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const auto count = arrayLength(octaves);

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (say up to 10 LOC), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables.

Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access.

This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

Your main() function is too long and doing too much. You should probably have another class, Sample that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

 std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

 // Length in elements of type 'T' of statically allocated C++ arrays.
 template<class T, std::size_t N>
 constexpr std::size_t arrayLength(const T (&)[N])
 {
     return N;
 }

 ...

 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const size_t count = arrayLength(octaves);

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (say up to 10 LOC), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables.

Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access.

This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

Your main() function is too long and doing too much. You should probably have another class, Sample that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

 std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

 // Length in elements of type 'T' of statically allocated C++ arrays.
 template<class T, std::size_t N>
 constexpr std::size_t arrayLength(const T (&)[N])
 {
     return N;
 }

 ...

 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const auto count = arrayLength(octaves);