33
\$\begingroup\$

I have a method that has a lot of loops:

private void update(double depth)
        {

            Console.WriteLine("update with level " + depth);

            this.Dispatcher.Invoke(System.Windows.Threading.DispatcherPriority.Normal, new Action(delegate()
                {


            List<Grid> grids = new List<Grid>();

            Dependencies.Children.Clear();

            Grid g = new Grid();
            //Canvas.SetZIndex(g, 100);
            g.Width = 50;
            g.Height = 50;
            g.Tag = focus;

            Ellipse e = new Ellipse();
            e.Width = 50;
            e.Height = 50;
            e.Fill = Brushes.Red;
            if (depth == 1)
            {
                Canvas.SetTop(g, 163);
            }
            else if (depth == 2)
            {
                Canvas.SetTop(g, 108);
            }
            else if (depth == 3)
            {
                Canvas.SetTop(g, 81);
            }
            else if (depth == 4)
            {
                Canvas.SetTop(g, 65);
            }
            else if (depth == 5)
            {
                Canvas.SetTop(g, 54);
            }
            else if (depth == 6)
            {
                Canvas.SetTop(g, 46);
            }
            Canvas.SetLeft(g, 500);

            g.Children.Add(e);

            Viewbox box = new Viewbox();
            box.Width = e.Width;
            box.Height = e.Height;


            TextBox txt = new TextBox();
            txt.Text = focus.getName();
            box.Child = txt;
            txt.Background = Brushes.Transparent;
            txt.BorderBrush = Brushes.Transparent;

            g.Children.Add(box);



            grids.Add(g);

            List<SourceFile> list = new List<SourceFile>();

            list = focus.getInvocations();

            int counter = 1;
            foreach (SourceFile sf in list)
            {
                Grid g1 = new Grid();
                //Canvas.SetZIndex(g, 101);
                g1.Width = 50;
                g1.Height = 50;
                g1.Tag = sf;

                Ellipse e1 = new Ellipse();
                //Dependencies.Children.Add(e1);
                sf.setGrid(g1);
                e1.Width = 50;
                e1.Height = 50;
                e1.Fill = Brushes.Red;

                g1.Children.Add(e1);

                if (depth == 1)
                {
                    Canvas.SetTop(g1, 488);
                }
                else if (depth == 2)
                {
                    Canvas.SetTop(g1, 324);
                }
                else if (depth == 3)
                {
                    Canvas.SetTop(g1, 244);
                }
                else if (depth == 4)
                {
                    Canvas.SetTop(g1, 195);
                }
                else if (depth == 5)
                {
                    Canvas.SetTop(g1, 163);
                }
                else if (depth == 6)
                {
                    Canvas.SetTop(g1, 139);
                }
                Canvas.SetLeft(g1, counter * (1000 / (list.Count + 1) ));

                Viewbox box1 = new Viewbox();
                box1.Width = g1.Width;
                box1.Height = g1.Height;

                TextBox txt1 = new TextBox();
                txt1.Text = sf.getName();
                txt1.Background = Brushes.Transparent;
                txt1.BorderBrush = Brushes.Transparent;

                box1.Child = txt1;
                g1.Children.Add(box1);

                Line l = new Line();
                //Canvas.SetZIndex(l, 1);
                l.Stroke = Brushes.Green;
                l.StrokeThickness = 10;
                Binding x1 = new Binding(); x1.Path = new PropertyPath(Canvas.LeftProperty);
                x1.Converter = new MyConverter();
                x1.ConverterParameter = g;
                Binding y1 = new Binding(); y1.Path = new PropertyPath(Canvas.TopProperty);
                y1.Converter = new MyConverter();
                y1.ConverterParameter = g;
                Binding x2 = new Binding(); x2.Path = new PropertyPath(Canvas.LeftProperty);
                x2.Converter = new MyConverter();
                x2.ConverterParameter = g1;
                Binding y2 = new Binding(); y2.Path = new PropertyPath(Canvas.TopProperty);
                y2.Converter = new MyConverter();
                y2.ConverterParameter = g1;
                x1.Source = y1.Source = g;
                x2.Source = y2.Source = g1;
                l.SetBinding(Line.X1Property, x1);
                l.SetBinding(Line.Y1Property, y1);
                l.SetBinding(Line.X2Property, x2);
                l.SetBinding(Line.Y2Property, y2);
                Dependencies.Children.Add(l);
                l.Tag = new Call(focus, sf);
                Contacts.AddPreviewContactDownHandler(l, OnLineDown);
                counter++;

                grids.Add(g1);

                SizeChangedEventHandler act = (Object s, SizeChangedEventArgs args) =>
                {
                    BindingOperations.GetBindingExpressionBase(l, Line.X1Property).UpdateTarget();
                    BindingOperations.GetBindingExpressionBase(l, Line.Y1Property).UpdateTarget();
                    BindingOperations.GetBindingExpressionBase(l, Line.X2Property).UpdateTarget();
                    BindingOperations.GetBindingExpressionBase(l, Line.Y2Property).UpdateTarget();
                };

                g.SizeChanged += act;
                g1.SizeChanged += act;
            }


            int counter2 = 1;
            if (depth >= 2)
            {

                int invocCount = 0;

                foreach (SourceFile s in list)
                {
                    foreach (SourceFile source in s.getInvocations())
                    {
                        invocCount = invocCount + s.getInvocations().Count;
                    }
                }

                Console.WriteLine(invocCount);

                foreach (SourceFile s in list)
                {
                    foreach (SourceFile source in s.getInvocations())
                    {

                        Console.WriteLine("`Found invocation of " + s.getName() + ": " + source.getName());

                        Grid g1 = new Grid();
                        g1.Width = 50;
                        g1.Height = 50;

                        Ellipse e1 = new Ellipse();
                       // Canvas.SetZIndex(g1, 102);
                        grids.Add(g1);
                        e1.Width = 50;
                        e1.Height = 50;
                        e1.Fill = Brushes.Red;
                        source.setGrid(g1);
                        g1.Tag = source;

                        g1.Children.Add(e1);

                        if (depth == 2)
                        {
                            Canvas.SetTop(g1, 540);
                        }
                        else if (depth == 3)
                        {
                            Canvas.SetTop(g1, 406);
                        }
                        else if (depth == 4)
                        {
                            Canvas.SetTop(g1, 325);
                        }
                        else if (depth == 5)
                        {
                            Canvas.SetTop(g1, 271);
                        }
                        else if (depth == 6)
                        {
                            Canvas.SetTop(g1, 232);
                        }

                        Canvas.SetLeft(g1, counter2 * (1000 / (invocCount + 1)));

                        Viewbox box1 = new Viewbox();
                        box1.Width = g1.Width;
                        box1.Height = g1.Height;

                        TextBox txt1 = new TextBox();
                        txt1.Text = source.getName();
                        txt1.Background = Brushes.Transparent;
                        txt1.BorderBrush = Brushes.Transparent;

                        box1.Child = txt1;
                        g1.Children.Add(box1);

                        Line l = new Line();
                        //Canvas.SetZIndex(l, 2);
                        l.Stroke = Brushes.Green;
                        l.StrokeThickness = 10;
                        Binding x1 = new Binding(); x1.Path = new PropertyPath(Canvas.LeftProperty);
                        x1.Converter = new MyConverter();
                        x1.ConverterParameter = s.getGrid();
                        Binding y1 = new Binding(); y1.Path = new PropertyPath(Canvas.TopProperty);
                        y1.Converter = new MyConverter();
                        y1.ConverterParameter = s.getGrid();
                        Binding x2 = new Binding(); x2.Path = new PropertyPath(Canvas.LeftProperty);
                        x2.Converter = new MyConverter();
                        x2.ConverterParameter = g1;
                        Binding y2 = new Binding(); y2.Path = new PropertyPath(Canvas.TopProperty);
                        y2.Converter = new MyConverter();
                        y2.ConverterParameter = g1;
                        x1.Source = y1.Source = findGrid(grids, s, source);
                        x2.Source = y2.Source = g1;
                        l.SetBinding(Line.X1Property, x1);
                        l.SetBinding(Line.Y1Property, y1);
                        l.SetBinding(Line.X2Property, x2);
                        l.SetBinding(Line.Y2Property, y2);
                        Dependencies.Children.Add(l);
                        l.Tag = new Call(s, source);
                        Contacts.AddPreviewContactDownHandler(l, OnLineDown);

                        counter2++;

                        SizeChangedEventHandler act = (Object o, SizeChangedEventArgs args) =>
                        {
                            BindingOperations.GetBindingExpressionBase(l, Line.X1Property).UpdateTarget();
                            BindingOperations.GetBindingExpressionBase(l, Line.Y1Property).UpdateTarget();
                            BindingOperations.GetBindingExpressionBase(l, Line.X2Property).UpdateTarget();
                            BindingOperations.GetBindingExpressionBase(l, Line.Y2Property).UpdateTarget();
                        };

                        source.getGrid().SizeChanged += act;
                        g1.SizeChanged += act;
                    }
                }
            }


            int counter3 = 1;
            if (depth >= 3)
            {

                int invocCount = 0;

                foreach (SourceFile s in list)
                {
                    foreach (SourceFile source in s.getInvocations())
                    {
                        foreach (SourceFile s1 in source.getInvocations())
                        {
                            invocCount = invocCount + source.getInvocations().Count;
                        }
                    }
                }

                foreach (SourceFile s in list)
                {
                    foreach (SourceFile source in s.getInvocations())
                    {
                        foreach (SourceFile s1 in source.getInvocations())
                        {
                            Grid g1 = new Grid();
                            grids.Add(g1);
                            g1.Width = 50;
                            g1.Height = 50;
                            g1.Tag = s1;
                            Ellipse e1 = new Ellipse();

                            e1.Width = 50;
                            e1.Height = 50;
                            e1.Fill = Brushes.Red;
                            s1.setGrid(g1);
                            g1.Children.Add(e1);

                            if (depth == 3)
                            {
                                Canvas.SetTop(g1, 569);
                            }
                            else if (depth == 4)
                            {
                                Canvas.SetTop(g1, 455);
                            }
                            else if (depth == 5)
                            {
                                Canvas.SetTop(g1, 379);
                            }
                            else if (depth == 6)
                            {
                                Canvas.SetTop(g1, 325);
                            }
                            Canvas.SetLeft(g1, counter3 * (1000 / (invocCount + 1)));

                            Viewbox box1 = new Viewbox();
                            box1.Width = g1.Width;
                            box1.Height = g1.Height;

                            TextBox txt1 = new TextBox();
                            txt1.Background = Brushes.Transparent;
                            txt1.BorderBrush = Brushes.Transparent;
                            txt1.Text = s1.getName();
                            box1.Child = txt1;
                            g1.Children.Add(box1);

                            Line l = new Line();
                            //Canvas.SetZIndex(l, 2);
                            l.Stroke = Brushes.Green;
                            l.StrokeThickness = 10;
                            Binding x1 = new Binding(); x1.Path = new PropertyPath(Canvas.LeftProperty);
                            x1.Converter = new MyConverter();
                            x1.ConverterParameter = source.getGrid();
                            Binding y1 = new Binding(); y1.Path = new PropertyPath(Canvas.TopProperty);
                            y1.Converter = new MyConverter();
                            y1.ConverterParameter = source.getGrid();
                            Binding x2 = new Binding(); x2.Path = new PropertyPath(Canvas.LeftProperty);
                            x2.Converter = new MyConverter();
                            x2.ConverterParameter = g1;
                            Binding y2 = new Binding(); y2.Path = new PropertyPath(Canvas.TopProperty);
                            y2.Converter = new MyConverter();
                            y2.ConverterParameter = g1;
                            x1.Source = y1.Source = findGrid(grids, source, s1);
                            x2.Source = y2.Source = g1;
                            l.SetBinding(Line.X1Property, x1);
                            l.SetBinding(Line.Y1Property, y1);
                            l.SetBinding(Line.X2Property, x2);
                            l.SetBinding(Line.Y2Property, y2);
                            Dependencies.Children.Add(l);
                            l.Tag = new Call(source, s1);
                            Contacts.AddPreviewContactDownHandler(l, OnLineDown);

                            counter3++;

                            SizeChangedEventHandler act = (Object o, SizeChangedEventArgs args) =>
                            {
                                BindingOperations.GetBindingExpressionBase(l, Line.X1Property).UpdateTarget();
                                BindingOperations.GetBindingExpressionBase(l, Line.Y1Property).UpdateTarget();
                                BindingOperations.GetBindingExpressionBase(l, Line.X2Property).UpdateTarget();
                                BindingOperations.GetBindingExpressionBase(l, Line.Y2Property).UpdateTarget();
                            };

                            s1.getGrid().SizeChanged += act;
                            g1.SizeChanged += act;
                        }
                    }
                }
            }


         int counter4 = 1;
         if (depth >= 4)
         {

             int invoCount = 0;
             foreach (SourceFile s in list)
             {
                 foreach (SourceFile source in s.getInvocations())
                 {
                     foreach (SourceFile s1 in source.getInvocations())
                     {
                         foreach (SourceFile s2 in s1.getInvocations())
                         {
                             invoCount = invoCount + s1.getInvocations().Count;
                         }
                     }
                 }
             }

             foreach (SourceFile s in list)
             {
                 foreach (SourceFile source in s.getInvocations())
                 {
                     foreach (SourceFile s1 in source.getInvocations())
                     {
                         foreach (SourceFile s2 in s1.getInvocations())
                         {

                             Grid g1 = new Grid();
                             grids.Add(g1);
                             g1.Width = 50;
                             g1.Height = 50;
                             g1.Tag = s2;
                             Ellipse e1 = new Ellipse();

                             e1.Width = 50;
                             e1.Height = 50;
                             e1.Fill = Brushes.Red;
                             s2.setGrid(g1);

                             g1.Children.Add(e1);

                             if (depth == 4)
                             {
                                 Canvas.SetTop(g1, 585);
                             }
                             else if (depth == 5)
                             {
                                 Canvas.SetTop(g1, 488);
                             }
                             else if (depth == 6)
                             {
                                 Canvas.SetTop(g1, 418);
                             }
                             Canvas.SetLeft(g1, counter4 * (1000 / (invoCount + 1)));

                             Viewbox box1 = new Viewbox();
                             box1.Width = g1.Width;
                             box1.Height = g1.Height;

                             TextBox txt1 = new TextBox();
                             txt1.Background = Brushes.Transparent;
                             txt1.BorderBrush = Brushes.Transparent;
                             txt1.Text = s2.getName();
                             box1.Child = txt1;
                             g1.Children.Add(box1);

                             Line l = new Line();
                             //Canvas.SetZIndex(l, 2);
                             l.Stroke = Brushes.Green;
                             l.StrokeThickness = 10;
                             Binding x1 = new Binding(); x1.Path = new PropertyPath(Canvas.LeftProperty);
                             x1.Converter = new MyConverter();
                             x1.ConverterParameter = s1.getGrid();
                             Binding y1 = new Binding(); y1.Path = new PropertyPath(Canvas.TopProperty);
                             y1.Converter = new MyConverter();
                             y1.ConverterParameter = s1.getGrid();
                             Binding x2 = new Binding(); x2.Path = new PropertyPath(Canvas.LeftProperty);
                             x2.Converter = new MyConverter();
                             x2.ConverterParameter = g1;
                             Binding y2 = new Binding(); y2.Path = new PropertyPath(Canvas.TopProperty);
                             y2.Converter = new MyConverter();
                             y2.ConverterParameter = g1;
                             x1.Source = y1.Source = findGrid(grids, s1, s2);
                             x2.Source = y2.Source = g1;
                             l.SetBinding(Line.X1Property, x1);
                             l.SetBinding(Line.Y1Property, y1);
                             l.SetBinding(Line.X2Property, x2);
                             l.SetBinding(Line.Y2Property, y2);
                             Dependencies.Children.Add(l);
                             l.Tag = new Call(s1, s2);
                             Contacts.AddPreviewContactDownHandler(l, OnLineDown);
                             counter4++;

                             SizeChangedEventHandler act = (Object o, SizeChangedEventArgs args) =>
                             {
                                 BindingOperations.GetBindingExpressionBase(l, Line.X1Property).UpdateTarget();
                                 BindingOperations.GetBindingExpressionBase(l, Line.Y1Property).UpdateTarget();
                                 BindingOperations.GetBindingExpressionBase(l, Line.X2Property).UpdateTarget();
                                 BindingOperations.GetBindingExpressionBase(l, Line.Y2Property).UpdateTarget();
                             };

                             s2.getGrid().SizeChanged += act;
                             g1.SizeChanged += act;
                         }
                     }
                 }
             }
         }



      int counter5 = 1;
      if (depth >= 5)
      {

          int invoCount = 0;

          foreach (SourceFile s in list)
          {
              foreach (SourceFile source in s.getInvocations())
              {
                  foreach (SourceFile s1 in source.getInvocations())
                  {
                      foreach (SourceFile s2 in s1.getInvocations())
                      {
                          foreach (SourceFile s3 in s2.getInvocations())
                          {
                              invoCount = invoCount + s2.getInvocations().Count;
                          }
                      }
                  }
              }
          }

          foreach (SourceFile s in list)
          {
              foreach (SourceFile source in s.getInvocations())
              {
                  foreach (SourceFile s1 in source.getInvocations())
                  {
                      foreach (SourceFile s2 in s1.getInvocations())
                      {
                          foreach (SourceFile s3 in s2.getInvocations())
                          {
                              Grid g1 = new Grid();
                              g1.Width = 50;
                              g1.Height = 50;
                              grids.Add(g1);
                              g1.Tag = s3;
                              Ellipse e1 = new Ellipse();
                              //Dependencies.Children.Add(e1);
                              e1.Width = 50;
                              e1.Height = 50;
                              e1.Fill = Brushes.Red;
                              s3.setGrid(g1);

                              g1.Children.Add(e1);

                              if (depth == 5)
                              {
                                  Canvas.SetTop(g1, 596);
                              }
                              else if (depth == 6)
                              {
                                  Canvas.SetTop(g1, 511);
                              }
                              Canvas.SetLeft(g1, counter5 * (1000 / (invoCount + 1)));

                              Viewbox box1 = new Viewbox();
                              box1.Width = g1.Width;
                              box1.Height = g1.Height;

                              TextBox txt1 = new TextBox();
                              txt1.Background = Brushes.Transparent;
                              txt1.BorderBrush = Brushes.Transparent;
                              txt1.Text = s3.getName();
                              box1.Child = txt1;
                              g1.Children.Add(box1);

                              Line l = new Line();
                              //Canvas.SetZIndex(l, 2);
                              l.Stroke = Brushes.Green;
                              l.StrokeThickness = 10;
                              Binding x1 = new Binding(); x1.Path = new PropertyPath(Canvas.LeftProperty);
                              x1.Converter = new MyConverter();
                              x1.ConverterParameter = s2.getGrid();
                              Binding y1 = new Binding(); y1.Path = new PropertyPath(Canvas.TopProperty);
                              y1.Converter = new MyConverter();
                              y1.ConverterParameter = s2.getGrid();
                              Binding x2 = new Binding(); x2.Path = new PropertyPath(Canvas.LeftProperty);
                              x2.Converter = new MyConverter();
                              x2.ConverterParameter = g1;
                              Binding y2 = new Binding(); y2.Path = new PropertyPath(Canvas.TopProperty);
                              y2.Converter = new MyConverter();
                              y2.ConverterParameter = g1;
                              x1.Source = y1.Source = findGrid(grids, s2, s3);
                              x2.Source = y2.Source = g1;
                              l.SetBinding(Line.X1Property, x1);
                              l.SetBinding(Line.Y1Property, y1);
                              l.SetBinding(Line.X2Property, x2);
                              l.SetBinding(Line.Y2Property, y2);
                              l.Tag = new Call(s2, s3);
                              Contacts.AddPreviewContactDownHandler(l, OnLineDown);
                              Dependencies.Children.Add(l);

                              counter5++;

                              SizeChangedEventHandler act = (Object o, SizeChangedEventArgs args) =>
                              {
                                  BindingOperations.GetBindingExpressionBase(l, Line.X1Property).UpdateTarget();
                                  BindingOperations.GetBindingExpressionBase(l, Line.Y1Property).UpdateTarget();
                                  BindingOperations.GetBindingExpressionBase(l, Line.X2Property).UpdateTarget();
                                  BindingOperations.GetBindingExpressionBase(l, Line.Y2Property).UpdateTarget();
                              };

                              s3.getGrid().SizeChanged += act;
                              g1.SizeChanged += act;
                          }
                      }
                  }
              }
          }
      }



            foreach (Grid grid in grids)
            {
                Dependencies.Children.Add(grid);
                Contacts.AddPreviewContactDownHandler(grid, DownOnSourceFile);
            }
                }
  ));
        }

Is there any easy way to improve that? And to make it working not only for 6 steps but also for n steps?

\$\endgroup\$
  • 17
    \$\begingroup\$ tl;dr. You'll likely have a better response if you remove a lot of the boilerplate or focus on the part you're really interested in. I think pseudo-code would help a lot here. \$\endgroup\$ – Wade Tandy Jan 19 '11 at 21:26
  • 12
    \$\begingroup\$ Whenever you think of using copy and paste, don't. Make a method and call it twice. \$\endgroup\$ – adamk Jan 20 '11 at 0:20
  • 3
    \$\begingroup\$ Break your method into a lot of smaller ones. \$\endgroup\$ – Sergio Tapia Jan 21 '11 at 1:04
  • \$\begingroup\$ Even with your code abstracted, changing this from a static depth to n-depth is very non-trivial and off topic for this site. You will require formulas instead of constants and recursion or otherwise completely different looping structure to walk the tree (depending on how deep you want to go recursion becomes problematic due to limited stack space, though optimizations on x64 machines may be able to help with that). Regardless, the review is unanimous: refactor/abstract and improve naming; the solution to your unsolved problem, however, is for a different site. \$\endgroup\$ – TheXenocide Feb 3 '11 at 0:15

11 Answers 11

62
\$\begingroup\$

Break this down into several methods - it's very long, meaning it's not easy to read.

\$\endgroup\$
  • 1
    \$\begingroup\$ Actually it is even longer ;) I wasn't able to post the whole method because of the character limit;) \$\endgroup\$ – RoflcoptrException Jan 20 '11 at 0:28
  • 2
    \$\begingroup\$ @Roflcoptr You must be trying to beat my former supervisor 1200 lines-long methods. Definitely break it down, it is a nightmare in the making. \$\endgroup\$ – MetalMikester Jan 21 '11 at 1:10
  • 1
    \$\begingroup\$ +1 definitely break it down to smaller methods - and use some Linq as well!! \$\endgroup\$ – Aim Kai Jan 21 '11 at 10:24
  • \$\begingroup\$ @Roflcoptr In the future if you need to post a lot of code maybe try GitHub Gists. (Although, it's likely a good indicator that your example it too long if it won't fit.) \$\endgroup\$ – Captain Man May 5 '15 at 1:31
59
\$\begingroup\$

This code...

if (depth == 1)
{
    Canvas.SetTop(g1, 163);
}
else if (depth == 2)
{
    Canvas.SetTop(g1, 108);
}
else if (depth == 3)
{
    Canvas.SetTop(g1, 81);
}
else if (depth == 4)
{
    Canvas.SetTop(g1, 65);
}
else if (depth == 5)
{
    Canvas.SetTop(g1, 54);
}
else if (depth == 6)
{
    Canvas.SetTop(g1, 46);
}

Could be better implemented using an array...

int[] values = new [] { 0, 163, 108, 81, 65, 54, 46  }

Or Dictionary...

var values = new Dictionary<int,int>() { { 1, 163 }, { 2, 108 }, { 3, 81 }, { 4, 65 }, { 5, 54 }, { 6, 46} };

This way you could simple say

Canvas.SetTop(g1, values[depth])
\$\endgroup\$
  • 3
    \$\begingroup\$ alternatively, a switch statement would help and introduce absolutely no overhead (in fact, it's faster than repeated if/else operations) \$\endgroup\$ – Felix Dombek Jan 20 '11 at 0:42
  • 16
    \$\begingroup\$ @Felix: But a switch statement wouldn't really make the code shorter. \$\endgroup\$ – Adam Lear Jan 20 '11 at 2:18
  • 4
    \$\begingroup\$ There seem to be some prejudices towards using a control statement exactly for what it was made. I embrace brevity, but if this is in its own method it will be readable again, and if performance makes a difference, I would strongly prefer the switch statement \$\endgroup\$ – Felix Dombek Jan 20 '11 at 22:03
  • 2
    \$\begingroup\$ Don't forget The question says it should be done not just for 6 but for n iterations. There's no such thing as a dynamic-n-cases-switch. But I do agree that portion of code should be in it own method \$\endgroup\$ – Carlos Muñoz Jan 20 '11 at 23:59
  • 1
    \$\begingroup\$ @Carlos right -- what we need is a formula to spit out a number for any n. I can't think of the right one just now, but it probably has do something with log2 \$\endgroup\$ – Felix Dombek Jan 21 '11 at 15:36
29
\$\begingroup\$

There are a couple of immediately obvious things that havent been mentioned yet:

You have a lot of magic numbers in your code. Try to define them as consts with meaningful names.

For example

g.Width = 50;

becomes

private const int DefaultGridWidth = 50;
...
g.Width = DefaultGridWidth;

It seems like a trivial change but it makes a big difference to someone who is reading your code. It gives an indication of why the value is 50, not just that it is 50.


You should use more meaningful names for your identifiers. Names like g and g1 do not tell me a lot about what the object is, but mainGrid and innerGrid contain more information for the reader.

\$\endgroup\$
  • \$\begingroup\$ That is indeed true, too many literals in code make it harder to maintain and adapt, especially so if there are cross-dependencies among the values! \$\endgroup\$ – Felix Dombek Jan 20 '11 at 22:05
  • 1
    \$\begingroup\$ He should use static readonly instead of const - then he won't have to recompile assemblies that depend on his code with every change! Also, he's not limited to only values but can use references as constants as well. \$\endgroup\$ – Zolomon Mar 23 '11 at 7:47
18
\$\begingroup\$

Give some thought to abstracting the decisions around setting the top of the canvas (cf. all those if statements) out into a set of classes - or perhaps a single class with different suitable parameters in the constructor. A lot of this code differs only in the numbers being applied.

A simple rule is "Abstract the concept that varies".

\$\endgroup\$
18
\$\begingroup\$

Since you're using C#, you can make your initializers a bit nicer:

Grid g = new Grid()
{
    Width = 50,
    Height = 50,
    Tag = focus,
}
//Canvas.SetZIndex(g, 100);

The last part of your code (or rather, the second half) does very similar things multiple times: code duplication is a sign your code can be made clearer. For instance (note that your code almost surely contains a bug! The statement inside all the loops invokes s2 and not s3):

foreach (SourceFile s in list)
{
    foreach (SourceFile source in s.getInvocations())
    {
        foreach (SourceFile s1 in source.getInvocations())
        {
            foreach (SourceFile s2 in s1.getInvocations())
            {
                foreach (SourceFile s3 in s2.getInvocations())
                {
                    invoCount = invoCount + s2.getInvocations().Count;
                }
            }
        }
    }
}

can be changed into

list.CountRecursive(t => t.getInvocations(), t => t.getInvocations().Count, 5);

(...)

public static int CountRecursive<T>(this IEnumerable<T> x, Func<T, IEnumerable<T>> f, Func<T, int> c, int depth)
{
    int counter = 0;
    foreach (T t in x)
    {
        if (depth > 1)
        {
            counter += f(t).CountRecursive(f, c, depth - 1);
        }
        else
        {
            counter += c(t);
        }
    }
    return counter;
}

which also makes it a lot easier to do it for different recursion levels.

\$\endgroup\$
9
\$\begingroup\$

I might recommend the use of switch statements and white space along with LRE's suggestion of breaking this into multiple methods. It also looks like you have quite a bit of repeated code maybe try to break that out for sure it its own methods.

\$\endgroup\$
  • \$\begingroup\$ In this case switch may not be the best option. It would make the code use case instead of if/else but the code will almost be equally lengthy \$\endgroup\$ – Carlos Muñoz Jan 19 '11 at 22:41
8
\$\begingroup\$

I refactored it for you. I did all of this without testing, so there's likely a bunch of bugs and off-by-one errors. I also made some assumptions about the code that you didn't include. Now that the code's a lot smaller, it should be easier for you to find those bugs.

A couple of important points:

  1. Recursion is a fundamental concept in programming. If you are a professional programmer, you absolutely must be comfortable with it, or you will never be able to deal with nested structures effectively.

  2. If you copy and paste, you're doing it wrong. Every time you hit Ctrl+C, a kitten dies. No no no.

  3. If you have variables named something1, something2, something3, etc. you're doing it wrong. At the very least, those should be an array.

  4. You had depth as a double but were comparing it to literal values other than zero. That's bad.

Here you go:

private void update(int depth)
{
    Console.WriteLine("update with level " + depth);

    Dispatcher.Invoke(System.Windows.Threading.DispatcherPriority.Normal, new Action(delegate()
    {
        List<Grid> grids = new List<Grid>();

        Dependencies.Children.Clear();

        Grid grid = MakeOuterGrid(grids, focus, e.Width, e.Height, depth);

        List<SourceFile> list = focus.getInvocations();

        for (int i = 1; i <= depth; i++)
        {
            int invocCount = CountInvocations(focus, i + 1);
            int counter = 0;
            MakeRecursiveGrids(grids, null, focus, i, invocCount, i, ref counter);
        }

        foreach (Grid grid in grids)
        {
            Dependencies.Children.Add(grid);
            Contacts.AddPreviewContactDownHandler(grid, DownOnSourceFile);
        }
    }));
}

void AdjustTop(int depth, int table) {
    int[][] depthTable = new int[][] {
        new int[] { 163, 108,  81,  65,  54,  46 },
        new int[] { 488, 324, 244, 195, 163, 139 },
        new int[] {  -1, 540, 406, 325, 271, 232 },
        new int[] {  -1,  -1, 569, 455, 379, 325 },
        new int[] {  -1,  -1,  -1, 585, 488, 418 },
        new int[] {  -1,  -1,  -1,  -1, 596, 511 },
    }

    int[] depths = depthTable[table];
    if ((depth < depths.Length) && (depths[depth - 1] != -1)) {
        Canvas.SetTop(depths[depth - 1]);
    }
}

SizeChangedEventHandler UpdateBindings(Line line) {
    SizeChangedEventHandler act = (Object o, SizeChangedEventArgs args) =>
    {
        BindingOperations.GetBindingExpressionBase(line, Line.X1Property).UpdateTarget();
        BindingOperations.GetBindingExpressionBase(line, Line.Y1Property).UpdateTarget();
        BindingOperations.GetBindingExpressionBase(line, Line.X2Property).UpdateTarget();
        BindingOperations.GetBindingExpressionBase(line, Line.Y2Property).UpdateTarget();
    };

    return act;
}

int CountInvocations(SourceFile source, int depth)
{
    int count = 0;

    if (depth > 0)
    {
        foreach (SourceFile inner in source.getInvocations())
        {
            count = count + CountInvocations(inner, depth - 1);
        }
    }
    else
    {
        count = source.Count;
    }

    return count;
}

Grid MakeGrid(List<Grid> grids, SourceFile source)
{
    Grid grid = new Grid();
    grid.Width = 50;
    grid.Height = 50;
    grid.Tag = source;
    source.setGrid(grid);
    grids.Add(grid);

    Ellipse ellipse = new Ellipse();
    ellipse.Width = 50;
    ellipse.Height = 50;
    ellipse.Fill = Brushes.Red;

    grid.Children.Add(ellipse);

    return grids;
}

void MakeRecursiveGrids(List<Grid> grids, SourceFile outer, SourceFile source,
    int maxDepth, int invocCount, int recurseDepth, ref int counter)
{
    if (recurseDepth > 0)
    {
        foreach (SourceFile inner in source)
        {
            MakeRecursiveGrids(grids, source, inner, maxDepth, invocCount,
                recurseDepth - 1, ref counter);
        }
    }
    else
    {
        MakeGrid(grids, outer, inner, depth, maxDepth, invocCount, counter);
        counter++;
    }
}

Grid MakeGrid(List<Grid> grids, SourceFile outer, SourceFile inner,
    int depth, int[] depths, int invocCount, int counter)
{
    Grid grid = MakeGrid(grids, inner);

    MakeViewbox(grid, grid.Width, grid.Height, inner.getName());

    AdjustTop(depth, depths);
    Canvas.SetLeft(grid, counter * (1000 / (invocCount + 1)));

    MakeLine(grids, grid, outer, inner);

    return grid;
}

Grid MakeOuterGrid(List<Grid> grids, SourceFile inner, int width, int height,
    int depth)
{
    Grid grid = MakeGrid(grids, inner);

    MakeViewbox(grid, width, height, inner.getName());

    AdjustTop(depth, 0);
    Canvas.SetLeft(grid, 500);

    return grid;
}

Binding MakeBinding(Object parameter, Grid grid)
{
    Binding binding = new Binding();
    binding.Path = new PropertyPath(parameter);
    binding.Converter = new MyConverter();
    binding.ConverterParameter = grid;
}

void MakeLine(List<Grid> grids, Grid grid, SourceFile outer, SourceFile inner)
{
    Grid g2 = outer.getGrid();

    Line line = new Line();
    line.Stroke = Brushes.Green;
    line.StrokeThickness = 10;

    Binding x1 = MakeBinding(Canvas.LeftProperty, g2);
    Binding y1 = MakeBinding(Canvas.TopProperty, g2);
    Binding x2 = MakeBinding(Canvas.LeftProperty, grid);
    Binding y2 = MakeBinding(Canvas.TopProperty, grid);

    Grid g = findGrid(grids, outer, inner);
    x1.Source = g;
    y1.Source = g;
    x2.Source = grid;
    y2.Source = grid;

    line.SetBinding(Line.X1Property, x1);
    line.SetBinding(Line.Y1Property, y1);
    line.SetBinding(Line.X2Property, x2);
    line.SetBinding(Line.Y2Property, y2);

    Dependencies.Children.Add(line);

    Contacts.AddPreviewContactDownHandler(line, OnLineDown);

    line.Tag = new Call(outer, inner);

    SizeChangedEventHandler act = UpdateBindings(line);
    inner.getGrid().SizeChanged += act;
    g1.SizeChanged += act;
}

void MakeViewBox(Grid grid, int width, int height, string text)
{
    Viewbox box = new Viewbox();
    box.Width = width;
    box.Height = height;

    TextBox textBox = new TextBox();
    textBox.Text = text;

    box.Child = textBox;

    grid.Children.Add(box);
}
\$\endgroup\$
7
\$\begingroup\$

This should simplify the looping assuming that you can generalize the positioning code like mention in several of the other answers

        //Get the initial set of sourcefiles
        var sourceFiles = from file in list
              from invocation in file.getInvocations()
              group invocation by (SourceFile)null into groupedByInvoker
              select groupedByInvoker;

        for (var currentDepth = 0; currentDepth <= depth; currentDepth++)
        {
            foreach (var currentGroup in sourceFiles)
            {
                int sourceFileCount = currentGroup.Count();
                int counter = 0;

                foreach (var invocation in currentGroup)
                {
                    /*
                     * Generalized grid code goes here
                     */
                    counter++;
                }
            }

            //Select the current sub source files
         sourceFiles = from invokerGroup in sourceFiles
              from file in invokerGroup
              from invocation in file.getInvocations()
              group invocation by file into groupedByInvoker
              select groupedByInvoker;

        }

This is not an exact mapping to the above code in that this goes over the getInvocations tree breadth first instead of depth first.

Updated with imput from Update grid from source hierarchy

\$\endgroup\$
6
\$\begingroup\$

For the n steps bit, consider using recursion - but tread carefully.

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

Instead of Grid g and Ellipse e, use Grid grid and Ellipse ellipse. A loc with e.size= says less than ellipse.Size=.

\$\endgroup\$
  • \$\begingroup\$ You're absoluletly right, but I don't think that this is the real problem here ;) \$\endgroup\$ – RoflcoptrException Jan 27 '11 at 21:47
3
\$\begingroup\$

Break your code into its own methods definitely, there are other ways, but that will probably be the easiest and less time consuming way to make it easier read and debug down the track for a start.

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