4
\$\begingroup\$

I like to think I'm an intermediate programmer, but I learned to code with the Unity engine so I'm still getting used to not using it, and I've never had someone review my code before. I don't know any other programmers, most people in my town don't even have a computer. I'd appreciate it greatly if you'd be so kind and tell me what you think of my little script here. Rather, be brutal if you want.

My script works fine, but I have some concerns, like the readability of calling a Boolean method with a bunch of parameters in the condition of a while loop for example.

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

namespace Course_Assignment
{
class DistanceCalculator
{
    /// <summary>
    /// This class calculates the distance between two points and the angle of the slope.
    /// </summary>

    static void Main(string[] args)
    {
        float point1X = 0;
        float point1Y = 0;
        float point2X = 0;
        float point2Y = 0;

        float deltaX = 0;
        float deltaY = 0;

        double distance = 0;
        double angle;

        Console.WriteLine("Welcome. This program calculates the distance \nbetween two points as well as their angle.\nPlease type the prompted values.");

        //GetPoints() defines all the point coordinates via user input and 
        //returns true if the data entered was invalid.  
        NewPoints : while (GetPoints(ref point1X, ref point1Y, ref point2X, ref point2Y))
        {
            Console.WriteLine("Invalid data has been entered. Try again.");
        }

        //Getting the change in x and y from point 1 to 2.
        deltaX = point2X - point1X;
        deltaY = point2Y - point1Y;

        //Calc() uses the Pythagorean Theorem to calculate and return       
        //distance, and calculates angle in degrees by multiplying the
        //tangent of deltaX and deltaY.
        distance = Calc(deltaX, deltaY, out angle);

        //Displaying calculations
        Console.WriteLine("\nDistance: {0:N3}\nAngle: {1:N3} degrees\nWould you like to calculate new points? (y,n)", distance, angle);

        //User decides whether or not to do new calculations.
        string newCalc = Console.ReadLine().ToLower();

        if (newCalc.Equals("y") || newCalc.Equals("yes"))
        {
            goto NewPoints;
        }
        else return;
    }

    static bool GetPoints(ref float newPoint1X, ref float newPoint1Y, ref float newPoint2X, ref float newPoint2Y)
    {

        bool invalid = false;

        //Prompts user for coordinates and sets the cursor in front of the 
        //prompt for neatness, then checks validity of user input 
        Console.WriteLine("\nPoint 1\nX Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint1X))
        {
            invalid =  true;
            return invalid;
        }
        Console.WriteLine("Y Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint1Y))
        {
            invalid = true;
            return invalid;
        }

        Console.WriteLine("Point 2\nX Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint2X))
        {
            invalid = true;
            return invalid;
        }
        Console.WriteLine("Y Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint2Y))
        {
            invalid = true;
            return invalid;
        }   

        return invalid;
    }

    static double Calc(float deltaX, float deltaY, out double angle)
    {
        double newDistance = Math.Sqrt((deltaX * deltaX) + (deltaY * deltaY));
        angle = (Math.Atan2(deltaY, deltaX)) * (180 / Math.PI);
        return newDistance;
    }
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd recommend taking a look at the System.Window.Point and System.Windows.Vector structures. \$\endgroup\$ – RubberDuck Feb 6 '17 at 23:01
  • \$\begingroup\$ I would have concerns about user experience - if they got a single character wrong, all previous values are lost. \$\endgroup\$ – wondra Feb 6 '17 at 23:56
  • \$\begingroup\$ Oh wow, I was trying to implement vectors similar to those in Unity but I after some tinkering I thought they didn't exist. Those links are definitely helpful. wondra, excellent point, I should have made it reset the invalid coordinate instead of all of them. I think that's what I wanted to do at first, but the simplicity of going to the beginning of the script won me over and I forgot about user friendliness. \$\endgroup\$ – Kyle Freeman Feb 7 '17 at 1:47
7
\$\begingroup\$
  1. You should avoid using goto statements. Not because it took me a minute to recall what NewPoints : syntax even means, but because goto makes it extremely hard to follow the execution flow. Cases where goto makes code more readable are rare, and yours is not on of those. Just use another while (or do while) loop instead.
  2. Calc is a poor name for a method, because it doesn't say anything about what sort of calculation it performs. It calculates distance you say? Name it CalcDistance then. Oh, wait, it also calculates angle! Maybe CalcDistanceAndAngle then? That doesn't look right... Which is an indication, that you probably need two separate methods: one to calculate angle and one to calculate distance (related to SRP).

    var distance = CalcDistance(...);
    var angle = CalcAngle(...);
    
  3. There is a lot of copy-pasting in GetPoints method. You should probably have a method to query single value. For example

    static float RequestValue(string message)
    { 
        float value;
        while (true) 
        {
            Console.WriteLine(message);
            Console.SetCursorPosition(14, Console.CursorTop - 1);
            if (float.TryParse(Console.ReadLine(), out value)) break;
            Console.WriteLine("Invalid data has been entered. Try again."); 
        } 
        return value;
    }
    

    Then you can just initialize individual points:

    var point1X = RequestValue("\nPoint 1\nX Coordinate: ");
    
  4. I am pretty sure you don't need to move cursor, if you call Console.Write instead of Console.WriteLine.

  5. You are using both floats and doubles in your calculations. You should probably use one or the other.

  6. else return; is unnecessary.

  7. I think it would make more sense for Calc to take actual points as input instead of "delta"-s. Meaning you should move:

    //Getting the change in x and y from point 1 to 2.
    deltaX = point2X - point1X;
    deltaY = point2Y - point1Y;
    

    inside Calc's body.

\$\endgroup\$
  • \$\begingroup\$ Excellent advice, thank you. I'm especially grateful for that SRP link. I encountered that a long time ago but I don't think I really understood it enough at the time for it to make a real impression, and the links on that wiki will help too. \$\endgroup\$ – Kyle Freeman Feb 8 '17 at 17:18
4
\$\begingroup\$
string newCalc = Console.ReadLine().ToLower();
..
if (newCalc.Equals("y") || newCalc.Equals("yes"))

If you are comparing strings like these you don't shouldn't ToLower or ToUpper anything because it's not easy to find that you actually do a case insensitive comparison. I find it's cleaner if you use the case insensitive overload:

var newCalc = Console.ReadLine();
var yes =
   newCalc.Equals("y", StringComparison.OrdinalIgnoreCase) ||
   newCalc.Equals("yes", StringComparison.OrdinalIgnoreCase);

if (yes)

or even shorter with regex:

var yes = Regex.IsMatch(newCalc, "^(y|yes)$", RegexOptions.IgnoreCase);
\$\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.