工作中发现一个代码重构的例子

上个月做了一个ListBox的内容上下移动，有段代码之前看着一直不爽，后来改了又改，现在总算看得顺眼点了，所以想拿出来跟大家探讨下。并且可以用来做笔试题。

开始的代码是这样的:

class RecodeExample
    {
        public object[] Pages
        {
            get;
            set;
        }

        public void MoveUpPage(int selectedIndex = 1)
        {
            if (selectedIndex == 0 || selectedIndex == LocalConfiguration.Pages.Count())
            {
                return;
            }

            object[] localItems = LocalConfiguration.Pages;
            object[] groupItems = GroupConfiguration.Pages;

            if (selectedIndex < localItems.Count())
            {
                List<object> pages = localItems.ToList();

                // Update the list of local config item.
                object selectedPage = pages[selectedIndex];
                pages.RemoveAt(selectedIndex);
                pages.Insert(selectedIndex - 1, selectedPage);

                localItems = pages.ToArray();
            }
            else
            {
                List<object> ticker = groupItems.ToList();

                // Update the list of group config item.
                int position = selectedIndex - localItems.Count();

                object selectedTicker = ticker[position];
                ticker.RemoveAt(position);
                ticker.Insert(position - 1, selectedTicker);

                groupItems = ticker.ToArray();
            }

            LocalConfiguration.Pages = localItems;
            GroupConfiguration.Pages = groupItems;
        }

        public void MoveDownPage(int selectedIndex = 1)
        {
            if (selectedIndex == (LocalConfiguration.Pages.Count() - 1)
            || selectedIndex == (LocalConfiguration.Pages.Count() + GroupConfiguration.Pages.Count() - 1))
            {
                return;
            }

            object[] localItems = LocalConfiguration.Pages;
            object[] groupItems = GroupConfiguration.Pages;

            if (selectedIndex < localItems.Count())
            {
                List<object> pages = localItems.ToList();

                // Update the list of local config item.
                object selectedPage = pages[selectedIndex];
                pages.RemoveAt(selectedIndex);
                pages.Insert(selectedIndex + 1, selectedPage);

                localItems = pages.ToArray();
            }
            else
            {
                List<object> ticker = groupItems.ToList();

                // Update the list of group config item.
                int position = selectedIndex - localItems.Count();

                object selectedTicker = ticker[position];
                ticker.RemoveAt(position);
                ticker.Insert(position + 1, selectedTicker);

                groupItems = ticker.ToArray();
            }

            LocalConfiguration.Pages = localItems;
            GroupConfiguration.Pages = groupItems;
        }
    }

 public class LocalConfiguration
    {
        public static object[] Pages
        {
            get;
            set;
        }
    }

    public class GroupConfiguration
    {
        public static object[] Pages
        {
            get;
            set;
        }
    }

针对上面的代码，我一个同事做了这样的重构, 如下:

    class RecodeVersion2 : RecodeExample
    {        
        public void MoveUpPage(int selectedIndex = 1)
        {
            object[] localMarkee = LocalConfiguration.Pages;
            object[] groupMarkee = GroupConfiguration.Pages;

            this.UpdateConfigList(true, selectedIndex, ref localMarkee, ref groupMarkee);

            LocalConfiguration.Pages = localMarkee;
            GroupConfiguration.Pages = groupMarkee;
        }

        public void MoveDownPage(int selectedIndex = 1)
        {        
            object[] localPages = LocalConfiguration.Pages;
            object[] groupPages = GroupConfiguration.Pages;

            this.UpdateConfigList(false, selectedIndex, ref localPages, ref groupPages);

            LocalConfiguration.Pages = localPages;
            GroupConfiguration.Pages = groupPages;
        }

        /// <summary>
        /// Move up or move down the selected configuration item.
        /// </summary>
        /// <param name="isMoveUp">If move up it will be true or false.</param>
        /// <param name="selectedIndex">The index of the selected item in the listbox.</param>
        /// <param name="localItems">Local configuration.</param>
        /// <param name="groupItems">Group configuration.</param>
        private void UpdateConfigList(bool isMoveUp, int selectedIndex, ref object[] localItems, ref object[] groupItems)
        {
            int step = -1;
            if (!isMoveUp)
            {
                step = 1;
            }

            // The edge item will not to be moved up or move down.
            if (isMoveUp)
            {
                if (selectedIndex == 0 || selectedIndex == localItems.Count())
                {
                    return;
                }
            }
            else
            {
                if (selectedIndex == (localItems.Count() - 1) || selectedIndex == (localItems.Count() + groupItems.Count() - 1))
                {
                    return;
                }
            }

            if (selectedIndex < localItems.Count())
            {
                List<object> ticker = localItems.ToList();

                // Update the list of local config item.
                object selectedTicker = ticker[selectedIndex];
                ticker.RemoveAt(selectedIndex);
                ticker.Insert(selectedIndex + step, selectedTicker);

                localItems = ticker.ToArray();
            }
            else
            {
                List<object> ticker = groupItems.ToList();

                // Update the list of group config item.
                int position = selectedIndex - localItems.Count();

                object selectedTicker = ticker[position];
                ticker.RemoveAt(position);
                ticker.Insert(position + step, selectedTicker);

                groupItems = ticker.ToArray();
            }
        }
    }

我开始做了这样的重构, 如下:

class RecodeVersion3 : RecodeExample
    {
        public new void MoveUpPage(int selectedIndex = 1)
        {
            // No non-local items can be porten into local item lists. 
            if (selectedIndex == LocalConfiguration.Pages.Count())
            {
                return;
            }

            int index = selectedIndex;
            List<Object> pages = new List<Object>();

            if ((LocalConfiguration.Pages.Count() > 0)
                && (index > LocalConfiguration.Pages.Count()))
            {
                index = selectedIndex - LocalConfiguration.Pages.Count();
            }

            if (selectedIndex < LocalConfiguration.Pages.Count())
            {
                pages = MoveUpReset(index, LocalConfiguration.Pages.ToList());
                LocalConfiguration.Pages = pages.ToArray();
            }
            else
            {
                pages = MoveUpReset(index, GroupConfiguration.Pages.ToList());
                GroupConfiguration.Pages = pages.ToArray();
            }
        }

        public new void MoveDownPage(int selectedIndex = 1)
        {
            // No local items can be porten into non-local items.
            if ((selectedIndex + 1 == LocalConfiguration.Pages.Count())
                && (LocalConfiguration.Pages.Count() > 0))
            {
                return;
            }

            int index = selectedIndex;
            List<Object> pages = new List<Object>();

            if ((LocalConfiguration.Pages.Count() > 0)
                && (index > LocalConfiguration.Pages.Count()))
            {
                index = selectedIndex - LocalConfiguration.Pages.Count();
            }

            if (selectedIndex < LocalConfiguration.Pages.Count())
            {
                pages = MoveDownReset(index, LocalConfiguration.Pages.ToList());
                LocalConfiguration.Pages = pages.ToArray();
            }
            else
            {
                pages = MoveDownReset(index, GroupConfiguration.Pages.ToList());
                GroupConfiguration.Pages = pages.ToArray();
            }
        }

        private static List<Object> MoveUpReset(int index, List<Object> pages)
        {
            Object item = pages.ElementAt(index);
            pages.RemoveAt(index);
            pages.Insert(index - 1, item);
            return pages;
        }

        private static List<Object> MoveDownReset(int index, List<Object> pages)
        {
            Object item = pages.ElementAt(index);
            pages.RemoveAt(index);
            pages.Insert(index + 1, item);
            return pages;
        }
    }

发现虽然把业务逻辑封装了，但是代码有一个硬伤，就是数据类型的问题，LocalConfiguration.Pages的类型是数组，使用起来不方便，改成泛型，代码就好多了。

class RecodeVersion4 : RecodeExample
    {
        public List<object> Pages
        {
            get;
            set;
        }

        public new void MoveUpPage(int selectedIndex = 1)
        {
            Up(selectedIndex, LocalConfiguration.OverridePages, GroupConfiguration.OverridePages);
        }

        public new void MoveDownPage(int selectedIndex = 1)
        {
            Down(selectedIndex, LocalConfiguration.OverridePages, GroupConfiguration.OverridePages);
        }

        private void Down(int selectedIndex, List<Object> local, List<Object> group)
        {            
            // No local items can be porten into non-local items.
            if ((selectedIndex + 1 == local.Count())
                && (0 < local.Count()))
            {
                return;
            }

            InnerReset(selectedIndex, local, group, selectedIndex, new Action(MoveDownReset));
        }

        private void Up(int selectedIndex, List<Object> local, List<Object> group)
        {
            // No non-local items can be porten into local item lists. 
            if (selectedIndex == local.Count())
            {
                return;
            }

            InnerReset(selectedIndex, local, group, selectedIndex, new Action(MoveUpReset));
        }

        private void InnerReset(int selectedIndex, List<Object> local, List<Object> group, int index, Action action)
        {
            if ((0 < local.Count())
                && (index > local.Count()))
            {
                index = index - local.Count();
            }

            if (selectedIndex < local.Count())
            {
                action(index, local);
            }
            else
            {
                action(index, group);
            }
        }

        private delegate void Action(int index, List<Object> items);

        private static void MoveUpReset(int index, List<Object> pages)
        {
            Object item = pages.ElementAt(index);
            pages.RemoveAt(index);
            pages.Insert(index - 1, item);
        }

        private static void MoveDownReset(int index, List<Object> pages)
        {
            Object item = pages.ElementAt(index);
            pages.RemoveAt(index);
            pages.Insert(index + 1, item);
        }
    }

注意这里为了说明性使用，所以跟源码不完全一样。

 public class LocalConfiguration
    {
        public static object[] Pages
        {
            get;
            set;
        }

        public static List<object> OverridePages
        {
            get;
            set;
        }
    }

    public class GroupConfiguration
    {
        public static object[] Pages
        {
            get;
            set;
        }

        public static List<object> OverridePages
        {
            get;
            set;
        }
    }