patterncsharpMinor
Simple C# Interest Calculating Program
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)
```
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
Be descriptive
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
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
Putting some of the above together
Using some of the above, you might end up with input code that looks more like this:
With the calling code being:
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
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.