Bisection method solver

Question

I have a simple bisection method solver that I was told it has some problems in design and I need my code to be reviewed, so I was hoping for someone to give me guideline on how to improve my code.

It starts with a delegate:

 public delegate double MyFun (double x) ;

A class to add functions:

 class functions
{
 public static double fun1(double x) => Math.Pow(x,2) - 3;
 public static double fun2(double x) => 5* Math.Pow(x, 3) - 2* Math.Pow(x, 2) + 3 * x - 2;
 public static double fun3(double x) => 2 * Math.Sqrt(x) ;
}

A class that has a function that solves the equation. It has a function that takes 4 input parameters: delegate, start point, end point, guess of solution.

class Bisection
{
 public static double Bisection_method (MyFun fun , double start , double end , double? guess)
 {
 if ( fun(start) * fun(end) > 0 )
 {
 Console.WriteLine("wrong Entry");
 return -1;
 }
 if (fun(start) == 0)
 {
 return start; 
 }
 double avg,tolerance,sign;
 avg = (guess.HasValue) ? guess.Value : ( (start + end) / 2 );
 do
 {
 tolerance = Math.Abs ( fun(end) - fun(start) );
 sign = fun(start) * fun(avg);
 if (sign < 0)
 end = avg;
 else if (sign > 0)
 start = avg;
 avg = (start + end) / 2;
 }
 while ( tolerance > 0.0001 );
 return end;
 }
}

Then the main method:

class Program
{
 static void Main(string[] args)
 {
 MyFun fun1 = functions.fun1;
 double start = 0;
 double end = 500;
 double guess = 10;
 double answer = Bisection.Bisection_method(fun1, start, end, guess);
 Console.WriteLine($"The Root = {answer}");
 }
}

I was hoping for someone to help me how to improve this simple code design and there is also some cases that I need to handle.

• Gerrit0: I'm going to guess the #1 issue that your reviewer had is the names of your functions, without looking what does fun3 do? Pretty hard to answer that...
• Ahmad Adel: it's fun1, I'm sorry, Edited.

Answer 1

There is a of lot room for improvements in your code so let us start straight ahead with your class

functions

• Based on the .NET Naming Guidelines classes should be named using PascalCase casing which isn't the only problem here. Naming things is hard but its much harder to grasp at first glance what a class, method or field is used for if one uses names like function, MyFun or fun1..fun3. If you come back in a few weeks/months to fix a bug or to add a feature you still need to understand quickly what the code is doing which will be harder if you keep these names.

• Math.Pow(x,2) should always be replaced by x * x which will be faster

Bisection

• You are executing fun(start) at least one time but up to four times. By storing the result of the method call inside a variable you code will be easier to read and also be faster. The same applies to fun(end).

• double avg,tolerance,sign; don't do this. Its to hard to read. Always declare one variable per line.

• omitting braces {} is valid in C# but it is dangerous as well because it can lead to hidden and therefor hard to find bugs. But if you decide for yourself to take the risk you should at least be consequent with your style. Right now you are sometimes using braces and sometimes you don't.

• If a value of a method parameter doesn't fit in the range of an expected value one should throw an ArgumentOutOfRangeException or if the value is plainly wrong one should throw an ArgumentException instead of writing to the console and returning a magic number (-1).

• Numbers used like tolerance > 0.0001 should be stored in a meaningful named constant so every reader of the code exactly knows what 0.0001 represents.

Answer 2

Heslacher's review covers most of what I would say, but there are a couple of things to add about the main loop:

 do
 {
 tolerance = Math.Abs ( fun(end) - fun(start) );
 sign = fun(start) * fun(avg);
 if (sign < 0)
 end = avg;
 else if (sign > 0)
 start = avg;
 avg = (start + end) / 2;
 }
 while ( tolerance > 0.0001 );

1. tolerance makes more sense to me as the name of the constant 0.0001 than as the width of the interval under consideration.
2. Why calculate the width of the interval at the start of the loop, before updating start / end? There may be valid reasons, but if so they should be commented because it's a surprising place to calculate it.
3. I think there's a bug. If avg is exactly the root, sign will be zero, so neither end nor start will change, locking the execution in an infinite loop.

• Rick Davin: What OP calls tolerance should be renamed range. Along with adding a Tolerance constant the final loop condition would become while (range > Tolerance);.