10
\$\begingroup\$

I've just started learning C# + MVC. Here's my attempt with MVC:

using System;

namespace QuadraticSolver
{
    class Program
    {
        static void Main(string[] args)
        {
            Controller c = new Controller();

            c.Welcome();

            string keyPressed = null;
            do
            {
                if (keyPressed == "Y")
                {
                    c.DrawLineAfterContinue();
                }

                c.ProgramProper();
                c.Continue();
                keyPressed = Console.ReadKey().Key.ToString();

            } while (keyPressed == "Y");
        }
    }

    class Controller
    {
        public void Welcome()
        {
            View.WelcomeMessage();
        }

        public void DrawLineAfterContinue()
        {
            View.DrawHorizontalLine(true);
        }

        public void ProgramProper()
        {
            Model.Input i = default(Model.Input);
            View.CollectInput(ref i);

            Model.Result r = new Model.Result(ref i);
            View.DisplayResult(r);
        }

        public void Continue()
        {
            View.DisplayContinue();
        }
    }

    class View
    {
        public static void WelcomeMessage()
        {
            Console.WriteLine("Welcome to my Quadratic Solver");
            DrawHorizontalLine();
        }

        public static void DrawHorizontalLine()
        {
            DrawHorizontalLine(false);
        }

        public static void DrawHorizontalLine(bool isLineBreakNeeded)
        {
            string s = null;

            if (isLineBreakNeeded)
            {
                s = "\n";
            }

            Console.WriteLine("{0}----------------------------------", s);
        }

        public static void CollectInput(ref Model.Input i)
        {
            Console.WriteLine("\nGeneral form of quadratic equation:");
            Console.WriteLine("ax^2 + bx + c = 0");

            bool isInputComplete = false;
            bool isAComplete = false;
            bool isBComplete = false;
            bool isCComplete = false;

            do
            {
                try
                {
                    if (!isAComplete)
                    {
                        Console.WriteLine("\nValue of a?");
                        i.A = Convert.ToDouble(Console.ReadLine());

                        while (i.A == 0)
                        {
                            Console.WriteLine("\nValue of a cannot be zero. Please enter a non zero value for a.");
                            i.A = Convert.ToDouble(Console.ReadLine());
                        }

                        isAComplete = true;
                    }

                    if (!isBComplete)
                    {
                        Console.WriteLine("\nValue of b?");
                        i.B = Convert.ToDouble(Console.ReadLine());
                        isBComplete = true;
                    }

                    if (!isCComplete)
                    {
                        Console.WriteLine("\nValue of c?");
                        i.C = Convert.ToDouble(Console.ReadLine());
                        isCComplete = true;
                    }

                    isInputComplete = true;
                }
                catch (System.FormatException)
                {
                    Console.WriteLine("Incorrect input, please enter a valid number.");
                }
                catch (SystemException e)
                {
                    Console.WriteLine("Unhandled exception:\n{0}", e);
                }
            } while (isInputComplete == false);
        }

        public static void DisplayResult(Model.Result r)
        {
            Console.WriteLine("-------------Results--------------");
            if (r.IsReal)
            {
                Console.WriteLine("Roots are real");
                Console.WriteLine("Value of x1: {0}", r.X1);
                Console.WriteLine("Value of x2: {0}", r.X2);
            }
            else
            {
                Console.WriteLine("Roots are imaginary");
                Console.WriteLine("Value of x1: {0} {1} {2}i", r.X1, r.I1.Sign, r.I1.Coefficient);
                Console.WriteLine("Value of x2: {0} {1} {2}i", r.X2, r.I2.Sign, r.I2.Coefficient);
            }
        }

        public static void DisplayContinue()
        {
            DrawHorizontalLine();
            Console.WriteLine("Do you want to continue to solve another quadratic equation?");
            Console.WriteLine("Press 'Y' to continue");
        }
    }

    namespace Model
    {
        struct Input
        {
            public double A { get; set; }
            public double B { get; set; }
            public double C { get; set; }
        }

        class Result
        {
            bool isReal;

            double d = 0, x1 = 0, x2 = 0;

            ComplexPart i1, i2;

            public double D { get { return d; } }
            public bool IsReal { get { return isReal;} }
            public double X1 { get { return RoundAnswer(x1); } }
            public double X2 { get { return RoundAnswer(x2); } }
            public ComplexPart I1 { get { return i1; } }
            public ComplexPart I2 { get { return i2; } }

            public struct ComplexPart
            {
                double coefficient;

                public double Coefficient
                {
                    get
                    {
                        return RoundAnswer(Math.Abs(coefficient));
                    }

                    set
                    {
                        coefficient = value;
                    }
                }

                public string Sign
                {
                    get
                    {
                        if (coefficient > 0)
                        {
                            return "+";
                        }
                        return "-";
                    }
                }
            }

            public Result(ref Input i)
            //constructor with struct Model.Input as parameter by ref
            {
                d = i.B * i.B - 4 * i.A * i.C;

                if (d >= 0)
                {
                    isReal = true;
                }
                else isReal = false;

                if (d == 0)
                {
                    x1 = - i.B / (2 * i.A);
                    x2 = X1;
                }
                else if (d > 0)
                {
                    x1 = (- i.B + Math.Sqrt(d)) / (2 * i.A);
                    x2 = (- i.B - Math.Sqrt(d)) / (2 * i.A);
                }
                else
                {
                    x1 = -i.B / (2 * i.A);
                    x2 = x1;
                    i1.Coefficient = (Math.Sqrt(-d)) / (2 * i.A);
                    i2.Coefficient = -(Math.Sqrt(-d)) / (2 * i.A);
                }
            }

            static double RoundAnswer(double result)
            //helper method to finalise number of digits
            {
                return Math.Round(result, 2);
            }
        }
    }
}
  1. Basically, Main creates a Controller which creates Models to be consumed by View.
  2. Trying to implement Model.Input as struct but ended up in some ugly hacks:
    • default constructor: due to object unassigned compiler error
    • by ref parameter: due to the variable i not passing correctly (all values turned into 0)
    • both problems do not exist when Model.Input is implemented as class, should I just use class for everything?
  3. Input models are to be filled in by controllers? (Did it in View in this example)
  4. How to implement input checking in a better way?
  5. Is it possible to make Model.Result.ComplexPart protected (only accessible through Model.Result) while its variables in Model.Result still able to be exposed by Model.Result's properties? I get a "Inconsistent accessibility" error if I do that.
    I think I got it - if the nested type is protected, they are not meant to be exposed (e.g. returned out of class)?
    And if they are nested & public, they should just stay in the same namespace as the nesting class?
\$\endgroup\$
  • 2
    \$\begingroup\$ Preemptively recommending you read this meta about iterative reviews. \$\endgroup\$ – RubberDuck May 8 '15 at 15:57
  • 1
    \$\begingroup\$ @RubberDuck Thanks a lot for the link, was looking for something like this. It's going to take me some time to assimilate all the great ideas I've received here. \$\endgroup\$ – Lone guardian May 9 '15 at 15:37
9
\$\begingroup\$

Naming:

Names like c, r or s don't mean anything, not to you, not to others. Use meaningful names for your variables. c would become controller and so on.

The var keyword:

From the C# Programming Guide:

The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

Controller c = new Controller();

would become:

var controller = new Controller();

Exception handling for parsing?

I don't think secretly throwing/catching exceptions for parsing numeric input is good practice. Use the Double.TryParse method to handle this:

double b;

do
{
    Console.WriteLine("\nValue of b?");
} while (!Double.TryParse(Console.ReadLine(), out b));

i.B = b;

Throw this in a method and everything will look a lot cleaner:

private static double GetInput(string valueOf, bool positiveNumber = false)
{
    double output;

    do
    {
        Console.WriteLine("\nValue of {0}?", valueOf);
    } while (!Double.TryParse(Console.ReadLine(), out output));

    if (positiveNumber && output == 0)
    {
        Console.WriteLine("\nValue of a cannot be zero. Please enter a non zero value for a.");
        return GetInput(valueOf, true);
    }

    return output;
}

Your whole CollectInput method can now be shortened to this:

public static void CollectInput(ref Model.Input i)
{
    Console.WriteLine("\nGeneral form of quadratic equation:");
    Console.WriteLine("ax^2 + bx + c = 0");

    i.A = GetInput("a", true);
    i.B = GetInput("b");
    i.C = GetInput("c");
}

Avoiding ref:

I personally don't like writing code using references. You could rewrite your code for collecting input like this:

public static Input CollectInput()
{
    Console.WriteLine("\nGeneral form of quadratic equation:");
    Console.WriteLine("ax^2 + bx + c = 0");

    var input = new Input();
    input.A = GetInput("a", true);
    input.B = GetInput("b");
    input.C = GetInput("c");

    //Using the object initializer previous lines become:
    var input = new Input { A = GetInput("a", true), B = GetInput("b"), C = GetInput("c") };

    return input;
}

//Usage:
var input = View.CollectInput();

Same goes for the Result.

Nitpicking:

if (coefficient > 0)
{
    return "+";
}
return "-";

can be rewritten to:

return _coefficient > 0 ? "+" : "-";

This also applies to following:

if (d >= 0)
{
    isReal = true;
}
else isReal = false;

This can be shortened to:

_isReal = _d >= 0;
\$\endgroup\$
  • 3
    \$\begingroup\$ Downvoter care to explain? So I can learn WHY the review is not good in your opinion? \$\endgroup\$ – Abbas May 8 '15 at 16:43
  • \$\begingroup\$ thanks for the suggestions, just a question before I give my code a facelift - I am planning to throw in interfaces and factory to see if I am able to uncouple my components properly. Hence my plan: using var as in var controller = Factory.GetController(); while the factory static method returns IController type will still maintain the uncoupled state right? \$\endgroup\$ – Lone guardian May 9 '15 at 15:47
  • 1
    \$\begingroup\$ Your answer is solid: I do not understand the downvote. (+1) \$\endgroup\$ – Caridorc May 9 '15 at 17:12
6
\$\begingroup\$

Maybe it's a personal preference, but I find calls like this hard to understand.

   public void DrawLineAfterContinue()
   {
       View.DrawHorizontalLine(true);
   }

C# supports named parameters. That's one solution.

View.DrawHorizontalLine(isLineBreakNeeded: true);

The other would be to make the method private and introduce two separate methods that call the private one. Another way to go is to give the arg a default value. The example I showed is the "quick fix", but it's hard to maintain. It'd be better to improve the API a bit.


I'm not sure the controller should be responsible for creating the models outright. That responsibility should be with some provider or factory that gets injected into your controller along with the view.


Your exception handling seems really very good, but I'm not sure that it's entirely appropriate to use exceptions here at all. The code is using them to control program flow. Exceptions are for exceptional and unexpected behavior. I think TryParse would be more appropriate here.


I don't understand the trouble you were having with returning your model from CollectInput. You shouldn't need to pass it by ref.

public static Model.Input CollectInput()
{
    var input = new Input();

     //collect user input

    Return input;
}

If for some reason that doesn't work, I would recommend switching from ref to out. The latter forces you to assign some value to the arg before the method exits.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. I've been doing some reading and I've reached the conclusion of using class instead of struct when I'm passing it around like a football. There are some rules of thumb before one decides to implement struct, and obviously for beginners like me I shouldn't be bothered with optimization at this stage. \$\endgroup\$ – Lone guardian May 9 '15 at 15:51
3
\$\begingroup\$

Program.Main():

Move the if (keyPressed == "Y") to the end of the loop. It will never be taken the first time; it just serves to confuse. The horizontal line is an effect of pressing Y after a calculation. A side benefit is the code doesn't have to initialize keyPressed = null; then either.

View.CollectInput()

If you declare the parameter as out instead of ref then you don't have to initialize i.

But i is a horrible variable name anyway.

You shouldn't need to pass i as ref to Model.Result.

You need to be able to swap out any of model, view, and controller. Right now they're tightly coupled. There should be an interface for each, code to the interface instead of the specific class.

Result

Several of these properties should become auto properties {get; private set;} That way, they don't need a separate backing store.

In Result.Result, the calculation is mathematically unstable if B^2 is close to 4AC. You need to use different math in that case.

\$\endgroup\$
  • \$\begingroup\$ thanks for the answer, I'm planning to use interface and factory in my revised code. \$\endgroup\$ – Lone guardian May 9 '15 at 15:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.