5
\$\begingroup\$

This question is a followup to the last one.

I followed RobH's advice and his edge case bug found in my code with this solution:

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 = (float)(target.point1.x - offset * yDifference/length);
    parallelLine.point2.x = (float)(target.point2.x - offset * yDifference/length);
    parallelLine.point1.y = (float)(target.point1.y + offset * xDifference/length);
    parallelLine.point2.y = (float)(target.point2.y + offset * xDifference/length);

    return parallelLine;
}

Now, since his solution only provided a means of creating one parallel line, even though I need to draw several lines, I have this method that draws the parallel lines (it works) but it is long and there is repeat code:

private void DrawParallelLines(object sender, LineDrawingEventArgs e)
    {
        Line referenceLine = CameraObject.GetComponent<DrawLines>().GetLineParams(ViewType + StickLocationEnum.ToString() + linename);
        Line ParallelLine1 = CameraObject.GetComponent<DrawLines>().CreateParallelLine(referenceLine, 50);
        Line ParallelLine2 = CameraObject.GetComponent<DrawLines>().CreateParallelLine(referenceLine, -50);
        Line ParallelLine3 = CameraObject.GetComponent<DrawLines>().CreateParallelLine(referenceLine, 100);
        Line ParallelLine4 = CameraObject.GetComponent<DrawLines>().CreateParallelLine(referenceLine, -100);

        LineParams paramForParallelLine = new LineParams(ViewType + "ParallelLine", Color.green, Color.green, EndPointType.Circle, EndPointType.Circle, lineThickness, endPointThickness);
        Line parallellineWithParams1 = new Line(ParallelLine1.point1, ParallelLine1.point2, paramForParallelLine, ElementType.Line);
        Line parallellineWithParams2 = new Line(ParallelLine2.point1, ParallelLine2.point2, paramForParallelLine, ElementType.Line);
        Line parallellineWithParams3 = new Line(ParallelLine3.point1, ParallelLine3.point2, paramForParallelLine, ElementType.Line);
        Line parallellineWithParams4 = new Line(ParallelLine4.point1, ParallelLine4.point2, paramForParallelLine, ElementType.Line);

        CameraObject.GetComponent<DrawLines>().AddNewLine(parallellineWithParams1);
        CameraObject.GetComponent<DrawLines>().AddNewLine(parallellineWithParams2);
        CameraObject.GetComponent<DrawLines>().AddNewLine(parallellineWithParams3);
        CameraObject.GetComponent<DrawLines>().AddNewLine(parallellineWithParams4);
        CameraObject.GetComponent<DrawLines>().UpdateAllLabels();
    }

What is the best way to refactor this method to remove redundant code?

\$\endgroup\$
4
\$\begingroup\$

One option is to use a foreach loop to iterate over the offsets that you want to use.

private void DrawParallelLines(object sender, LineDrawingEventArgs e)
{
    var linesComponent = CameraObject.GetComponent<DrawLines>();

    var referenceLine = linesComponent.GetLineParams(ViewType + StickLocationEnum.ToString() + linename);
    var lineParams = new LineParams(ViewType + "ParallelLine", Color.green, Color.green, EndPointType.Circle, EndPointType.Circle, lineThickness, endPointThickness);

    foreach (var offset in new[] {50, -50, 100, -100})
    {
        var parallelLine = linesComponent.CreateParallelLine(referenceLine, offset);
        var parallelWithParams = new Line(parallelLine.Point1, parallelLine.Point2, lineParams, ElementType.Line);
        linesComponent.AddNewLine(parallelWithParams);
    }
}

EDIT

What I think Mat is getting at, is that this solution is localised to this usage, and may not be suitable in an application that frequently needs to generate parallel lines. If this is the only place in your app that you create parallel lines, then keep using it (YAGNI).

If however you need to create parallel lines a lot, you could make your implementation a little more general. One way of achieving this could be through extension methods. Consider the following

public static class LineExtensions
{
    public static Line CreateParallelLine(this Line target, int offset)
    {
        ... // Same as in question
    }

    public static IEnumerable<Line> CreateParallelLines(this Line line, params int[] offsets)
    {
        return offsets.Select(o => CreateParallelLine(line, o));
    }

    public static IEnumerable<Line> CreateParallelLines(this Line line, int count, int spacing)
    {
        for (var i = 1; i <= count; i++)
            yield return CreateParallelLine(line, i*spacing);
    }
}

Your foreach then becomes

foreach (var parallel in referenceLine.CreateParallelLines(50, -50, 100, -100))
    ...

Or in the case of generating 300 parallel lines

var positives = referenceLine.CreateParallelLines(150, 50);
var negatives = referenceLine.CreateParallelLines(150, -50);

foreach (var parallel in positives.Concat(negatives))
    ...
\$\endgroup\$
  • \$\begingroup\$ That doesn't entirely work because one of the parallel lines is on top of the reference line. all lines should be displaced from the reference line at some offset. \$\endgroup\$ – Kala J May 21 '15 at 18:32
  • \$\begingroup\$ Good spot. Updated the answer! \$\endgroup\$ – Matt Cole May 21 '15 at 18:43
  • \$\begingroup\$ Maybe I should ask another question but let's say I have two references lines. If I want to pass in the line name of the reference line here, should I do the same thing here twice in an if-else? Or? \$\endgroup\$ – Kala J May 21 '15 at 20:59
  • \$\begingroup\$ If the offset variance is a constant 50, this solution fails to scale and handle 300 lines. \$\endgroup\$ – Mathieu Guindon May 21 '15 at 21:38
  • \$\begingroup\$ @Matt'sMug, why does it fail? In this case, I would only need to draw four parallel lines per reference line. In my case, I will have only two reference lines. In the example above, I only passed in one reference line. \$\endgroup\$ – Kala J May 21 '15 at 22:52
4
\$\begingroup\$

How would you handle 300 parallel lines?

    Line ParallelLine1 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, 50);
    Line ParallelLine2 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, -50);
    Line ParallelLine3 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, 100);
    Line ParallelLine4 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, -100);
    ...
    Line ParallelLine297 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, 950);
    Line ParallelLine298 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, -950);
    Line ParallelLine299 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, 1000);
    Line ParallelLine300 = CameraObject.GetComponent<DrawLines>()
                                     .CreateParallelLine(referenceLine, -1000);

Something's not right isn't it?

I think you need to put words on the concepts involved here:

  • You have a bunch of lines.
  • Lines are drawn in pairs.
  • Lines are offset from the referenceLine by a fixed interval - one line with a positive offset, the other with a negative offset.

You need to write a method that can create n lines and return an IEnumerable<Line> given:

  • A referenceLine.
  • An offsetIncrement for the offset.
  • The n number of pairs of lines you want to create.
  • A LineParams instance.

And then you can just iterate the result and write the AddNewLine() call only once.

\$\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.