HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Simple C# Interest Calculating Program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calculatingsimpleinterestprogram

Problem

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)

Solution

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(ref T value, T max, T min, string inputName) where T : IComparable
{
    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)
        {
            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.

Code Snippets

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;
    }
}
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;
    }
}
getCalculationParametersFromUser();
if (interest == 0 || principal == 0 || numYears == 0)
{
    Console.WriteLine("Program will now exit...");
    break;
}

Context

StackExchange Code Review Q#135265, answer score: 5

Revisions (0)

No revisions yet.