5
\$\begingroup\$

We are converting a WinForms application to WPF. so I need to accept the input like WinForms. As I don't want to change the logic of it. Ao I write the class for those its works. But the is not good please take a look at the class once and suggest the edits in this class.

 public class TreeItem : INotifyPropertyChanged
    {
        private TreeItem()
        {
            if (Nodes == null)
                Nodes = new TreeItems();
        }
        private TreeItem(string DisplayName, string IdName) : this()
        {
            this.DisplayName = DisplayName;
            this.IdName = IdName;
        }
        public TreeItem(string DisplayName, string IdName, TreeItem parent = null) : this(DisplayName, IdName)
        {
            Parent = parent;
        }

        bool isChecked = false;
        private string idName = string.Empty;
        string name = string.Empty;
        private object tag;

        public object Tag
        {
            get { return tag; }
            set { tag = value; }
        }

        public bool IsChecked
        {
            get
            {
                return isChecked;
            }
            set
            {
                isChecked = value;
                OnPropertyChanged("IsChecked");
            }
        }
        public string IdName
        {
            get
            {
                return idName;
            }
            set
            {
                idName = value;
                OnPropertyChanged("IdName");
            }
        }
        public string DisplayName
        {
            get
            {
                return name;
            }
            set
            {
                name = value;
                OnPropertyChanged("Name");
            }
        }

        public TreeItem Parent { get; set; }
        private TreeItems nodes;
        public TreeItems Nodes
        {
            get
            {
                return nodes;
            }
            set
            {
                nodes = value;
                foreach (var item in nodes)
                {
                    item.Parent = this;
                }
            }
        }
        public event PropertyChangedEventHandler PropertyChanged;

        public void OnPropertyChanged(string propertyName)
        {
            if (PropertyChanged != null && !string.IsNullOrEmpty(propertyName))
            {
                PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
            }
        }
    }
    public class TreeItems : IList<TreeItem>
    {
        public TreeItem this[string itemName]
        {
            get
            {
                return GetNode(allParents, itemName);
            }
        }
        public TreeItem GetNode(List<TreeItem> currentList, string name)
        {
            foreach (var item in currentList)
            {
                if (item.IdName == name)
                {
                    return item;
                }
            }

            foreach (var item in currentList)
            {
                TreeItem treeItem = GetNode(item.Nodes.allParents, name);
                if (treeItem != null)
                {
                    return treeItem;
                }
            }
            return null;
        }
        List<TreeItem> allParents = new List<TreeItem>();
        public TreeItem this[int index] { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }

        public int Count => throw new NotImplementedException();

        public bool IsReadOnly => throw new NotImplementedException();

        public void Add(TreeItem item)
        {
            allParents.Add(item);
        }
        public void GetParent() { }
        public void Clear()
        {
            allParents.Clear();
        }

        public bool Contains(TreeItem item)
        {
            throw new NotImplementedException();
        }

        public void CopyTo(TreeItem[] array, int arrayIndex)
        {
            throw new NotImplementedException();
        }

        public int IndexOf(TreeItem item)
        {
            throw new NotImplementedException();
        }

        public void Insert(int index, TreeItem item)
        {
            throw new NotImplementedException();
        }

        public bool Remove(TreeItem item)
        {
            throw new NotImplementedException();
        }

        public void RemoveAt(int index)
        {
            throw new NotImplementedException();
        }

        public IEnumerator<TreeItem> GetEnumerator()
        {
            return allParents.GetEnumerator();
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return GetEnumerator();
        }
    }

the way im assigning the parent is bad. so any ideas will be helpful thanks.

Usage:

TreeItems  tvLieferantsItems ;
tvLieferantsItems[betr["PARTID"].ToString()]?.Nodes[betr["BETID"]?.ToString()]?.Nodes[betr["BERID"].ToString()]?.Nodes.Add(abt);
\$\endgroup\$
6
\$\begingroup\$

Review

  • I see no reason to have 3 constructors, each invoking the other, when two of them are private and only called by another constructor. Keep it simple.
  • You check for if (Nodes == null) in the constructor. Is there a scenario where this reference could have already been created upfront?
  • Constructor arguments should be camel cased.
  • There is no point in initializing idName and name because they get overwritten by the constructor chain.
  • Call OnPropertyChanged with nameof(IsChecked) rather than "IsChecked". Or perhaps with a lambda OnPropertyChanged(x => x.IsChecked). This avoids nasty typo's and is more robust against future changes.
  • Property Nodes has a public setter. Check the value against null, and navigational integrity (nodes can not be descendants, ancestors, self or children of a different parent).
  • Property Parent should not have a public setter. Private would do fine here. This way, you avoid consumers messing with the navigational integrity of the tree.
  • PropertyChanged?.Invoke(this, args) is a cleaner way of invoking the event.
  • Method GetNode does not use any instance state or behavior, so it should be made static.
  • Method GetParent is void, which is uncommon for a method named Get*. Its method body is also empty, which is even more uncommon :) What is the purpose of this method?
  • Method Add is public and accepts a parameter. I bet the parameter is not allwoed to be null. Check it against null and throw an appropriate exception.
  • Finish the implementation of the class. Don't throw NotImplementedException for methods you should definitely implement.
\$\endgroup\$
  • 1
    \$\begingroup\$ thanks for that and public TreeItems Nodes { get { return nodes; } set { nodes = value; foreach (var item in nodes) { item.Parent = this; } } }// how can i change this part of the implementation \$\endgroup\$ – Avinash Reddy Aug 12 at 9:22

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.