3
\$\begingroup\$

$$ \frac{|x|-|y|}{1+|xy|} $$

How can I reduce the amount of code without losing quality? What are your tips? Your comments on the structure, logic, in general, everything. How to make the type of data entered by the user determined automatically?

 using System;
 class taskTwo {

static void Main() {

    int x, y; // Declare variables
    double z, w;
    Console.WriteLine("Please, enter two the numbers");
   x = int.Parse(Console.ReadLine()); //The user enters
    y = int.Parse(Console.ReadLine());

    if (x == 0 && y == 0)//If two zeros are entered, then the numerator is 
                                                            zero. 
    {
        Console.WriteLine("The numerator and denominator can not be zero");
    }

    else {
        z = Math.Abs(x) - Math.Abs(y);//Know what the numerator is equal to 
        if (z == 0)//If the numerator is zero, then ... 
              {
                    Console.WriteLine("Numerator is zero, division is impossible");
                }
        else {
            w = z / (1 + Math.Abs(x) * Math.Abs(y)); //Final calculation
            Console.WriteLine("W = " + w); //Display the result on the screen
        }
    }


}}
\$\endgroup\$
  • 1
    \$\begingroup\$ You should reduce nesting (even if its bad effects aren't much visible on such short snippet) but I'm afraid to tell you that to improve quality you will increase code size. Error handling has a price to pay even in small tasks. Better "UI" (for example to retry if user doesn't type a number) also increase size. To determine type of input? Easiest way is to pick largest type you're allowed to use (biginteger for N or decimal for R). For an example codereview.stackexchange.com/q/104184/13424 \$\endgroup\$ – Adriano Repetti Jul 6 '17 at 20:54
  • 2
    \$\begingroup\$ Oh and please change title to describe what your code does, every question posted here is to make code slightly better! \$\endgroup\$ – Adriano Repetti Jul 6 '17 at 21:01
  • 4
    \$\begingroup\$ Why would you forbid x and y to be zero? Your expression is well defined for every (x, y) from R^2, (0, 0) included! \$\endgroup\$ – YSC Jul 7 '17 at 8:46
  • 1
    \$\begingroup\$ x and y can be zero, but 1+|xy| cannot. Thus, your numerator cannot be zero. (foornote: "this is stupilated by mathematics" really made me smile, this is kind of cute but not quite credible). \$\endgroup\$ – YSC Jul 10 '17 at 8:42
  • 1
    \$\begingroup\$ @YSC Why can not the numerator be zero? Having a zero in the numerator is a legal math operation that results as zero \$\endgroup\$ – Aleksey Nikolaev Jul 10 '17 at 19:32
1
\$\begingroup\$

How can I reduce the amount of code without losing quality?

Well... there is no quality yet ;-) and actually you are going to gain some code if you want to improve it.

I won't try to prove whether there are numbers that can break this fraction in any way but for the sake of the review let's assume the denominator can be zero.


The first improvement you can make is to create a class and a method for the formula because in a real application you wan't to write some test before you let anyone use it so how about a Calculator? I don't know if this formula has any special name so I just named it Fraction1.

Apart from putting it in a testable calculator you should work on the names. What is z or w? How are those letters related to the formula? If you are splitting it into multiple variables then give them resonable names.

Should it happen that the denominator is 0 then the DivideByZeroException is to be thrown and handeled.

class Calculator
{
    public double CalcFraction1(double x, double y)
    {
        var numerator = Math.Abs(x) - Math.Abs(y);
        var denominator = (1 + Math.Abs(x) * Math.Abs(y));

        if (denominator == 0)
        {
            throw new DivideByZeroException();
        }

        return numerator / denominator;
    }
}

You now have a calculator that works. The next step is to read from the console... but maybe you want to read the numbers until the input is correct? Currently it would crash if you enter a letter. So create a helper for it that will try to parse the input until it's a valid value. Instead of using int for this job you could use double to get more precision:

private static double ReadNumber()
{
    double value;
    while (!double.TryParse(Console.ReadLine(), out value)) { }
    return value;
}

The last step is to put everything together, this is, create the calculator, read the the numbers and calculate the result or handle the exception if it wasn't possible:

void Main()
{
    var calculator = new Calculator();

    Console.WriteLine("Please, enter two numbers");

    var x = ReadNumber();
    var y = ReadNumber();

    try
    {
        Console.WriteLine($"W = {calculator.CalcFraction1(x, y)}");
    }
    catch (DivideByZeroException)
    {
        Console.WriteLine("Cannot calculate fraction1 because the denominator is zero.");
    }
}

You should always try to encapsulate pieces of code that should be tested or can work and be reused independently for example you could copy/paste the ReadNumber method and use it for a different console application or tune it and add additional output about an invalid number without touching any other part. This is what we always try to achieve. Small units that do their job well and can be use like pieces of puzzle to build something bigger, something useful.

\$\endgroup\$
  • \$\begingroup\$ Why am I the only one who upvoted here? This answer is the best one IMO. \$\endgroup\$ – Rick Davin Jul 10 '17 at 16:20
  • \$\begingroup\$ @RickDavin thank you :-) I guess tl;dr or the effect of the first answer with the most upvotes receives the most upvotes. \$\endgroup\$ – t3chb0t Jul 10 '17 at 16:23
  • 2
    \$\begingroup\$ @ t3chb0t In this example, there is no value at which the denominator was zero \$\endgroup\$ – Aleksey Nikolaev Jul 10 '17 at 20:24
  • \$\begingroup\$ @AlekseyBudaev I know ;-) I assumed it just to make a point with the exception. \$\endgroup\$ – t3chb0t Jul 11 '17 at 3:42
9
\$\begingroup\$

There is no integer value for which this undefined. Having a zero in the numerator is a legal math operation that results as zero. having a zero in the denominator would result in an undefined result, but that concern is mitigated by the fact that the absolute value of the product of two integers will be greater than or equal to zero and then you add one to that product before doing the division. In other words, the smallest denominator that the equation could have is one which is legal. So you could remove the if statements

using System;

class TaskTwo {

    static void Main() {
        // Declare variables
        int x, y; 
        double w;

        Console.WriteLine("Please, enter two the numbers");
        x = int.Parse(Console.ReadLine());
        y = int.Parse(Console.ReadLine());


        w=(Math.Abs(x) - Math.Abs(y))/ (1 + Math.Abs(x*Y));

        // Display the result on the screen
        Console.WriteLine("W = " + w);
       }
    }

This should suffice.

\$\endgroup\$
  • 2
    \$\begingroup\$ There is at least one Int32 value that would throw an exception, and there are lots of combinations of Int32 values that would produce incorrect results. \$\endgroup\$ – Rick Davin Jul 6 '17 at 21:21
  • 2
    \$\begingroup\$ Either the numerator or the denominator should be cast to a double, otherwise the division operation will be an integer division, which will result in the loss of data. \$\endgroup\$ – squill25 Jul 6 '17 at 23:51
  • \$\begingroup\$ @Rick Davin More details please \$\endgroup\$ – Aleksey Nikolaev Jul 7 '17 at 4:59
  • \$\begingroup\$ Integer overflow would produce incorrect results. \$\endgroup\$ – Sebastian Redl Jul 7 '17 at 8:14
  • 1
    \$\begingroup\$ @squill25 That may be intended by OP, and if not easiest fix is 1.0 instead of 1. \$\endgroup\$ – 410_Gone Jul 7 '17 at 13:15
6
\$\begingroup\$

You really should improve your code formatting. Code which is formatted well (indentation, concise brackets) is easier to read and understand. I'd also suggest you to improve your variable naming a bit. For example z isn't too descriptive. Instead I'd name it numerator and name w result.

You don't need to declare your variables at the top of your method. You rather should declare them closer to the usage.

Your comments describe what you are doing. It's not what you should be doing. Comments should rather describe why are you doing what you are doing.

Compare my below final formatting to yours

using System;

class TaskTwo {

    static void Main() {

        Console.WriteLine("Please, enter two the numbers");
        int x = int.Parse(Console.ReadLine());
        int y = int.Parse(Console.ReadLine());

        // If two zeros are entered, then the numerator is zero 
        if (x == 0 && y == 0)
        {
            Console.WriteLine("The numerator and denominator can not be zero");
        }
        else 
        {
            // Know what the numerator is equal to
            double nominator = Math.Abs(x) - Math.Abs(y);

            //If the numerator is zero, then ... 
            if (z == 0)
            {
                Console.WriteLine("Numerator is zero, division is impossible");
            }
            else 
            {
                // Final calculation
                double result = nominator / (1 + Math.Abs(x) * Math.Abs(y)); 
                // Display the result on the screen
                Console.WriteLine("Result = " + result);
            }
        }
    }   
}
\$\endgroup\$
4
\$\begingroup\$
  • Math.Abs(x) can throw an exception. If x == int.MinValue, Math.Abs refuses to take its absolute value. Same thing for y, of course. Those inputs could be rejected, but it's probably nicer to just do the right thing. This could be fixed by converting the inputs to long, or to double, or by implementing an unsigned abs. Some of the options make the second part harder.
  • Math.Abs(x) * Math.Abs(y) can easily overflow, in fact it can result in -1 (try x = 3, y = 0x55555555) and then there would be a division by zero. Taking the product modulo 232 (and then reinterpreted as signed) does not look intended here. Converting the inputs to a wider type mostly fixes this, there is some precision loss with double though.

So with conversion to double, especially useful if you actually wanted a floating point division all along:

using System;

class TaskTwo
{
    static void Main()
    {
        Console.WriteLine("Please, enter two the numbers");

        // take ints as input but immediately convert them to doubles
        double x = int.Parse(Console.ReadLine());
        double y = int.Parse(Console.ReadLine());

        double w = (Math.Abs(x) - Math.Abs(y)) / (1 + Math.Abs(x*y));

        Console.WriteLine("W = " + w);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Integer division is pretty pointless. If x or y is zero then the denominator is 1 and you get abs(x) - abs(y); otherwise 1 + abs(x*y) >= 1 + max(abs(x), abs(y)) > max(abs(x), abs(y)) > abs(abs(x) - abs(y)) so since division rounds towards zero the result will always be zero (assuming you've successfully eliminated overflow). \$\endgroup\$ – Peter Taylor Jul 7 '17 at 8:05
0
\$\begingroup\$

Reduce amount of code should not be your objective.
You have some code problems.

That code will perform integer math. Will not get anything after the decimal.

Exclude 0 in the numerator is point less.

It fails to validate input.

I would recommend decimal over double here for edge accuracy cases.

It seems silly to allow negative integer input if you are going to take the absolute value but that is what you are doing.

This should take care of validation and perform double math

class Program
{
    static void Main(string[] args)
    {
        int i = ReadInt();
        int j = ReadInt();
        //works at extremes
        //i = int.MaxValue;
        //j = int.MinValue;
        double a = Math.Abs((double)i);
        double b = Math.Abs((double)j);
        double w = (a - b) / (1D + (a * b));
        Console.WriteLine("answer is " + w.ToString());
    }
    static int ReadInt()
    {
        int readInt;
        bool b = false;
        string s = string.Empty;
        do
        {
            Console.WriteLine("Enter integer");
            s = Console.ReadLine();
            b = int.TryParse(s, out readInt);
            if(!b)
            {
                Console.WriteLine("Not a valid integer");
            }
        } while (!b);
        return readInt;
    }
}
\$\endgroup\$

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.