7
\$\begingroup\$

Sorry if it's a bit long... I tried to make it modular with methods, not sure if I overcomplicated it

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Learning
{
    class Program
    {
        public static decimal principal;
        public static decimal interest;
        public static int numYears;
        private static string source;
        private static bool quitting;
        private static string consoleInput;

        static void Main(string[] args)
        {
            while (true)
            {
                getInput();
                if (interest == 0 || principal == 0 || numYears == 0)
                {
                    break;
                }
                printAnnual();

            }
        }

        static bool compareStrings(string input)
        {

            if (string.Equals(input, "exit", StringComparison.OrdinalIgnoreCase) || (string.Equals(input, "quit", StringComparison.OrdinalIgnoreCase)))
            {
                return true;
            }
            else
            {
                return false;
            }
        }




        static void getInput()
        { 
            principal = getPrincipal();
            if (principal == 0)
            {
                Console.WriteLine("Program will now exit...");
            }
            else
            {
                interest = getInterest();
                if(interest == 0)
                {
                    Console.WriteLine("Program will now exit...");
                }
                else
                {
                    numYears = getYears();
                    if (numYears == 0)
                    {
                        Console.WriteLine("Program will now exit...");
                    }
                }
            }
        }

        static void printAnnual()
        {
            Console.WriteLine();
            Console.WriteLine("Principal    = " + principal);
            Console.WriteLine("Interest     = " + interest);
            Console.WriteLine("Number years = " + numYears);

            int years = 1;
            while (years <= numYears)
            {
                principal = decimal.Round(getTotal(principal, interest), 2);
                // while (true)
                //  {
                Console.WriteLine();
                Console.WriteLine(years + "-" + principal);
                years++;
                //  }

            }
        }

        private static decimal getPrincipal()
        {
            decimal principal;
            while (true)
            {
                Console.WriteLine();
                Console.WriteLine("Enter principal, or exit/quit to quit:");
                consoleInput = Console.ReadLine();
                quitting = compareStrings(consoleInput);
                if (quitting == true)
                {
                    return 0;
                }
                principal = Convert.ToDecimal(consoleInput);
                if (principal >= 0)
                {
                    break;
                }
                Console.WriteLine("Principal cannot be negative");
                principal = 0;
            }
            return principal;
        }

        private static decimal getInterest()
        {
            decimal maxInterest = 50;
            decimal interest;
            while (true)
            {
                Console.WriteLine("Enter interest, or exit/quit to quit:");
                consoleInput = Console.ReadLine();
                quitting = compareStrings(consoleInput);
                if (quitting == true)
                {
                    return 0;
                }
                interest = Convert.ToDecimal(consoleInput);
                if (interest <= maxInterest && interest > 0)
                {
                    break;
                }
                Console.WriteLine("Interest cannot be negative or greater than " + maxInterest);
                interest = 0;
            }
            return interest;
        }

        private static int getYears()
        {
            int numYears;
            while (true)
            {
                Console.WriteLine("Enter number of years, or exit/quit to quit:");
                consoleInput = Console.ReadLine();
                quitting = compareStrings(consoleInput);
                if (quitting == true)
                {
                    return 0;
                }
                numYears = Convert.ToInt16(consoleInput);
                if (numYears > 0)
                {
                    break;
                }
                Console.WriteLine("Error! You cannot have a negative number of years");
                numYears = 0;
            }
            return numYears;
        }

        private static decimal getTotal(decimal principal, decimal interest)
        {
            decimal interestAmount = principal * (interest / 100);
            return principal + interestAmount;
        }

    }
}
\$\endgroup\$
  • \$\begingroup\$ You should use PascalCase for your methods in C#. It should be GetInput instead of getInput (as it would be in java). That is standard in C# as you can see with Console.WriteLine. The members in your code is camelCase as they should be eg. interestAmount. \$\endgroup\$ – Bent Jul 19 '16 at 13:12
5
\$\begingroup\$

Clean up after yourself

Commented out code creates noise that distracts the reader. When you've got your code working, remove any commented out code. If you feel the need to retain it, use source control to maintain different versions of the source.

You've also got an unused field declared source. Again, if you're not using it, get rid of it.

Be descriptive

getInput isn't a very descriptive name. It's a wrapper method for getting the calculation variables, so maybe getCalculationParametersFromUser would be better. compareStrings is really checking for isQuitOrExit.

Avoid over nesting

Overly nested code gets distracting. One of the easy ways to avoid it is if it's possible to return early. So for example, your getInput could look more like this:

static void getInput()
{
    principal = getPrincipal();
    if (principal == 0)
    {
        Console.WriteLine("Program will now exit...");
        return;
    }

    interest = getInterest();
    if (interest == 0)
    {
        Console.WriteLine("Program will now exit...");
        return;
    }

    numYears = getYears();
    if (numYears == 0)
    {
        Console.WriteLine("Program will now exit...");
        return;
    }
}

Look out for duplication

If you look at the code above, it's clear that there is some duplication. Fetching a value from the user, checking it for 0, exiting if it is. This strongly suggests that some of the code can be refactored to remove the duplication.

Members Vs Locals

Looking at your getXXX methods, they are all declaring a local variables (with the same name as the class field) and then returning it, at which point it is being assigned to the class field. All of the methods are private and have access to the private fields. If they updated them directly, it would allow the flow of your program to be simplified. Alternately, you could change them to be more generic input methods so are driven by parameters.

Putting some of the above together

Using some of the above, you might end up with input code that looks more like this:

static void getCalculationParametersFromUser()
{
    if(!getUserInput(ref principal, decimal.MaxValue, 0, "principal")) return;
    if(!getUserInput(ref interest, 50, 0, "interest")) return;
    getUserInput(ref numYears, int.MaxValue, 0, "number of years");
}    

private static bool getUserInput<T>(ref T value, T max, T min, string inputName) where T : IComparable<T>
{
    while (true)
    {
        Console.WriteLine();
        Console.WriteLine($"Enter {inputName}, or exit/quit to quit:");
        consoleInput = Console.ReadLine();
        if (isQuitOrExit(consoleInput))
        {
            return false;
        }
        value = (T)Convert.ChangeType(consoleInput, typeof(T));
        if (value.CompareTo(min) > 0 && value.CompareTo(max) <= 0)
        {
            return true;
        }
        if (value.CompareTo(min) <= 0)
        {
            Console.WriteLine($"{inputName} must be more than {min}");
        }
        if(value.CompareTo(max) > 0)
        {
            Console.WriteLine($"{inputName} must be less than {max}");
        }
        principal = 0;
    }
}

With the calling code being:

getCalculationParametersFromUser();
if (interest == 0 || principal == 0 || numYears == 0)
{
    Console.WriteLine("Program will now exit...");
    break;
}

Naming

As an aside, if you haven't already you might want to read some of the C# naming conventions. Although they generally only apply public properties/methods, it's fairly unusual to see even private methods that don't use Pascal casing for their names.

\$\endgroup\$
4
\$\begingroup\$

I am not an experienced C# programmer (or any sort of programmer) myself, but here are my two cents:

Modularity

You mention you're using methods to make it more modular. While this is certainly valid, it is not what the language is designed to do. C# is an Object Oriented language and modularity is achieved by separating related data and functionality into separate classes. A typical Main() method in a program like yours would be something like this:

static void Main()
{
    string input = Console.ReadLine();
    var calc = new InterestCalculator(input); // using var because the type is obvious
    Console.WriteLine(calc.Annual);
}

Even more ideally, you should only pass an int to InterestCalculator. Because an InterestCalculator should only calulcate interest, not parse strings. A method like int.TryParse would be helpful here.

To emphasize the point: the way C# achieves modularity is by having different objects do different things. So the Console object handles reading and writing, the Program object handles running the program, and the InterestCalculator object handles, well, calculating interest. It seems a bit too much for a small program like this, and it is, but it is a lot more extensible and adjustable later. Or you could have something like an InterestPrinter that takes an InterestCalculator as input.

You could even create a constructor for InterestCalculator that takes the necessary values for it to function, and have it be the only constructor. Something like, and excuse my ignorance of interest rates here, InterestCalculator(decimal principal, decimal interest, int years).

Naming Methods

The names for your methods are not descriptive at all. Bordering on cryptic. Take this for example:

    static bool compareStrings(string input)
    {

        if(string.Equals(input, "exit", StringComparison.OrdinalIgnoreCase) || (string.Equals(input, "quit", StringComparison.OrdinalIgnoreCase)))
        {
            return true;
        }
        else
        {
            return false;
        }
    }

Aside from the fact that most of this construct is redundant (you should just return the condition), this is a method that is called compareStrings but only takes one string. See the problem? I cannot tell, from calling this method, what is it comparing it to. Maybe a name like VerifyExit would be better, but barely. You're also hard-coding the values "exit" and "quit", but what if you wanted later to add more possible exit codes? This is not just about this method in particular but in general: don't hard code values because it is more difficult to maintain the code later.

Global variables

Global variables are the devil !! Your methods are using variables that are not passed to the method explicitly. This is just as cryptic as confusing names.


This is what jumped out the most to me. Matt's answer also points out some off constructs in your code: while(true) loops are simply not idiomatic. Using internal variables with the same name as outer variables in confusing.

Also: access modifiers. Since all your code is in one class (it shouldn't be .. but eh), you don't need the public modifier. You also, arguably, don't need the private modifier because it is default and redundant. If you have multiple classes and you intend to have the methods in one class accessible to another then, and only then, use public.

\$\endgroup\$
3
\$\begingroup\$

Looking at your code I see the following.

  1. Why are principle, interest and numYears public?
  2. There are no comments in the code.
  3. I think it would be better if most of the code was in a class and main was in Program on its own.
  4. There is too much static stuff.
  5. Its a matter of preference, but you could shorten your if statements.
if (condition)
  return true;
else
  return false;

return (condition);
  1. You are hiding class members by using variables of the same name. It causes confusion and can introduce problems so isn't a good idea.

  2. In getPrinciple() you have a while(true) loop. I don't think this is a good idea, is there a way you could refactor it so you removed the return and break statements and set the while condition instead?

  3. In getInterest you have maxInterest which seems to be a constant value. If it is then it would make the code clearer if you marked it as const.

  4. getInterest/principle and years are almost identical. You could extract the common elements into a new function and reduce you line count.

  5. in printAnual() you have a while loop that should be a for loop. Its a long time since I though about the reason why, but for loops are quicker (I think). Also you will remove another two lines of code.

\$\endgroup\$
  • 4
    \$\begingroup\$ I notice quite a bit of vague statements and guessing in your answer. Can you clarify, for example, why static is bad, why a while(true) loop is bad, what kind of problems can be introduced by using variables of the same name and why for loops are supposedly quicker? \$\endgroup\$ – Jeroen Vannevel Jul 19 '16 at 10:18
  • \$\begingroup\$ @JeroenVannevel - In this case static is bad because it limits the reusability of the code. There is no reason for those 3 variables to be static apart from their use within main. Taking this program as it is and creating a console and gui version would be quite difficult. If the code was properly encapsulated within a class the task should be much easier. Ideally in OO programming the main function should just create an instance of the object. \$\endgroup\$ – Code Gorilla Jul 19 '16 at 13:51
  • \$\begingroup\$ In general while loops are much easier to make mistakes with than for loops, because the code is spread over more lines, usually. while(true) is a waste of processing power. You are evaluating a constant condition (hopefully it will be optimized away), while (x < 5) would be better than while(true) if (x > 4) break; but IMO for (int x = 0; x < 5; ++x) would be better still since all of the controls for the loop are in one place. \$\endgroup\$ – Code Gorilla Jul 19 '16 at 13:52
  • 1
    \$\begingroup\$ I suggest you add the clarifications to your post to make them visible rather than here. That being said: a for loop gets lowered into a while() by the compiler so the "waste of processing power" makes little sense. \$\endgroup\$ – Jeroen Vannevel Jul 19 '16 at 14:27
  • \$\begingroup\$ @JeroenVannevel - A while (!quitting || principle < 0) is fine. what I said was while(true) was a waste \$\endgroup\$ – Code Gorilla Jul 20 '16 at 8:59

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.