Please critique this method

Question

Hi I've been looking around the net for some tab button close functionality, but all those solutions had some complicated eventhandler, and i wanted to try and keep it simple, but I might have broken good code ethics doing so, so please review this method and tell me what is wrong.

   public void AddCloseItem(string header, object content){

   //Create tabitem with header and content
   StackPanel headerPanel = new StackPanel() { Orientation = Orientation.Horizontal, Height = 14};
   headerPanel.Children.Add(new TextBlock() { Text = header });
   Button closeBtn = new Button() { Content = new Image() { Source = new BitmapImage(new Uri("images/cross.png", UriKind.Relative)) }, Margin = new Thickness() { Left = 10 } };
   headerPanel.Children.Add(closeBtn);
   TabItem newTabItem = new TabItem() { Header = headerPanel, Content = content };

   //Add close button functionality
   closeBtn.Tag = newTabItem;
   closeBtn.Click += new RoutedEventHandler(closeBtn_Click);

   //Add item to list
   this.Add(newTabItem);
  }

  void closeBtn_Click(object sender, RoutedEventArgs e)
  {
   this.Remove((TabItem)((Button)sender).Tag);
  }

So what I'm doing is storing the tabitem in the btn.Tag property, and then when the button is clicked i just remove the tabitem from my observablecollection, and the UI is updated appropriately.

Am I using too much memory saving the tabitem to the Tag property?

Answer 1

I don't know enough about WPF to say whether this is the best way to achieve what you want but I wouldn't have thought that memory usage would be a concern here.

When you do

closeBtn.Tag = newTabItem;

you're only storing a reference to newTabItem in the closeBtn.Tag property.

The TabItem object will need to stay in memory for as long as it is displayed in any case.

Answer 2

Since you're asking for feedback on your code I'd recommend you take a look at binding your tab list to an ObservableCollection of dataitems and using a DataTemplate to define the looks of each tab.

Take a look at Josh Smith's excellent MSDN article about MVVM where he has example code that adds and removes tab items without touching the UI code.

As for having a close button on the actual tab, there's an example here that subclasses the TabItem. Of course, you don't really have to subclass it - you could just re-define the template of the standard TabItem. If you decide to use MVVM design pattern for your application (you should!) you could bind the close button to a command inside your viewmodel that simply removes the data object from the aforementioned ObservableCollection.