Setting new lines and adding them to a list to be drawn

Question

I have a method that basically passes in a known drawn line segment in my view and then draws parallel lines n distances away from the passed in original line. This one in particular draws two parallel lines on either side.

It seems to be a lot of repeat code here, so how can I refactor it to a smaller method and avoid redundancy?

public List<Line> DrawTwoParallelLines(Line line)
   {
       List<Line> listOfParallelLines = new List<Line>(); 

       Line parallelLine1 = new Line(); 
       Line parallelLine2 = new Line();
       Line parallelLine3 = new Line();
       Line parallelLine4 = new Line();

       parallelLine1.point1.x =(float)(line.point1.x + 50 * Math.Cos(90));
       parallelLine1.point1.y = (float)(line.point1.y + 0 * Math.Cos(90));
       parallelLine1.point2.x = (float)(line.point2.x + 50 * Math.Cos(90));
       parallelLine1.point2.y = (float)(line.point2.y + 0 * Math.Cos(90));

       parallelLine2.point1.x = (float)(line.point1.x + 100 * Math.Cos(90));
       parallelLine2.point1.y = (float)(line.point1.y + 0 * Math.Cos(90));
       parallelLine2.point2.x = (float)(line.point2.x + 100 * Math.Cos(90));
       parallelLine2.point2.y = (float)(line.point2.y + 0 * Math.Cos(90));

       parallelLine3.point1.x = (float)(line.point1.x + -50 * Math.Cos(90));
       parallelLine3.point1.y = (float)(line.point1.y + 0 * Math.Cos(90));
       parallelLine3.point2.x = (float)(line.point2.x + -50 * Math.Cos(90));
       parallelLine3.point2.y = (float)(line.point2.y + 0 * Math.Cos(90));

       parallelLine4.point1.x = (float)(line.point1.x + -100 * Math.Cos(90));
       parallelLine4.point1.y = (float)(line.point1.y + 0 * Math.Cos(90));
       parallelLine4.point2.x = (float)(line.point2.x + -100 * Math.Cos(90));
       parallelLine4.point2.y = (float)(line.point2.y + 0 * Math.Cos(90));

       listOfParallelLines.Add(parallelLine1);
       listOfParallelLines.Add(parallelLine2);
       listOfParallelLines.Add(parallelLine3);
       listOfParallelLines.Add(parallelLine4);

       return listOfParallelLines;

   }

Answer 1

Do you know about radians?

Math.Cos (and the other trig functions in .Net) are based on radians not degrees.

Math.Cos(90) radians is roughly Math.Cos(5157) in degrees. Which is a bit odd...

There are 2π radians in a circle so 1 degree is π/180. As long as you use the same value the lines will be parallel anyway but I thought it was worth pointing out.

Why do you need the Cos at all?

You have a line from (x1, y1) to (x2, y2) with length L = Math.Sqrt((x1-x2)^2 +(y1-y2)^2).

You know that you need to offset the line by an amount (offset) orthogonal to the current line.

private Line CreateParallelLine(Line target, int offset)
{
    var parallelLine = new Line();
    var xDifference = target.point1.x - target.point2.x;
    var yDifference = target.point1.y - target.point2.y;
    var length = Math.Sqrt(Math.Pow(xDifference, 2) + Math.Pow(yDifference, 2));

    parallelLine.point1.x = target.point1.x - offset * yDifference/length;
    parallelLine.point2.x = target.point2.x - offset * yDifference/length;
    parallelLine.point1.y = target.point1.y + offset * xDifference/length;
    parallelLine.point2.y = target.point2.y + offset * xDifference/length;

    return parallelLine;
}

Edit:

I realised that I've only given you an opportunity to create a single parallel line but I think it's straightforward to call the method more times with different offsets!

You didn't show your Line class/struct but it's worth noting that point1 should be at least PascalCase (Point1) but preferably renamed to something more like: Start and point2 renamed to be End or similar.

As x and y are also public, you should PascalCase them too: X and Y.

Answer 2

I would say that Heslacher's idea to use a copy constructor is a good direction, but that has two arguments, doesn't have a descriptive name, and violates the Do one thing principle as it copies a line and then shifts the copy.

My suggestion is to separate those responsibilities:

Line(Line source)
{
    point1.x = source.point1.x;
    point1.y = source.point1.y;
    point2.x = source.point2.x;
    point2.y = source.point2.y
}

void ShiftHorizontally(float adjustment)
{
    point1.x += + adjustment;
    point2.x += adjustment;
}

So the usages are more readable and understandable:

public List<Line> DrawTwoParallelLines(Line line)
{
    List<Line> listOfParallelLines = new List<Line>(); 

    double cos = Math.Cos(90d);  
    float factor50 = (float)(50 * cos);
    float factor100 = (float)(100 * cos);

    listOfParallelLines.Add(new Line(line).ShiftHorizontally(factor50));
    listOfParallelLines.Add(new Line(line).ShiftHorizontally(factor100));
    listOfParallelLines.Add(new Line(line).ShiftHorizontally(-factor50));
    listOfParallelLines.Add(new Line(line).ShiftHorizontally(-factor100));

    return listOfParallelLines;
}

The name ShiftHorizontally documents what is happening and you are forced to understand only one parameter instead of two.

I also recommend The Clean Coder: A Code of Conduct for Professional Programmers by Robert Cecil Martin to learn more about clean code.

Answer 3

• Instead of calculating Math.Cos(90) 16 times, you should calculate it once and reuse it.

• Instead of assigning like

  parallelLine4.point1.y = (float)(line.point1.y + 0 * Math.Cos(90));  
  

  you could simply use

  parallelLine4.point1.y = line.point1.y;

---

Basically I would add a CopyConstructor with an additional xAdjustment parameter to the Line class like

public class Line
{
    public Line(Line source, float xAdjustment)
    {
        point1.x = source.point1.x + xAdjustment;
        point1.y = source.point1.y;
        point2.x = source.point2.x + xAdjustment;
        point2.y = source.point2.y
    }
    // the rest of your code here
}  

and refactor the shown method to

public List<Line> DrawTwoParallelLines(Line line)
   {
       List<Line> listOfParallelLines = new List<Line>(); 

       double cos = Math.Cos(90d);  
       float factor50 = (float)(50 * cos);
       float factor100 = (float)(100 * cos);

       listOfParallelLines.Add(new Line(line, factor50));
       listOfParallelLines.Add(new Line(line, factor100));
       listOfParallelLines.Add(new Line(line, -factor50));
       listOfParallelLines.Add(new Line(line, -factor100));

       return listOfParallelLines;

   }  

---

A methodname like DrawTwoParallelLines implies that there would be some drawing action. So better rename it to something which is more clear so that Bob Sam the maintainer won't be surprised by the code inside the method.

Answer 4

Design

This is a heavily-hardcoded function, as RobH and Heslacher have mentioned. There's no way to change the distance between the lines. Consider parameterizing this function so that it is more flexible for future use.#

Secondly I recommend returning a collection interface, depending on the functionality you wish to allow users of this function. I would typically recommend IEnumerable<Line> which means it can be iterated over, but cannot be modified, since you're expecting users to execute this method to create new parallel lines, but it depends on the rest of your implementation and intentions.

Style

As with all style pointers, this is optional, but I do recommend it.

Use implicit typing (the var keyword) when the right-hand side of a declaration makes the type obvious.

e.g.

List<Line> listOfParallelLines = new List<Line>();

Should be

var listOfParallelLines = new List<Line>();

Naming

Avoid putting the type of a variable in its name. That's kind of what the type declaration is for, and if you change the type later (say, to a Collection, or IEnumerable) and you forget to rename the variable, it will make for some confusing code.

Instead I would recommend:

var parallelLines = new List<Line>();