3
\$\begingroup\$

I am developing a WinForm application that extracts data from our database and draws a tree like structure on the form. For the users to get a better idea of how the data is structured.

The database is designed in such a way, that data looks like tree structures. Based on that I can fetch the data and draw the tree structure.

I am using GraphicsPath objects to draw the nodes and the links between them, and to do hit testing.

Here is the Node class:

public class OINode
{
    #region Const
    public int NodeDiamenter = 55;
    private string FontString = "Tahoma";
    private FontFamily GPFont = new FontFamily("Tahoma");
    private float GPFontSize = 11.0f;
    #endregion

    #region Graphics
    private GraphicsPath GPNode;
    private GraphicsPath GPLabels;
    private Point TopLeft;
    private Size NodeSize;
    private Point _Top;
    private Point _Bottom;
    private bool Selected = false;
    private bool didHit = false;
    #endregion

    #region Properties
    private string _ID = "ID";
    private string _Type = "Type";
    private string _SubType = "SubType";
    private string _KnownByID = "SubType";
    private string _InternalStatus = "InternalStatus";
    private Point _Location = new Point(0, 0);
    private Color _NodePen = Color.Black;
    private Color _NodeBrush = Color.White;
    private Color _NodeSelectPen = Color.Yellow;
    private Color _NodeSelectBrush = Color.WhiteSmoke;
    private int _AssignedLevel = -1;

    public string ID { get { return _ID; } set { _ID = value; } }
    public string Type { get { return _Type; } set { _Type = value; } }
    public string SubType { get { return _SubType; } set { _SubType = value; } }
    public string KnownByID { get { return _KnownByID; } set { _KnownByID = value; } }
    public string InternalStatus { get { return _InternalStatus; } set { _InternalStatus = value; } }
    public Point Location { get { return _Location; } set { _Location = value; ConstuctNode(); } }
    public Color NodePen { get { return _NodePen; } set { _NodePen = value; } }
    public Color NodeBrush { get { return _NodeBrush; } set { _NodeBrush = value; } }
    public Color NodeSelectPen { get { return _NodeSelectPen; } set { _NodeSelectPen = value; } }
    public Color NodeSelectBrush { get { return _NodeSelectBrush; } set { _NodeSelectBrush = value; } }

    public Point Top { get { return _Top; } set { } }
    public Point Bottom { get { return _Bottom; } set { } }

    public int AssignedLevel { get { return _AssignedLevel; } set { _AssignedLevel = value; } }
    #endregion

    #region Links
    public ArrayList ChildLinks = new ArrayList();
    #endregion

    public OINode(string id, string type, string subType, string knownByID, string internalStatus)
    {
        ID = id;
        Type = type;
        SubType = subType;
        KnownByID = knownByID;
        InternalStatus = internalStatus;
    }

    ~OINode()
    {
        if (GPNode != null)
        {
            GPNode.Dispose();
        }
        if (GPLabels != null)
        {
            GPLabels.Dispose();
        }
    }

    public override string ToString()
    {
        return string.Format("ID = {0} Type = {1} SubType = {2}, KnownByID = {3} IS = {4}", ID, Type, SubType, KnownByID, InternalStatus);
    }

    private void Initilzie()
    {
        TopLeft = new Point(Location.X - NodeDiamenter, Location.Y - NodeDiamenter);
        NodeSize = new Size(NodeDiamenter * 2, NodeDiamenter * 2);
        _Top = new Point(Location.X, Location.Y - NodeDiamenter);
        _Bottom = new Point(Location.X, Location.Y + NodeDiamenter);
    }

    private void ConstuctNode()
    {
        Initilzie();
        GPNode = new GraphicsPath();
        GPNode.AddEllipse(new Rectangle(TopLeft, NodeSize));
        GPNode.CloseFigure();

        StringFormat sf = new StringFormat();
        sf.Alignment = StringAlignment.Center;
        sf.LineAlignment = StringAlignment.Center;
        int[] lines = FindLineForLable();

        GPLabels = new GraphicsPath();
        GPLabels.AddString(ID.Substring(9, 9), GPFont, (int)FontStyle.Regular, GPFontSize, BuildCenteredRectangle(ID.Substring(9, 9), lines[0]), sf);
        GPLabels.AddString(Type, GPFont, (int)FontStyle.Regular, GPFontSize, BuildCenteredRectangle(Type, lines[1]), sf);
        GPLabels.AddString(SubType, GPFont, (int)FontStyle.Regular, GPFontSize, BuildCenteredRectangle(SubType, lines[2]), sf);
        GPLabels.AddString(KnownByID, GPFont, (int)FontStyle.Regular, GPFontSize, BuildCenteredRectangle(KnownByID, lines[3]), sf);
        GPLabels.AddString(InternalStatus, GPFont, (int)FontStyle.Regular, GPFontSize, BuildCenteredRectangle(InternalStatus, lines[4]), sf);
        GPLabels.CloseFigure();
    }

    private Rectangle BuildCenteredRectangle(string text, int line)
    {
        Size textSize = TextRenderer.MeasureText(text, new Font(FontString, GPFontSize));
        Rectangle rect = new Rectangle(Location.X - (textSize.Width / 2), line, textSize.Width, textSize.Height);

        return rect;
    }

    private int[] FindLineForLable()
    {
        int TopOfNode = Top.Y;
        int BottomOfNode = Bottom.Y;
        int TextHeight = TextRenderer.MeasureText(ID + Type + SubType + KnownByID + InternalStatus, new Font(FontString, GPFontSize)).Height - 4;
        int[] lines = new int[5];

        lines[0] = TopOfNode + TextHeight;
        lines[1] = TopOfNode + TextHeight * 2;
        lines[2] = TopOfNode + TextHeight * 3;
        lines[3] = TopOfNode + TextHeight * 4;
        lines[4] = TopOfNode + TextHeight * 5;

        return lines;
    }

    public void Draw(Graphics g)
    {
        foreach (OILink link in ChildLinks)
        {
            link.Draw(g);
        }

        Paint(g);
    }

    private void Paint(Graphics g)
    {
        if (Selected)
        {
            g.FillPath(ConvertColorToBrush(NodeSelectBrush), GPNode);
            g.DrawPath(new Pen(NodeSelectPen, 2.5f), GPNode);

            g.FillPath(Brushes.Black, GPLabels);
            g.DrawPath(new Pen(Color.Black, 1f), GPLabels);
        }
        else
        {
            g.FillPath(ConvertColorToBrush(NodeBrush), GPNode);
            g.DrawPath(new Pen(NodePen, 2.5f), GPNode);

            g.FillPath(Brushes.Black, GPLabels);
            g.DrawPath(new Pen(Color.Black, 1f), GPLabels);
        }
    }

    public bool HitTest(Graphics g, MouseEventArgs e)
    {
        bool neededUpdated = false;

        if (GPNode.IsVisible(e.Location))
        {
            if (!didHit)
            {
                didHit = true;

                if (!Selected)
                {
                    Selected = true;

                    neededUpdated = true;

                    foreach (OILink link in ChildLinks)
                    {
                        link.isHit(g, Selected);
                    }

                    Paint(g);
                }
            }
        }
        else
        {
            if (didHit)
            {
                didHit = false;

                Selected = false;

                neededUpdated = true;

                foreach (OILink link in ChildLinks)
                {
                    link.isHit(g, Selected);
                }

                Paint(g);
            }
        }

        if (!neededUpdated)
        {
            foreach (OILink link in ChildLinks)
            {
                if (link.HitTest(g, e))
                {
                    neededUpdated = true;
                }
            }
        }

        return neededUpdated;
    }

    private Brush ConvertColorToBrush(Color color)
    {
        return new SolidBrush(color);
    }

    public void AddChildNode(OINode childNode, string internalStatus, bool repeatLink)
    {
        bool found = false;

        if (repeatLink)
        {
            foreach (OILink link in childNode.ChildLinks)
            {
                if (link.ChildNode.ID == ID)
                {
                    link.Repeat = true;
                    found = true;
                    break;
                }
            }
        }
        else
        {
            found = false;
        }
        if (!found)
        {
            ChildLinks.Add(new OILink(this, childNode, internalStatus));
        }

    }

    public void isHit(Graphics g, bool hit)
    {
        Selected = hit;

        foreach (OILink link in ChildLinks)
        {
            link.isHit(g, Selected);
        }

        Paint(g);
    }
}

And here is the link class:

public class OILink
{
    #region Const
    private const float LinkWidth = 2f;
    private const int GPLinkWidth = 5;
    #endregion

    #region Graphics
    private GraphicsPath GPLink;
    private bool Selected = false;
    public bool Repeat = false;
    private bool didHit = false;
    #endregion

    #region Properties
    private string _InternalStatus = "InternalStatus";
    private Color _Start = Color.LightBlue;
    private Color _End = Color.Salmon;
    private Color _StartSelected = Color.Blue;
    private Color _EndSelected = Color.Red;

    public string InternalStatus { get { return _InternalStatus; } set { _InternalStatus = value; } }
    public Color Start { get { return _Start; } set { _Start = value; } }
    public Color End { get { return _End; } set { _End = value; } }
    public Color StartSelected { get { return _StartSelected; } set { _StartSelected = value; } }
    public Color EndSelected { get { return _EndSelected; } set { _EndSelected = value; } }
    #endregion

    #region Nodes
    public OINode ParentNode;
    public OINode ChildNode;
    #endregion

    public OILink(OINode parentNode, OINode childNode, string internalStatus)
    {
        InternalStatus = internalStatus;
        ParentNode = parentNode;
        ChildNode = childNode;
    }

    ~OILink()
    {
        if (GPLink != null)
        {
            GPLink.Dispose();
        }
    }

    public override string ToString()
    {
        return string.Format("PID = {0} CID = {1} LIS = {2}", ParentNode.ID, ChildNode.ID, InternalStatus);
    }

    private void ConstuctLink()
    {
        if ((ParentNode.Location.X != 0) && (ParentNode.Location.Y != 0) && (ChildNode.Location.X != 0) && (ChildNode.Location.Y != 0))
        {
            GPLink = new GraphicsPath();
            GPLink.AddLine(ParentNode.Bottom.X - GPLinkWidth, ParentNode.Bottom.Y, ParentNode.Bottom.X + GPLinkWidth, ParentNode.Bottom.Y);
            GPLink.AddLine(ParentNode.Bottom.X + GPLinkWidth, ParentNode.Bottom.Y, ChildNode.Top.X + GPLinkWidth, ChildNode.Top.Y);
            GPLink.AddLine(ChildNode.Top.X + GPLinkWidth, ChildNode.Top.Y, ChildNode.Top.X - GPLinkWidth, ChildNode.Top.Y);
            GPLink.AddLine(ChildNode.Top.X - GPLinkWidth, ChildNode.Top.Y, ParentNode.Bottom.X - GPLinkWidth, ParentNode.Bottom.Y);
            GPLink.CloseFigure();
        }
    }

    public void Draw(Graphics g)
    {
        ConstuctLink();

        if (GPLink != null)
        {
            Paint(g);

            ChildNode.Draw(g);
        }
    }

    private void Paint(Graphics g)
    {
        if (Selected)
        {
            LinearGradientBrush brush = new LinearGradientBrush(ParentNode.Location, ChildNode.Location, StartSelected, EndSelected);
            g.FillPath(brush, GPLink);
            g.DrawPath(new Pen(brush, LinkWidth), GPLink);
            brush.Dispose();
        }
        else
        {
            LinearGradientBrush brush = new LinearGradientBrush(ParentNode.Location, ChildNode.Location, Start, End);
            g.FillPath(brush, GPLink);
            g.DrawPath(new Pen(brush, LinkWidth), GPLink);
            brush.Dispose();
        }
    }

    public bool HitTest(Graphics g, MouseEventArgs e)
    {
        bool neededUpdated = false;

        if (GPLink != null)
        {
            if (GPLink.IsVisible(e.Location))
            {
                if (!didHit)
                {
                    didHit = true;

                    neededUpdated = true;

                    if (!Selected)
                    {
                        Selected = true;

                        ChildNode.isHit(g, Selected);

                        Paint(g);
                    }
                }
            }
            else
            {
                if (didHit)
                {
                    didHit = false;

                    Selected = false;

                    neededUpdated = true;

                    ChildNode.isHit(g, Selected);

                    Paint(g);
                }
            }
            if (!neededUpdated)
            {
                if (ChildNode.HitTest(g, e))
                {
                    neededUpdated = true;
                }
            }
        }

        return neededUpdated;
    }

    private Brush ConvertColorToBrush(Color color)
    {
        return new SolidBrush(color);
    }

    private bool CheckForRepeatLinks(OINode parentNode, OINode childNode)
    {
        foreach (OILink link in childNode.ChildLinks)
        {
            if (link.ChildNode == childNode)
            {
                Repeat = true;
                link.Repeat = Repeat;

                return Repeat;
            }
        }           

        Repeat = false;
        return Repeat;
    }

    public void isHit(Graphics g, bool hit)
    {
        Selected = hit;

        ChildNode.isHit(g, Selected);

        Paint(g);
    }
}

With these two classes I can make tree structures like so:

OINode node1 = new Node("node1", "", "", "", "");
OINode node2 = new Node("node2", "", "", "", "");
OINode node3 = new Node("node3", "", "", "", "");
OINode node4 = new Node("node4", "", "", "", "");
OINode node5 = new Node("node5", "", "", "", "");
OINode node6 = new Node("node6", "", "", "", "");

node1.AddChildNode(node2, "", false);
node2.AddChildNode(node3, "", false);
node2.AddChildNode(node4, "", false);
node3.AddChildNode(node5, "", false);
node4.AddChildNode(node6, "", false);

Then I draw this onto Bitmap, witch displays in a PictureBox, allowing me to scroll with bigger trees structures.

How the drawing works: All the OINodes objects are link by the OILink objects. So all I have to do, is call the first node node1.HitTest(...) and it will loop through all its children links and nodes. Each one calling the Paint(...) method to redraw itself if need be. If the hit has proven to be true if (GPNode.IsVisible(e.Location)) (see the HitTest(...) method)

Question: This work well for not too big structures, as soon as it gets bigger it lags a lot to redraw the structures.

Is there a better design I can follow, to render the structure?

Also to keep in mind, I want to extend on this application to allow the user to interact with the nodes and links. By clicking on the objects the user can view other data associated with that object in the database. I was thinking of an interactive hover menu. When the mouse hovers or the user clicks on an object, it display basic information about the object, and has buttons (on the hover menu), that the user can click on to bring up more detailed information

I was thinking of using, user controls, for the node and link objects, but it was suggested to me an the Stack Overflow Chat to inherit from Control class instead.

EDIT

Here is the code used to draw the structure. I am using a user control that has a Panel and in the panel is has a picture box as suggested in the comments of my previous post here

The Code:

    private void DrawPicture(int Width, int Height)
    {
        TreeStructure = new Bitmap(Width, Height, PixelFormat.Format24bppRgb);
        Graphics g = this.CreateGraphics();
        g = Graphics.FromImage(TreeStructure);

        // Just draws the background
        DrawBack(g, Width, Height);

        Construct(g);
        StructurePictureBox.Size = new Size(Width, Height);
        StructurePictureBox.Image = TreeStructure;

        g.Dispose();
    }

    private void Construct(Graphics g)
    {
        // edit for this post
        node1.Draw(g);
    }
\$\endgroup\$
  • \$\begingroup\$ have I overlooked something, or is it true that each node's Location has to be manually assigned? \$\endgroup\$ – Adam Sep 7 '12 at 14:02
  • \$\begingroup\$ @codesparkle you are right, but that piece of code is irelavent to this question. I have other classes that set the location. \$\endgroup\$ – ZioN Sep 7 '12 at 14:23
3
\$\begingroup\$

I am not going to look at the details of your code (Jeff did that already); rather, I want to make some comments on the architecture here.

Recently I had the task of drawing a tree structure (an organigram to be precise) starting with Excel data. I split my program into several components, each one of them responsible for one aspect of the task. The idea is to have a greater flexibility. E.g. the customer might want to retrieve the data from a database in future, instead of an Excel worksheet. In my case I had to change the graphics technology. I started by creating the organigram through Visio automation. I worked but was extremely slow. Now I am creating SVG graphics. This is much faster and the graphic can be opened in Visio. The change was easy to implement, thanks to my good architecture.

I split my logic into these tasks, each one of them realized as a separate component:

  1. Read the data source
  2. Build a logical tree structure
  3. Layout the tree
  4. Draw the diagram

The point here is that the layouter does not need to know which technology is used for drawing the graphic. The layouter creates a list of shapes using technology independent shapes.

public interface IShape
{
    double Top { get; }
    double Left { get; }
    double Height { get; }
    double Width { get; }

    // Calculated
    double Right { get; }
    double Bottom { get; }

    void Render(); // This method does all the technology dependent stuff.
}

public interface ILine : IShape
{
    LineOrientation Orientation { get; }
    Color LineColor { get; }
    double LineWidth { get; }
}

And so on. I have polygon shapes, text shapes, rectangle shapes and more. An important part is the shape factory. Concrete shape factories will produce technology dependent shapes like Visio shapes or SVG shapes.

public interface IShapeFactory
{
    IDiagram CreateDiagram(List<IShape> shapes);
    ILine CreateLine(PointD p0, PointD p1, Color lineColor, double lineWidth);
    IPolygon CreatePolygon(List<PointD> points, Color fillColor,
                           Color borderColor, double borderWidth);
    IRectangle CreateRectangle(RectangleD rect, Color fillColor,
                               Color borderColor, double borderWidth,
                               double cornerRadius);
    ISymbol CreateSymbol(RectangleD rect, IService<IDiagramNode> service);
    IText CreateText(RectangleD rect, string text, string fontName,
                     double pointSize, Color color, TextAlignment alignment,
                     TextStyle style, TextOrientation orientation);
}

The layouter creates the requested shapes through an IShapeFactory that is injected through its constructor and does not need to know which graphics system will be used.

\$\endgroup\$
  • \$\begingroup\$ I like you architecture, I always strive to develop my programs like this. But I think my components 2, and 4 (Build a logical tree structure, and Draw the diagram) are in one. My next step after I improve the speed of drawing the structure is too allow the users to interact with the Nodes. So I am not simply develop just graphics, but more like controls. \$\endgroup\$ – ZioN Sep 13 '12 at 10:48
  • \$\begingroup\$ My step 1 (read the data source) yields a list objects (persons in the case of an organigram). Step 2 (build a logical tree structure) creates a tree of objects in memory (only references are set, no coordinates). Step 3 (layout the tree) calculates the coordinates of the objects and creates junction lines in memory. The last step (#4) finally creates a graphics. The reason step 4 is a separate step is to allow all previous steps to have no reference to types referring to a specific graphics technology. But of cause, you must adapt the architecture to your needs. \$\endgroup\$ – Olivier Jacot-Descombes Sep 13 '12 at 15:41
  • \$\begingroup\$ Well thank you for your input, it has been helpful, and I think I most likely have to do redesign of my program, not getting the results I want anyway (speed wise). And your architecture is a step in the right direction I believe. \$\endgroup\$ – ZioN Sep 14 '12 at 5:47
  • \$\begingroup\$ Enabling double buffering can help speed up drawing. \$\endgroup\$ – Olivier Jacot-Descombes Sep 14 '12 at 17:51
  • \$\begingroup\$ I have already done that, but this is clearly an architecture issue. \$\endgroup\$ – ZioN Sep 17 '12 at 5:39
2
\$\begingroup\$

First things first, lose the #region tags. They do nothing but clutter up your code.

I would make all of the class variables that are assigned at the top of OINode constants

public const int NodeDiamenter = 55;
private const string FontString = "Tahoma";
private readonly FontFamily GPFont = new FontFamily("Tahoma");
private const float GPFontSize = 11.0f;

Your class variables do not follow standardized naming conventions. They should be camel case, prefixed with a _

private GraphicsPath _gpNode;
private GraphicsPath _gpLabels;
private Point _topLeft;
private Size _nodeSize;
private Point _top;
private Point _bottom;
private bool _selected;
private bool _didHit;

All of the properties can be set as autoproperty:

public string ID { get; set; }
public string Type { get; set; }
public string SubType { get; set; }
public string KnownByID { get; set; }
public string InternalStatus { get; set; }
public Point Location { get; set; }
public Color NodePen { get; set; }
public Color NodeBrush { get; set; }
public Color NodeSelectPen { get; set; }
public Color NodeSelectBrush { get; set; }
public int AssignedLevel { get; set; }

Then set defaults in the constructor:

Location = new Point(0, 0);
NodePen = Color.Black;
NodeBrush = Color.White;
NodeSelectPen = Color.Yellow;
NodeSelectBrush = Color.WhiteSmoke;
AssignedLevel = -1;

If you want a readonly property, just don't put the set in:

public Point Top { get { return _top; } }
public Point Bottom { get { return _bottom; } }

I would change the ChildLinks ArrayList to a list of OILink and make it a readonly property:

public List<OILink> ChildLinks { get; private set; }

Use the var keyword and object initializer where ever possible:

var sf = new StringFormat
        {
            Alignment = StringAlignment.Center, 
            LineAlignment = StringAlignment.Center
        };

The localvariable in FindLineForLable

var bottomOfNode = Bottom.Y;

is not used. Remove it to avoid confusion.

Your Paint method has duplicate code. I would assign the brush then do the paint

private void Paint(Graphics g)
{
    var fillBrush = _selected ? NodeSelectBrush : NodeBrush;

    g.FillPath(ConvertColorToBrush(fillBrush), _gpNode);
    g.DrawPath(new Pen(NodePen, 2.5f), _gpNode);

    g.FillPath(Brushes.Black, _gpLabels);
    g.DrawPath(new Pen(Color.Black, 1f), _gpLabels);
}

HitTest has too many nested ifs.

I would do it something like this

public bool HitTest(Graphics g, MouseEventArgs e)
{
    var neededUpdated = _gpNode.IsVisible(e.Location) ? ProcessNodeIfNotHit(g) : ProcessNodeIfHit(g);

    Paint(g);

    if (!neededUpdated)
    {
        neededUpdated = ChildLinks.Any(l => l.HitTest(g, e));
    }

    return neededUpdated;
}

private bool ProcessNodeIfNotHit(Graphics g)
{
    if (!_didHit)
    {
        _didHit = true;

        if (!_selected)
        {
            _selected = true;

            UpdateNodeHitStatus(true, g);
        }

        return true;
    }

    return false;
}

private bool ProcessNodeIfHit(Graphics g)
{
    if (_didHit)
    {
        UpdateNodeHitStatus(g);

        return true;
    }

    return false;
}

private void UpdateNodeHitStatus(bool status, Graphics g)
{   
    _didHit = status;
    _selected = status;

    foreach (OILink link in ChildLinks)
    {
        link.isHit(g, _selected);
    }
}

AddChildNode has an unneccessary loop in it, the linq FirstOrDefault is a much more efficient and clean way of finding one element. You could also use SingleOrDefault if you think that will work better:

public void AddChildNode(OINode childNode, string internalStatus, bool repeatLink)
{
    var found = false;

    if (repeatLink)
    {
        var link = childNode.ChildLinks.FirstOrDefault(l => l.ChildNode.ID == ID);

        if (link != null)
        {
            link.Repeat = true;
            found = true;
        }
    }

    if (!found)
    {
        ChildLinks.Add(new OILink(this, childNode, internalStatus));
    }

}

The method isHit should be changed to IsHit to keep with C# coding standards.

For OILink class variables and properties, see above.

In ConstructLink, I would change this if statement and pull it out into its own method. The method name would better portray what the if statement is checking, that way when you come back to fix something in the code, you can glance over it instead of trying to figure out what it does:

if ((ParentNode.Location.X != 0) && (ParentNode.Location.Y != 0) && (ChildNode.Location.X != 0) && (ChildNode.Location.Y != 0))
{
    // CODE HERE
}

to

if (ListHasNotSizeSet())
{
     return;
}

// CODE HERE

private bool ListHasNotSizeSet()
    {
        return ParentNode.Location.X == 0 || ParentNode.Location.Y == 0 || ChildNode.Location.X == 0 ||
               ChildNode.Location.Y == 0;
    }

In draw, check for null then return if its found. This will reduce nesting in your class.

public void Draw(Graphics g)
{
    ConstuctLink();

    if (GPLink == null)
    {
        return;
    }

    Paint(g);

    ChildNode.Draw(g);
}

In your Paint method, it would be better to determine the brushes, then set up the paint. You should also create the LinearGradientBrush using the using keyword. This will eliminate duplication and unneccesary code:

private void Paint(Graphics g)
{
    var startBrush = Selected ? StartSelected : Start;
    var endBrush = Selected ? EndSelected : End;

    using (var brush = new LinearGradientBrush(ParentNode.Location, ChildNode.Location, startBrush, endBrush))
    {
        g.FillPath(brush, GPLink);
        g.DrawPath(new Pen(brush, LinkWidth), GPLink);
    }
}

For HitTest, see above.

in CheckForRepeatLinks, there is a number of things you can do. The parameter parentNode is not used, get rid of it. You can use the .FirstOrDefault linq statement instead of a foreach loop:

private bool CheckForRepeatLinks(OINode childNode)
{
    var link = childNode.ChildLinks.FirstOrDefault(l => l.ChildNode == childNode);

    if (link == null)
    {
        Repeat = false;
        return Repeat;
    }

    link.Repeat = true;
    Repeat = true;
    return Repeat;
}

As for the code that draws the picture, I would again use the using keyword for the Graphics variable. I also noticed you are assigning g twice in 2 lines, is that really necessary?

private void DrawPicture(int Width, int Height)
{
    TreeStructure = new Bitmap(Width, Height, PixelFormat.Format24bppRgb);
    using (var g = Graphics.FromImage(TreeStructure))
    {
        // Just draws the background
        DrawBack(g, Width, Height);

        Construct(g);
        StructurePictureBox.Size = new Size(Width, Height);
        StructurePictureBox.Image = TreeStructure;
    }        
}

As for creating the classes, because you now have defaults in the constructor, you only have to pass in the parts needed

var node1 = new Node("node1");
var node2 = new Node("node2");
var node3 = new Node("node3");
var node4 = new Node("node4");
var node5 = new Node("node5");
var node6 = new Node("node6");

node1.AddChildNode(node2, "", false);
node2.AddChildNode(node3, "", false);
node2.AddChildNode(node4, "", false);
node3.AddChildNode(node5, "", false);
node4.AddChildNode(node6, "", false);
\$\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.