##Comments
OK I like the first comment.
/*******************************************************************************
*
* Grant Williams
*
* Version 1.0.0
* August 27, 2015
*
* Test Program for Brent's Method Function.
*
* Brent's method makes use of the bisection method, the secant method, and inverse quadratic interpolation in one algorithm.
*
* To Compile Please use icc -std=c++11 if using intel or g++ -std=c++11 if using GCC.
*
********************************************************************************/
You may want to add a copyright notice.
It is over my head the description but I assume somebody with some maths skills could understand it. So it provides some good information.
But there are a lot of other comments in this application are useless. Comments should be used judiciously within code. Bad comments are worse than no comments. You have to maintain comments in the same way you maintain the code and the compiler will not help you with the comments.
Your comments should not explain what the code is doing (the code should be easy to read and by self explanatory). You should explain WHY the code is doing something in a comment.
Example:
fs = f(s); // calculate fs
d = c; // first time d is being used (wasnt used on first iteration because mflag was set)
c = b; // set c equal to upper bound
fc = fb; // set f(c) = f(b)
These comments are all should be removed. The code should also be fixed so that it makes the comments redundant.
fs = f(s);
c = upper_bound;
fc = fb;
You'll notice that I even removed a line. That's because of after much clicking I found it was never used. WTF!
double a; // lower bound
double b; // upper bound
double TOL = 0.0001; // tolerance
double MAX_ITER = 1000; // maximum number of iterations
Much easier to write if you use names that define what they mean.
double lowerBound;
double upperBound;
double tolerance = 0.0001;
double maxIterations = 1000;
##Identifier Names
Using short identifier names is fine for maths. But when you write real applications that need to be maintained for decades you need people to understand your code. You can not rely on comments (because code changes over time and comments and code will drift apart). So you must use good names that explain their meaning for both variables and functions.
Single character names should be banned. A visual inspection of your code did not reveal any use of d
. But I don't trust my eyes. So I had to search your code for occurrences using tools. There were so many false positives in the search because the single character d
appears a lot in the middle of other words.
Pick names that are easy to see and spot and that provide meaning to the context that they are used in.
###Reserved Words.
Certain styles of identifier are reserved for different Jobs. You should understand these reservations.
Identifiers that are all upper case are traditionally reserved for use by the pre-processor.
MAX_ITER
This will get clobbered by the pre-processors if you are unlucky. Pick names that use a combination of upper and lower case characters.
Note it is allso traditional to have type names (User Defined) start with an uppercase letter and all method/variable names start with a lower case letter.
Weather You prefer camel-casing your words or using '' between words is still a very divided subject. The '' is the minority group (but it is a big minority). I am in the group of camel-casing because using '_' is error prone and hard to spot when you get it wrong.
##Declare an initialize in one statement.
std::clock_t start;
start = std::clock();
// It is best to initialize your variables on declaration.
// It makes sure that value are always defined.
std::clock_t start = std::clock();
Also don't declare variables until you need them. Its not much of a problem with POD types that don't run code. But objects that have a type and therefore a constructor will run the constructor at declaration.
Also for readability you want the variable declared just before use so that you can see they type of variable easily and not have to go searching to the top of the function just to find the type.
##Prefer '\n' to std::endl
The std::endl
places a '\n' on the stream then flushes the stream. Manually flushing the stream is usually a bad idea. The code will flush it for you at the best times. You should only manually flush the streams if there is something critical that needs to happen that the code does not understand directly (like the application could be crashing and will not flush the stream).
If you flush streams manually you are likely to find your IO very inefficient.
##Declare loop variables so they are scoped correctly.
for (function = 1; function <= 3; function++)
// More usual to see this:
for (int function = 0; function < 3; ++function)
- Scope the loop variable to the for loop.
This prevent you from leaking scope informaiton.
- Most loops in C/C++ are
<
loops
(Usually because we use zero indexed arrays.)
- Prefer prefix increment
For POD it does not matter. But for user defined types it does. If you change the type of the iterator later then you only need to change the type not change the type and then change all the postfix increments into prefix increments.
Your loop code seems to be an obtuse way of writing:
brents_fun(bradsFunction, -1.5, 0, tolerance, maxIteration);
brents_fun(alexsFunction, -10, 10, tolerance, maxIteration);
brents_fun(johnsFunction, -4, 3, tolerance, maxIteration);
##Self documenting code
The body of this loop is way too long.
for (unsigned int iter = 1; iter < MAX_ITER; ++iter)
{
You should put the body in a function (with a descriptive name). Then your code becomes more self documenting.
Yep Crazy
/*
Crazy condition statement!:
-------------------------------------------------------
(condition 1) s is not between (3a+b)/4 and b or
(condition 2) (mflag is true and |s−b| ≥ |b−c|/2) or
(condition 3) (mflag is false and |s−b| ≥ |c−d|/2) or
(condition 4) (mflag is set and |b−c| < |TOL|) or
(condition 5) (mflag is false and |c−d| < |TOL|)
*/
Also the comment doe not help. The comment basically describes the code. Which is absolutely no help as I can read the code and see what it says. Now when I read the comment and the code I have to verify that they match (which is a pain).
What would be better is to describe WHY there is a conditional tests here. What are you trying to achieve with the test?
In the code rather than have this obtuse expression.
if ( ( (s < (3 * a + b) * 0.25) || (s > b) ) ||
( mflag && (std::abs(s-b) >= (std::abs(b-c) * 0.5)) ) ||
( !mflag && (std::abs(s-b) >= (std::abs(c-d) * 0.5)) ) ||
( mflag && (std::abs(b-c) < TOL) ) ||
( !mflag && (std::abs(c-d) < TOL)) )
Make the expression a function with a name that explains the test.
if (falloutDegredationHasDroppedBelowThreshold(sign, falloutCoeffecient, dropLevel, plank)