5
\$\begingroup\$

I have several tabbed views with a Unity scene in them. What I do between scene is hide the elements in that scene, then render them if the right view is shown.

The method below is the method that renders all the lines previously drawn in my view. The lines can be lines, angles, points, etc..

private void RenderLines()
    {
        try
        {
            // Draw all lines & points
            if (lines != null)
            {
                Vector3 sp;
                Vector3 sp1;
                Vector3 sp2;

                createLineMaterial();
                lineMaterial.SetPass(0);

                // Draw lines
                GL.PushMatrix();
                GL.LoadPixelMatrix();
                GL.Begin(GL.QUADS);
                for (int i = 0; i < lines.Count; i++)
                {
                    // solid line
                    if (lines[i].elementType == ElementType.Line && !lines[i].lineParams.lineName.Contains("IntersectingLine"))
                    {
                        sp1 = camera.WorldToScreenPoint(lines[i].point1);
                        sp2 = camera.WorldToScreenPoint(lines[i].point2);
                        if (lines[i].lineParams.endPoint1Type == EndPointType.HollowCircle)
                        {
                            float endpointRadius = lines[i].lineParams.pointThickness;
                            Vector3 diff = (sp2 - sp1).normalized * endpointRadius;
                            sp1 += diff;
                        }
                        else if (lines[i].lineParams.endPoint1Type == EndPointType.DoubleCircle || lines[i].lineParams.endPoint1Type == EndPointType.DoubleHollowCircle)
                        {
                            float endpointRadius = lines[i].lineParams.pointThickness * doubleCircleRadiusRatio;
                            Vector3 diff = (sp2 - sp1).normalized * endpointRadius;
                            sp1 += diff;
                        }
                        if (lines[i].lineParams.endPoint2Type == EndPointType.HollowCircle)
                        {
                            float endpointRadius = lines[i].lineParams.pointThickness;
                            Vector3 diff = (sp1 - sp2).normalized * endpointRadius;
                            sp2 += diff;
                        }
                        else if (lines[i].lineParams.endPoint2Type == EndPointType.DoubleCircle || lines[i].lineParams.endPoint2Type == EndPointType.DoubleHollowCircle)
                        {
                            float endpointRadius = lines[i].lineParams.pointThickness * doubleCircleRadiusRatio; ;
                            Vector3 diff = (sp1 - sp2).normalized * endpointRadius;
                            sp2 += diff;
                        }
                        DrawLine(sp1, sp2, lines[i].lineParams.lineThickness, lines[i].lineParams.lineColor);
                    }

                    //Draw Angle lines
                    if (enableDrawingAngle && lines[i].elementType == ElementType.Angle)
                    {
                        sp = camera.WorldToScreenPoint(lines[i].point1);
                        sp1 = camera.WorldToScreenPoint(lines[i].point2);
                        sp2 = camera.WorldToScreenPoint(lines[i].point3);
                        DrawLine(sp, sp1, lines[i].lineParams.lineThickness, lines[i].lineParams.lineColor);
                        DrawLine(sp2, sp1, lines[i].lineParams.lineThickness, lines[i].lineParams.lineColor);
                    }

                    //Draw TwoPair Lines
                    if (enableDrawingTwoPair && lines[i].elementType == ElementType.Line && lines[i].lineParams.lineName.Contains("IntersectingLine"))
                    {
                        sp1 = camera.WorldToScreenPoint(lines[i].point1);
                        sp2 = camera.WorldToScreenPoint(lines[i].point2);
                        DrawLine(sp1, sp2, lines[i].lineParams.lineThickness, lines[i].lineParams.lineColor);

                    }

                    //Draw Dashed Lines
                    if (lines[i].elementType == ElementType.DashedLine)
                    {
                        sp1 = camera.WorldToScreenPoint(lines[i].point1);
                        sp2 = camera.WorldToScreenPoint(lines[i].point2);
                        DrawDashedLine(sp1, sp2, lines[i].lineParams.dashedScaling, lines[i].lineParams.lineColor, lines[i].lineParams.lineThickness);
                    }

                    //Point 1
                    if (lines[i].elementType == ElementType.Point
                       || (lines[i].elementType == ElementType.Line && !lines[i].lineParams.lineName.Contains("IntersectingLine"))
                       || lines[i].elementType == ElementType.DashedLine
                       || (enableDrawingAngle && lines[i].elementType == ElementType.Angle))
                    {
                        if (lines[i].lineParams.endPoint1Type == EndPointType.CrossHair)
                        {
                            //CrossHair end point
                            sp = camera.WorldToScreenPoint(lines[i].point1);
                            DrawCrossHairPoints(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
                        }
                        else if (lines[i].lineParams.endPoint1Type == EndPointType.Arrow)
                        {
                            //Arrow end point
                            sp = camera.WorldToScreenPoint(lines[i].point1);
                            sp1 = camera.WorldToScreenPoint(lines[i].point2);
                            DrawArrows(sp, sp1, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
                        }
                    }

                    //Point 2
                    if ((lines[i].elementType == ElementType.Line && !lines[i].lineParams.lineName.Contains("IntersectingLine"))
                       || lines[i].elementType == ElementType.DashedLine
                       || (enableDrawingAngle && lines[i].elementType == ElementType.Angle))
                    {
                        if (lines[i].lineParams.endPoint2Type == EndPointType.CrossHair)
                        {
                            //CrossHair end point
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            DrawCrossHairPoints(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
                        }
                        else if (lines[i].lineParams.endPoint2Type == EndPointType.Arrow)
                        {
                            //Arrow end point
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            sp1 = camera.WorldToScreenPoint(lines[i].point1);
                            DrawArrows(sp, sp1, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
                        }
                    }

                    //Point 3
                    if (enableDrawingAngle && lines[i].elementType == ElementType.Angle)
                    {
                        if (lines[i].lineParams.endPoint3Type == EndPointType.Arrow)
                        {
                            //Arrow end point
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            sp1 = camera.WorldToScreenPoint(lines[i].point3);
                            DrawArrows(sp1, sp, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
                        }
                    }


                }
                GL.End();
                GL.PopMatrix();
                GL.PushMatrix();
                GL.LoadPixelMatrix();
                GL.Begin(GL.LINES);
                for (int i = 0; i < lines.Count; i++)
                {
                    //Point 1
                    if (lines[i].elementType == ElementType.Point
                       || (lines[i].elementType == ElementType.Line && !lines[i].lineParams.lineName.Contains("IntersectingLine"))
                       || lines[i].elementType == ElementType.DashedLine
                       || (enableDrawingAngle && lines[i].elementType == ElementType.Angle) || (enableDrawingTwoPair && lines[i].elementType == ElementType.Line && lines[i].lineParams.lineName.Contains("IntersectingLine")))
                    {
                        if (lines[i].lineParams.endPoint1Type == EndPointType.Circle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point1);
                            DrawCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                        else if (lines[i].lineParams.endPoint1Type == EndPointType.HollowCircle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point1);
                            DrawHollowCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                        else if (lines[i].lineParams.endPoint1Type == EndPointType.DoubleCircle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point1);
                            DrawDoubleCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                        else if (lines[i].lineParams.endPoint1Type == EndPointType.DoubleHollowCircle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point1);
                            DrawDoubleHollowCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                    }

                    //Point 2
                    if ((lines[i].elementType == ElementType.Line && !lines[i].lineParams.lineName.Contains("IntersectingLine"))
                       || lines[i].elementType == ElementType.DashedLine
                       || (enableDrawingAngle && lines[i].elementType == ElementType.Angle) || (enableDrawingTwoPair && lines[i].elementType == ElementType.Line && lines[i].lineParams.lineName.Contains("IntersectingLine")))
                    {
                        if (lines[i].lineParams.endPoint2Type == EndPointType.DoubleHollowCircle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            DrawDoubleHollowCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                        else if (lines[i].lineParams.endPoint2Type == EndPointType.DoubleCircle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            DrawDoubleCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                        else if (lines[i].lineParams.endPoint2Type == EndPointType.HollowCircle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            DrawHollowCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                        else if (lines[i].lineParams.endPoint2Type == EndPointType.Circle)
                        {
                            sp = camera.WorldToScreenPoint(lines[i].point2);
                            DrawCircularPoint(sp, lines[i].lineParams.endPointColor, lines[i].lineParams.pointThickness);
                        }
                    }

                    // draw arc for angle
                    if (enableDrawingAngle && lines[i].elementType == ElementType.Angle)
                    {
                        DrawArc(lines[i].point1, lines[i].point2, lines[i].point3, lines[i].lineParams.lineColor);
                    }
                }
                GL.End();
                GL.PopMatrix();

            }
        }
        catch(Exception ex)
        {
            Log.Item(ex);
        }
    }

While it works fine, it is giant method. What are some ways I can refactor this method into smaller, manageable chunks?

\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

DO NOT CATCH GENERAL EXCEPTION TYPES

Seriously, this is very important. You're logging it, which is nice, but do not just catch them.

Imagine this scenario. Somebody calls RenderLines but some of the state is invalid. Nothing appears on screen, but their code keeps on running. They're calling RenderLinesin their loop method, and the game is getting noticeably slower. They check the logs and find millions of generic exception messages all saying something like NullReferenceException.

This is horrible and a pain to debug, especially if you end up calling RenderLines in multiple places.

When you catch your exceptions for logging purposes, rethrow that exception. An exception is like a fish. If you catch a fish but have no intention of eating it, let it go again. Don't throw it in a basket and then dump it out the car on the way back. Only keep the fish you intend to deal with.

catch(Exception ex)
    {
        Log.Item(ex);
        throw;
    }

There are exceptions to this rule, but they are few, and certainly do not apply here.

Foreach

You're referencing lines[i] a lot in your loops, so much so that I think it would be cleaner for you to use a foreach loop instead.

foreach (var line in lines)
{
    // solid line
    if (line.elementType == ElementType.Line && !line.lineParams.lineName.Contains("IntersectingLine"))
        {
        etc...

Break up that method!

You're right, the method is huge, but there's no reason it has to be. You've conveniently delimited the major functionality segments in your method with comments, e.g. // solid line and // dashed line. Instead of having those comments there, why not extract those chunks into private methods and just call them?

\$\endgroup\$
2
  • \$\begingroup\$ Looking back at this, when do I know to throw an exception? Should I throw an exception for all exceptions? \$\endgroup\$
    – Kala J
    Commented Jul 6, 2015 at 15:28
  • 1
    \$\begingroup\$ The best guidelines is "throw an exception when some unusual condition from which you cannot reasonably continue occurs." Catch all exceptions you can reasonably deal with, and no more. \$\endgroup\$
    – Nick Udell
    Commented Jul 6, 2015 at 15:31
3
\$\begingroup\$

for this if statement that is nested

                //Point 3
                if (enableDrawingAngle && lines[i].elementType == ElementType.Angle)
                {
                    if (lines[i].lineParams.endPoint3Type == EndPointType.Arrow)
                    {
                        //Arrow end point
                        sp = camera.WorldToScreenPoint(lines[i].point2);
                        sp1 = camera.WorldToScreenPoint(lines[i].point3);
                        DrawArrows(sp1, sp, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
                    }
                }

it would actually look better if you put all the conditions into the first if statement(it looks like you are not worried about too many conditions)

so you should write it like this

//Point 3
if (enableDrawingAngle && lines[i].elementType == ElementType.Angle
    && lines[i].lineParams.endPoint3Type == EndPointType.Arrow)
{
    //Arrow end point
    sp = camera.WorldToScreenPoint(lines[i].point2);
    sp1 = camera.WorldToScreenPoint(lines[i].point3);
    DrawArrows(sp1, sp, lines[i].lineParams.endPointColor, lines[i].lineParams.lineThickness);
}

less nesting always looks better especially when you have so many if statements to begin with.


you also have some places among your if statement blocks that you should be adding new lines to keep them separate, it helps when you are reading through the code.

                    sp1 = camera.WorldToScreenPoint(lines[i].point1);
                    sp2 = camera.WorldToScreenPoint(lines[i].point2);
                    if (lines[i].lineParams.endPoint1Type == EndPointType.HollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness;
                        Vector3 diff = (sp2 - sp1).normalized * endpointRadius;
                        sp1 += diff;
                    }
                    else if (lines[i].lineParams.endPoint1Type == EndPointType.DoubleCircle || lines[i].lineParams.endPoint1Type == EndPointType.DoubleHollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness * doubleCircleRadiusRatio;
                        Vector3 diff = (sp2 - sp1).normalized * endpointRadius;
                        sp1 += diff;
                    }
                    if (lines[i].lineParams.endPoint2Type == EndPointType.HollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness;
                        Vector3 diff = (sp1 - sp2).normalized * endpointRadius;
                        sp2 += diff;
                    }
                    else if (lines[i].lineParams.endPoint2Type == EndPointType.DoubleCircle || lines[i].lineParams.endPoint2Type == EndPointType.DoubleHollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness * doubleCircleRadiusRatio; ;
                        Vector3 diff = (sp1 - sp2).normalized * endpointRadius;
                        sp2 += diff;
                    }
                    DrawLine(sp1, sp2, lines[i].lineParams.lineThickness, lines[i].lineParams.lineColor);

it should look like this

                    sp1 = camera.WorldToScreenPoint(lines[i].point1);
                    sp2 = camera.WorldToScreenPoint(lines[i].point2);

                    if (lines[i].lineParams.endPoint1Type == EndPointType.HollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness;
                        Vector3 diff = (sp2 - sp1).normalized * endpointRadius;
                        sp1 += diff;
                    }
                    else if (lines[i].lineParams.endPoint1Type == EndPointType.DoubleCircle || lines[i].lineParams.endPoint1Type == EndPointType.DoubleHollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness * doubleCircleRadiusRatio;
                        Vector3 diff = (sp2 - sp1).normalized * endpointRadius;
                        sp1 += diff;
                    }

                    if (lines[i].lineParams.endPoint2Type == EndPointType.HollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness;
                        Vector3 diff = (sp1 - sp2).normalized * endpointRadius;
                        sp2 += diff;
                    }
                    else if (lines[i].lineParams.endPoint2Type == EndPointType.DoubleCircle || lines[i].lineParams.endPoint2Type == EndPointType.DoubleHollowCircle)
                    {
                        float endpointRadius = lines[i].lineParams.pointThickness * doubleCircleRadiusRatio; ;
                        Vector3 diff = (sp1 - sp2).normalized * endpointRadius;
                        sp2 += diff;
                    }
                    DrawLine(sp1, sp2, lines[i].lineParams.lineThickness, lines[i].lineParams.lineColor);

so you know that there are two separate if blocks here.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ ++ another option, if the condition is too long for one's taste, could be to extract some bool IsAngleArrowEndPoint(Line) function. \$\endgroup\$ Commented May 27, 2015 at 14:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.