6
\$\begingroup\$

I'm an absolute beginner in programming, and Today I decided to put my knowledge to the test and create a basic c# calculator. I've Done it, but now I'm looking for ways to Make it more shorter and readible. If you Have any suggestions, please give them!

using System;

namespace lol
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Hi! What is your name?");
            string name = Console.ReadLine();

            Console.WriteLine(name + " What do you wanna do");

            Console.WriteLine("Type \"+\" for addition");
            Console.WriteLine("Type \"-\" for subtraction");
            Console.WriteLine("Type \"*\" for multiplication");
            Console.WriteLine("Type \"/\" for division");

            string operation = Console.ReadLine();

           

            Console.Write("Now, Give me number one: ");
            double num1 = Convert.ToDouble(Console.ReadLine());

            Console.Write("Now give me number two: ");
            double num2 = Convert.ToDouble(Console.ReadLine());

            if (operation == "+")
            {
                Console.WriteLine(num1 + num2);
            }
            else if (operation == "-")
            {
                Console.WriteLine(num1 - num2);
            }

            else if (operation == "*")
            {
                Console.WriteLine(num1 * num2);
            }

            else
            {
                Console.WriteLine(num1 / num2);

            }

           

           



        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I like your namespace lol :-P btw, you write four times Type X for addition \$\endgroup\$ – t3chb0t Aug 9 at 17:14
  • \$\begingroup\$ @t3chbot I'm so Sorry that is a mistake on my part. I'll edit the comment. Thanks for pointing it out! \$\endgroup\$ – AhmadDaPenguinz Aug 9 at 17:15
3
\$\begingroup\$

Compactness

  • use a switch statement rather than verbose if-elseif-.. statements
  • get rid of redundant blank lines

Readability

Avoid escaping characters.

Console.WriteLine("Type \"+\" for addition");
Console.WriteLine("Type \"-\" for subtraction");
Console.WriteLine("Type \"*\" for multiplication");
Console.WriteLine("Type \"/\" for division");

You could go with..

Console.WriteLine("Type '+' for addition");
Console.WriteLine("Type '-' for subtraction");
Console.WriteLine("Type '*' for multiplication");
Console.WriteLine("Type '/' for division");

Or..

Console.WriteLine(@"Type ""+"" for addition");
Console.WriteLine(@"Type ""-"" for subtraction");
Console.WriteLine(@"Type ""*"" for multiplication");
Console.WriteLine(@"Type ""/"" for division");

User Experience

If you ever wish to design end-user interfaces, you have to work on your lingo.

You're off to a good start:

Console.WriteLine("Hi! What is your name?");

You slip up slightly next (forgot a question mark):

Console.WriteLine(name + " What do you wanna do");

But then you start pressuring the end-user. You expect a number, not in a moment, but Now! And you are not asking, you are demanding.

Console.Write("Now, Give me number one: ");

Also, one time with a comma and capital after it, and one time the other way around:

Console.Write("Now give me number two: ");

The user may also expect this application to crash on:

  • double num1 = Convert.ToDouble(Console.ReadLine());
  • double num2 = Convert.ToDouble(Console.ReadLine());
  • Console.WriteLine(num1 / num2); // when num2 is 0
  • Any other overflow

And nothing happens when an unknown operator is provided by the user.

The user is not able to verify the results, since the application immediately terminates after the calculation is evaluated.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for the review! I'll work on my hospitality a little. I was too excited to work on my first proper program and I forgot the Question marks. And I'm aware about the Bug, but I'm not sure how to fix it. Could you please Explain how to do it? \$\endgroup\$ – AhmadDaPenguinz Aug 9 at 17:48
  • 1
    \$\begingroup\$ You may add the thrid alternative with @"Type ""+"" for addition"... let's not write a custom format-provider that would add those quotation marks for us just yet ;-P \$\endgroup\$ – t3chb0t Aug 9 at 17:49
  • \$\begingroup\$ The divide by zero bug?: docs.microsoft.com/en-us/dotnet/api/… \$\endgroup\$ – dfhwze Aug 9 at 17:51
  • \$\begingroup\$ @t3chb0t I was thinking of adding string interpolation, but that wouldn't really help readability here :p \$\endgroup\$ – dfhwze Aug 9 at 17:54
  • \$\begingroup\$ ...only if you made it a loop running over operators and their strings. Otherwise I'm on your side. It's just four strings. \$\endgroup\$ – t3chb0t Aug 9 at 17:56
3
\$\begingroup\$

You solution look fine so far (maybe to much empty lines, but that's just peanuts ;)).

If you are intested in a more object oriented approach, I'll show you an alternative solutions. It looks (and is) quite overengineert for such a simple problem - but for real problems it is often a good choice :).

First, abstract the 4 mathematical operations and it's attributes. That allows to define it once and use it gernerally.

Class Operation

public class Operation
{
    public Operation(string name, string op, Func<double, double, double> action)
    {
        this.Name = name;
        this.Operator = op;
        this.Calc = action;
    }

    public string Name { get; }
    public string Operator { get; }
    public Func<double, double, double> Calc { get; }   
}

Note that Func<double, double, double> is a so called delegate that can handle methods like variables. (a, b) => a + b is a lamda expression which is shorthand for private double Add(a, b) { return a + b; }.

Model definition

Here we define the available operations.

// define the available operations
var operations = new Operation[]
{
    new Operation("addition", "+", (a, b) => a + b),
    new Operation("subtraction", "-", (a, b) => a - b),
    new Operation("multiplication", "*", (a, b) => a * b),
    new Operation("division", "/", (a, b) => a / b),
};

Generic processing code

The actually proccing code is generic and uses only the abstractions:

// loop over all operations
foreach (var o in operations)
{
    // and print a generic info message
    Console.WriteLine($"Type \"{o.Operator}\" for {o.Name}.");
}

var opName = Console.ReadLine();
// try to get the operation based on the name
var op = operations.FirstOrDefault(o => o.Operator == opName);

// print error if operation is not available
if (op == null)
{
    Console.Write("Invalid Operator!");
    Environment.Exit(-1);
}

Console.Write("Now, Give me number one: ");
// try to parse the input and print an error if number is not valid
if (!double.TryParse(Console.ReadLine(), out var num1))
{
    Console.WriteLine("Invalid Number!");
    Environment.Exit(-1);
}

Console.Write("Now give me number two: ");
if (!double.TryParse(Console.ReadLine(), out var num2))
{
    Console.WriteLine("Invalid Number!");
    Environment.Exit(-1);
}

// use the delegate of the operation to do the actual calculation
Console.WriteLine(op.Calc(num1, num2));

The nice thing about that approch is that

  • Any changes to the operations (e.g. adding new ones) does not require to understand the processing code
  • The processing code is not redundant (changes has to be done once).

Compare the necessary changes of the following use cases with your solution:

  • Adding a new operation '%'. How many locations must be adjusted?
  • Change the text of the string that "Type "[op]" for [Name]" How many locations must be adjusted?
\$\endgroup\$
  • \$\begingroup\$ Thank you for that! It does seem a little Complicated, but that’s probably Because I’m still a beginner. Thanks again! :) I’ll learn from this! \$\endgroup\$ – AhmadDaPenguinz Aug 9 at 19:02
  • \$\begingroup\$ This isn't necessarily cleaner... and the for without {} is actually terrible :-P \$\endgroup\$ – t3chb0t Aug 9 at 19:57
  • 1
    \$\begingroup\$ It looks like you might have forgotten to include the Operation class... where you use lambdas and does not explain them. I bet OP has no idea what this code is about :-\ \$\endgroup\$ – t3chb0t Aug 9 at 19:59
  • \$\begingroup\$ ups, thanks fo the hint @t3chb0t ;) \$\endgroup\$ – JanDotNet Aug 9 at 20:30
  • 1
    \$\begingroup\$ You are absolutly right. I updated my answer with more explaination. \$\endgroup\$ – JanDotNet Aug 10 at 5:57

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.