5
\$\begingroup\$

I just start learning to code in C#. Is there any other short way to make calculator like this

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

namespace Calculator
{
   class Program
{

    static void Main(string[] args)
    {
        float result = 0;
        //Getting Input of First number
        Console.WriteLine("Enter first number");
        float num1 = float.Parse(Console.ReadLine());

        //Getting Input of second number
        Console.WriteLine("Enter second number");
        float num2 = float.Parse(Console.ReadLine());

        //Getting the math operator
        Console.WriteLine("Enter operator");
        string op = Console.ReadLine();

        switch (op) {

            case "+" : result = num1 + num2;
                break;
            case "-":
                result = num1 - num2;
                break;
            case "*":
                result = num1 * num2;
                break;
            case "/":
                result = num1 / num2;
                break;
            case "%":
                result = num1 % num2;
                break;



        }
        Console.WriteLine("Result = " + result);


    }
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. While it's obvious in this case make sure to add a description of your program to your post when you ask for a review. \$\endgroup\$ – Zeta Apr 16 '18 at 4:42
  • 2
    \$\begingroup\$ This is not an answer, but there a few cases that you're not handling here. What happens if the user chooses 0 as num2 and selects / as the op or enters 23 when asked for the operator? This code needs some input validation. \$\endgroup\$ – Sam Apr 16 '18 at 9:22
  • \$\begingroup\$ @Sam ok i will add those thing to my calculator \$\endgroup\$ – Kartik Apr 16 '18 at 12:27
7
\$\begingroup\$

Is there any other short way to make calculator

Without trying to be facetious, of course there are many different ways to try and do the same thing.

I assume this is a beginner exercise; so I'll review it with that in mind, giving you some tips on what to tackle next as well.

The coding style

  • The variables are acceptably named, but I'd refrain from using shorthand such as op, instead favoring operation. This makes no technical difference but it enhances readability. The current program is simple enough that it's not an issue, but more complex applications will suffer more from these unclear abbreviations.
  • Well structured paragraphs (although I personally would have put the last line on a separate paragraph, but I'm nitpicking).
  • You should really separate some of this logic into separate methods(/classes), but I do concede that the current code is short enough that's it's not a big issue right now.

The code itself

  • On a technical level, everything is as it should be.
  • You have not accounted for nonsensical input. Ask yourself what happens if the user enters "batman" as a number value.
  • Similarly, you have not accounted for non-existing operators. If I enter "batman" as the operator; your application will tell me that the result is 0.
    • As a user, I'm unable to differentiate whether this is correct (e.g. if I entered 5 - 5) or incorrect (if I use an unknown operator). That's not good. Suppose I get 0. That would make me paranoid and require me to backtrack what I gave as input to confirm whether this is a real answer or not.
    • Instead, your application should clearly tell the user that they used an unknown operator, e.g. Error - "batman" is not a known mathematical operation!
  • You've avoided the int division problem.
  • You've abstracted your result into a result variable instead of creating a different Console.WriteLine() for every operation. That is a good application of reusability.
  • You have not avoided division by zero. This is a fairly important fringe case to account for in a calculator application.

Improvements and future challenges

  • Implement a failsafe to prevent division by zero.
  • Implement clear user feedback in case the user enters nonsensical data.
  • The current application performs a single calculation and then exits. What if the user wants to calculate a few different things? A better approach would be to loop the application code until the user enters "exit", at which point the application closes.
  • Although the current code is simple enough that it's not necessary yet, I would suggest you abstract the number entry into a method, so your code looks like this: float num1 = AskForNumber(); float num2 = AskForNumber(); (feel free to use a different method name).
  • String comparison is not as type safe as enum comparison. It would be a good exercise to create an enum Operation { Add, Subtract, Multiply, Divide } and then use that in your switch case instead of directly checking string values. Note: The current code is simple enough that the enum adds more code than it's currently worth, but it is a good exercise and preparation for the future.
  • Since you're learning C#, I would suggest you try an OOP approach, e.g. a Calculator class which implements the calculations, so that your Program class doesn't contain any of the math logic. Again, the current example is simple enough that it's not quite necessary, but it's a good exercise. If you only start using OOP when the code is already complicated; that's not going to help you understand the core principles.
  • Currently, the user is asked for a number, another number, and then an operation. This is not intuitive from the perspective of a user. Notice how every real world calculator asks for the operation before it asks for the second number.
    • It would also be nice if the user could enter the entire calculation in a single go (1 + 2), and your code extract the needed num1, num2 and operation values.
    • Try allowing for multiple operations in a single statement (1 + 2 - 3).
    • Don't forget to apply the correct order of operations! (e.g. 1 + 2 * 3 is equal to 7, not 9).
    • Add functionality for () parentheses to influence the order of operations.
    • More operators: ^ for powers (e.g. 2 ^ 3 = 2³ = 8)

Small remark

Is there any other short way

Shortness is not a measure of good code. If anything, excessively short code is often bad due to inlining several operations and generally lowering code readability.

I know that you meant "simple", i.e. you didn't want a counterexample that is massively more complicated than your exercise.

But I just want to call your attention to the distinction between short code and simple code. These are not freely interchangeable; and more often end up being opposites of each other.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "Currently, the user is asked for a number, another number, and then an operation. This is not intuitive from the perspective of a user. Notice how every real world calculator asks for the operation before it asks for the second number." Not always true. I got though college in the 70's (I'm old) with an HP-10 calculator that used reverse polish notation. In RPN operators follow their operands. \$\endgroup\$ – JazzmanJim Apr 17 '18 at 18:17

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.