-3
\$\begingroup\$

I have function that builds different types of boxes.

namespace TestSome.ViewModel
{
    public class OneBoxViewModel : ViewModelBase
    {
        private ObservableCollection<BaseBox> boxes;
        public ObservableCollection<BaseBox> Boxes
        {
            get
            {
                return this.boxes;
            }

            set
            {
                this.boxes = value;
                this.RaisePropertyChanged("Boxes");
            }
        }
        OptionEnum CurrentEnum = OptionEnum.Quick;

        public OneBoxViewModel()
        {
            Boxes = new ObservableCollection<BaseBox>();
            Messenger.Default.Register<SelectorCommunicator>(this, (emp) =>
            {
                DrawBox(emp.SelectedAssets,emp.Option);
            });
        }

        public void DrawBox(List<string> id, OptionEnum Option)
        {
            if (CurrentEnum!=Option)
            {
                Boxes = new ObservableCollection<BaseBox>();
                CurrentEnum = Option;
            }
            switch(Option)
            {
                case OptionEnum.HighLow:
                    List<string> WhatExist1 = Boxes.OfType<HighLowBox>().Select(key=>key.Box.AssetID.ToString()).ToList();
                    List<string> WhatAdd1 = id.Except(WhatExist1).ToList();
                    List<string> WhatRemove1 = WhatExist1.Except(id).ToList();
                    foreach (string item in WhatAdd1)
                    {
                        Boxes.Add(new HighLowBox(int.Parse(item), SocketHandler.Socket));
                    }
                    foreach(string item in WhatRemove1)
                    {
                        HighLowBox box = Boxes.OfType<HighLowBox>().Where(key => key.Box.AssetID.ToString() == item).Select(key => key).FirstOrDefault();
                        box.Dispose();
                        Boxes.Remove(box);
                    }
                break;

                case OptionEnum.OneTouch:
                    List<string> WhatExist2 = Boxes.OfType<OneTouchBox>().Select(key=>key.Box.AssetID.ToString()).ToList();
                    List<string> WhatAdd2 = id.Except(WhatExist2).ToList();
                    List<string> WhatRemove2 = WhatExist2.Except(id).ToList();
                    foreach (string item in WhatAdd2)
                    {
                        Boxes.Add(new OneTouchBox(int.Parse(item), SocketHandler.Socket));
                    }
                    foreach(string item in WhatRemove2)
                    {
                        OneTouchBox box = Boxes.OfType<OneTouchBox>().Where(key => key.Box.AssetID.ToString() == item).Select(key => key).FirstOrDefault();
                        box.Dispose();
                        Boxes.Remove(box);
                    }
                break;

                case OptionEnum.Quick:
                    List<string> WhatExist = Boxes.OfType<Quickbox>().Select(key=>key.Box.AssetID.ToString()).ToList();
                    List<string> WhatAdd = id.Except(WhatExist).ToList();
                    List<string> WhatRemove = WhatExist.Except(id).ToList();
                    foreach (string item in WhatAdd)
                    {
                        Boxes.Add(new Quickbox(int.Parse(item),SocketHandler.Socket));
                    }
                    foreach(string item in WhatRemove)
                    {
                        Quickbox box = Boxes.OfType<Quickbox>().Where(key => key.Box.AssetID.ToString() == item).Select(key => key).FirstOrDefault();
                        box.Dispose();
                        Boxes.Remove(box);
                    }
                break;

                case OptionEnum.Range:
                    List<string> WhatExist3 = Boxes.OfType<RangeBox>().Select(key=>key.Box.AssetID.ToString()).ToList();
                    List<string> WhatAdd3 = id.Except(WhatExist3).ToList();
                    List<string> WhatRemove3 = WhatExist3.Except(id).ToList();
                    foreach (string item in WhatAdd3)
                    {
                        Boxes.Add(new RangeBox(int.Parse(item), SocketHandler.Socket));
                    }
                    foreach(string item in WhatRemove3)
                    {
                        RangeBox box = Boxes.OfType<RangeBox>().Where(key => key.Box.AssetID.ToString() == item).Select(key => key).FirstOrDefault();
                        box.Dispose();
                        Boxes.Remove(box);
                    }
                break;
            }
        }
    }
}

Also i have such hierarchy

public abstract class BaseBox
{
}
public class QuickBox : BaseBox 
{ 
}
public class RangeBox: BaseBox  
{ 
}
public class OneTouchBox: BaseBox 
{ 
}
public class HighLowBox: BaseBox 
{ 
}

Every case in that switch block contains very similar code. Could it be refactored to leverage generics?

\$\endgroup\$
  • 3
    \$\begingroup\$ Some users seem to deem this question unclear - feel free to edit your post to include a more detailed description of what your code is doing. \$\endgroup\$ – Mathieu Guindon Dec 10 '15 at 14:50
  • 1
    \$\begingroup\$ Try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Dec 10 '15 at 15:37
  • \$\begingroup\$ Do the classes QuickBox, RangeBox, etc. have any code in them? Is the abstract clsas BaseBox empty? If not, please add the real code in. If so, carry on. \$\endgroup\$ – Fund Monica's Lawsuit Dec 10 '15 at 17:55
3
\$\begingroup\$

Well done for spotting the duplication and wanting to fix it! You are correct that you can use generics to solve this problem. Here's some approximate code to start you off in the right direction:

private void UpdateBoxes<T>(IEnumerable<string> ids) where T : BaseBox
{
    var currentBoxAssetIds = Boxes.OfType<T>().Select(key => key.Box.AssetID.ToString()).ToList();
    var assetIdsToAdd = ids.Except(currentBoxAssetIds);
    var assetIdsToRemove = currentBoxAssetIds.Except(ids);

    foreach (string assetId in assetIdsToAdd)
    {
        // ** see below
        var newBox = (T)Activator.CreateInstance(typeof(T), int.Parse(assetId), SocketHandler.Socket);
        Boxes.Add(newBox );
    }
    foreach(string assetId in assetIdsToRemove)
    {
        var box = Boxes.OfType<T>().Select(key => key.Box).FirstOrDefault(box => box.AssetID.ToString() == item);
        box.Dispose();
        Boxes.Remove(box);
    }
}

You'll notice that I've changed almost all of the names in your code because yours weren't very descriptive.

** Creating the instances of the T is the most difficult part. You could solve that by adding properties on the base class and a parameterless constructor so you could simply do:

var newBox = new T();
newBox.Property1 = "some value";

There are lots of other ways you could solve it for example, by using reflection to instantiate the class.

Update:

var newBox = (T)Activator.CreateInstance(typeof(T), int.Parse(assetId), SocketHandler.Socket);

Also why are you converting ints to strings to compare them?

\$\endgroup\$
  • 2
    \$\begingroup\$ I'm of the opinion that you shouldn't be giving reviewee's homework; I don't think omitting a section of a review lends itself well to an "interesting learning opportunity" and (personally) sounds patronising. \$\endgroup\$ – Dan Pantry Dec 10 '15 at 14:56
  • 1
    \$\begingroup\$ @DanPantry I certainly didn't mean to be patronising - I'll edit that bit out. I can see the point you're trying to make but I giving someone code without them having to think about it isn't going to help them solve a problem in future \$\endgroup\$ – RobH Dec 10 '15 at 14:58
  • 2
    \$\begingroup\$ While educating the OP is often a by-product of Code Review. The purpose of Code Review is to make *code* better. For the OP and for anyone who reads the question in the future. As such, deliberately leaving out potential parts of a review is actively detrimental to the site's purpose. \$\endgroup\$ – Kaz Dec 10 '15 at 15:00
  • \$\begingroup\$ @Zak - An interesting take on it - thanks. I've updated the answer with a couple of options. \$\endgroup\$ – RobH Dec 10 '15 at 15:03
  • \$\begingroup\$ But what if base class is abstract and he have not such properties as derived? \$\endgroup\$ – A191919 Dec 10 '15 at 15:07

Not the answer you're looking for? Browse other questions tagged or ask your own question.