5
\$\begingroup\$

Here's some part of my code is use to draw objects on the screen :

Class members:

private double cursorX = 0;
private double cursorY = 0;
private LineObj cursorOldY = new LineObj();
private LineObj cursorOldX = new LineObj();
private int index1 = 0;

private double cursor2X = 0;
private double cursor2Y = 0;
private LineObj cursor2OldY = new LineObj();
private LineObj cursor2OldX = new LineObj();
private int index2 = -1;

ZedGraph.CurveItem curve = null;

Drawing function :

private void drawCursors(bool first, bool second, bool fromKeyboard = false, Point mousePoint = new Point()) 
{
    GraphPane p = lineGraphControl1.GraphPane;
    if (p.CurveList.Count == 0 || curve == null)
        return;

    ZedGraph.Axis axis = curve.IsY2Axis ? (Axis)p.Y2AxisList[curve.YAxisIndex] : (Axis)p.YAxisList[curve.YAxisIndex];
    ZedGraph.PointPair point = new PointPair(0.0, 0.0);
    if (first)
    {
        p.GraphObjList.Remove(cursorOldY);
        p.GraphObjList.Remove(cursorOldX);
        if (fromKeyboard)
           point = curve.Points[index1];
        else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
        {
            ZedGraph.CurveItem outputCurve;
            p.FindNearestPoint(mousePoint, curve, out outputCurve, out index1);
            if (index1 >= 0)
               point = curve.Points[index1];
        }
        else
        {
           if (!mousePoint.IsEmpty)
               p.ReverseTransform(mousePoint, out cursorX, out cursorY);
           point = findPoint(curve, cursorX);
        }
        cursorX = point.X;
        cursorY = transform(point.Y, axis);
        cursorOldY = new LineObj(cursorX, p.YAxis.Scale.Min, cursorX, p.YAxis.Scale.Max);
        cursorOldX = new LineObj(p.XAxis.Scale.Max, cursorY, p.XAxis.Scale.Min, cursorY);

       if (cursorX > p.XAxis.Scale.Min && cursorX < p.XAxis.Scale.Max)
           p.GraphObjList.Add(cursorOldY);

       if (cursorY > p.YAxis.Scale.Min && cursorY < p.YAxis.Scale.Max)
           p.GraphObjList.Add(cursorOldX);
   }
   if (second)
   {
       p.GraphObjList.Remove(cursor2OldY);
       p.GraphObjList.Remove(cursor2OldX);
       if (fromKeyboard)
           point = curve.Points[index2];
       else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
       {
           ZedGraph.CurveItem outputCurve;
           p.FindNearestPoint(mousePoint, curve, out outputCurve, out index2);
           if (index2 >= 0)
               point = curve.Points[index2];
       }
       else
       {
           if (!mousePoint.IsEmpty)
               p.ReverseTransform(mousePoint, out cursor2X, out cursor2Y);
           point = findPoint(curve, cursor2X);
           if (index2 == -1)
           {
               index2 = (int) Math.Round(curve.NPts * 0.75, 0);
               point = curve.Points[index2];
           }
       }

       cursor2X = point.X;
       cursor2Y = transform(point.Y, axis);
       cursor2OldY = new LineObj(cursor2X, p.YAxis.Scale.Min, cursor2X, p.YAxis.Scale.Max);
       cursor2OldX = new LineObj(p.XAxis.Scale.Max, cursor2Y, p.XAxis.Scale.Min, cursor2Y);

       if (cursor2X > p.XAxis.Scale.Min && cursor2X < p.XAxis.Scale.Max)
           p.GraphObjList.Add(cursor2OldY);
       if (cursor2Y > p.YAxis.Scale.Min && cursor2Y < p.YAxis.Scale.Max)
           p.GraphObjList.Add(cursor2OldX);
    }
}

Is (structurally speaking) there some optimizations I missed ?

\$\endgroup\$
  • \$\begingroup\$ How is drawCursors called? And by what? \$\endgroup\$ – Nick Udell Oct 22 '14 at 14:55
  • \$\begingroup\$ @NickUdell it is called in different places with different parameters depending on the user input. The only thing that will always be true is that (first || second) = true \$\endgroup\$ – Thomas Ayoub Oct 22 '14 at 14:57
  • \$\begingroup\$ It must, surely, only be called inside that class though, as it's private. Give us some examples from within that class? \$\endgroup\$ – Nick Udell Oct 22 '14 at 14:58
  • \$\begingroup\$ @NickUdell if the user moves the mouse, the function is called with drawCursors(true, false, false, Mouse.Position) \$\endgroup\$ – Thomas Ayoub Oct 22 '14 at 15:03
  • 1
    \$\begingroup\$ What you can and cannot do after receiving answers \$\endgroup\$ – Malachi Oct 22 '14 at 15:12
2
\$\begingroup\$

DRY Don't repeat yourself

Looking at the method, it is clearly seen that you do the same thing for first == true and for second == true, but you are using different variables.

As the set of variables

private double cursor2X = 0;
private double cursor2Y = 0;
private LineObj cursor2OldY = new LineObj();
private LineObj cursor2OldX = new LineObj();
private int index2 = -1;  

are defining, what you need to draw a crosshair, you should place them inside a class.

An instance of this class can then be passed instead of the bool first,second to your method. In this way you have less code to maintain, if e.g something should be changed in this drawing part, as you only need to change it once and not twice.

Naming

GraphPane p

Single letter variable names should be avoided. For loops as iteration variable they are ok, or for positioning like x,y. Your code would be easier to understand if you would name it pane instead.

Based on the naming guidlines you should use PascalCasing casing for method names.

\$\endgroup\$
5
\$\begingroup\$

Mis-Matching curly braces are not a good thing, you do it all over the place, it makes your code hard to read

   if (fromKeyboard)
       point = curve.Points[index1];
    else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
    {
        ZedGraph.CurveItem outputCurve;
        p.FindNearestPoint(mousePoint, curve, out outputCurve, out index1);
        if (index1 >= 0)
           point = curve.Points[index1];
    }
    else
    {

it should look like this

   if (fromKeyboard)
   {
       point = curve.Points[index1];
   }
   else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
   {
        ZedGraph.CurveItem outputCurve;
        p.FindNearestPoint(mousePoint, curve, out outputCurve, out index1);
        if (index1 >= 0)
           point = curve.Points[index1];
   }
   else
   {

Indentation only on an if statement all by itself is fine, I personally don't like it, but when an else statement is added to the if statement, it starts to muddy the waters around your if statement and make it look sloppy.


The single, one-line if statements should also have a buffer zone after them as well.

\$\endgroup\$
  • \$\begingroup\$ You're right, I'll change it \$\endgroup\$ – Thomas Ayoub Oct 22 '14 at 15:09
  • \$\begingroup\$ don't edit it into the question. when you get some good constructive answers, clean up your code and post a follow-up question. \$\endgroup\$ – Malachi Oct 22 '14 at 15:10
  • \$\begingroup\$ Do you mean that I should post another question after you answer ? I'm not sure to understand you well... \$\endgroup\$ – Thomas Ayoub Oct 22 '14 at 15:11
  • \$\begingroup\$ I posted a link in the comments on your question. when you get more answers, change the code on your local machine, make sure it works and post another question with it. \$\endgroup\$ – Malachi Oct 22 '14 at 15:13
  • 4
    \$\begingroup\$ Ok, I'll read your link. I like this site :) \$\endgroup\$ – Thomas Ayoub Oct 22 '14 at 15:13

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.