harriyott.com

Thursday, July 06, 2006

A new refactoring please

I use the built-in refactoring in Visual Studio for my C# code. I often use "Extract Method" to cut out a chunk of code from the middle of a growing method. This creates a new method under the original, moves the selected code into the new method, and replaces it with a call to the new method, working out what the parameters should be.

So, this contrived example:

        public void HaveAGoIfYouThinkYoureHardEnough(Person queueJumper)

        {

            bool hardEnough;

            int currentHardness = 0;

            if (queueJumper.Height >= 6)

            {

                currentHardness += 2;

            }

            if (queueJumper.Weight >= 14)

            {

                currentHardness += 3;

            }

            if (queueJumper.Weight >= 18)

            {

                currentHardness += 2;

            }

            if (queueJumper.Weight <= 8)

            {

                currentHardness -= 4;

            }

            if (queueJumper.ScarCount > 2)

            {

                currentHardness += 1;

            }

            if (currentHardness > 5)

            {

                hardEnough = true;

            }

            else

            {

                hardEnough = false;

            }

            if (hardEnough)

            {

                LetHimHaveAGo(queueJumper);

            }

        }



becomes

        public void HaveAGoIfYouThinkYoureHardEnough(Person queueJumper)

        {

            bool hardEnough;

            int currentHardness = 0;

            currentHardness = AddHeightHardness(queueJumper, currentHardness);

            currentHardness = AddWeightHardness(queueJumper, currentHardness);

            currentHardness = AddScarHardness(queueJumper, currentHardness);

            hardEnough = IsHardEnough(currentHardness);

            if (hardEnough)

            {

                LetHimHaveAGo(queueJumper);

            }

        }

 

        private static bool IsHardEnough(int currentHardness)

        {

            bool hardEnough;

            if (currentHardness > 5)

            {

                hardEnough = true;

            }

            else

            {

                hardEnough = false;

            }

            return hardEnough;

        }

 

        private static int AddScarHardness(Person queueJumper, int currentHardness)

        {

            if (queueJumper.ScarCount > 2)

            {

                currentHardness += 1;

            }

            return currentHardness;

        }

 

        private static int AddWeightHardness(Person queueJumper, int currentHardness)

        {

            if (queueJumper.Weight >= 14)

            {

                currentHardness += 3;

            }

            if (queueJumper.Weight >= 18)

            {

                currentHardness += 2;

            }

            if (queueJumper.Weight <= 8)

            {

                currentHardness -= 4;

            }

            return currentHardness;

        }

 

        private static int AddHeightHardness(Person queueJumper, int currentHardness)

        {

            if (queueJumper.Height >= 6)

            {

                currentHardness += 2;

            }

            return currentHardness;

        }



So I've added four more methods to make the first method smaller. Mostly, this type of new method will not be used by any other method, as I extract them for readability, not functionality.

If this main method (and it's four extracted methods) was called from an even mainer method, which did something else, then the two main methods (which originally were adjacent) become separated:

        public void GetIntoNightClub()

        {

            HaveAGoIfYouThinkYoureHardEnough(new Person("John Prescott"));

            WillTheBouncerLetMeIn();

        }



After refactoring, HaveAGoIfYouThinkYoureHardEnough and WillTheBouncerLetMeIn now have four methods between them.

I generally don't want to see these four methods again, and as I've made the method names self-commenting, I won't need to refer to them unless there's a bug. As there's only four static methods, and they're related to the existing method, creating a new class for them is probably not the right way to go.

Therefore, I propose two new refactorings. The first, and easiest, would be to add a region under the main method, which contains the extracted methods:

        #region Methods extracted from HaveAGoIfYouThinkYoureHardEnough

        // ...

        #endregion



Let's call this "Extract method to region". The region can be collapsed, and both main methods are visible at once.

The second refactoring would be to shuffle the methods off to another file, using the cunning partial class feature of C#. Let's call this "Extract method to new file". The file will be called <ClassName>_HaveAGoIfYouThinkYoureHardEnough.cs, so it's easy to see in the solution explorer what the file contains. Obviously the "Rename" refactoring would have some extra work to do here.

So, Visual Studio refactor team, please get to work...

2 Comments:

Anonymous said...

My dear sharriyott...

these posts of yours are well crafted, informative and at times downright pithy..

the slight flaw I detect is that you seem to find delight in the wrong kind of phenomenon. Consider Michaelangelo carving "David".. He creates a work of art whilst I fancy you would be interested most in the chisel he was holding or the colour of his shoes.

Please find the art in software development and concentrate less on the mechanics..

July 07, 2006 10:23 PM  
Simon said...

Thankyou for your comment - possibly the best one I've ever had. Thankyou for the compliments too. I'm glad you think my posts are well crafted, informative and pithy - thankyou.

You're absolutely right of course; I do focus on the chisel. I think this is because I use chisels everyday (to use your analogy). I also use them differently to old Mike, I use them more like Jesus did; to create something useful, rather than artistic. I see my role as a software developer as a craftsman, rather than an artist.

I'm not saying that insurance software (which is what I write for my day job) can't be beautiful, but the main focus is usefulness, and that is what I mainly focus on.

For me and my blog, there are three types of software. The software I write, the software I use to write it with, and everything else. I tend to write about the first two types, and not so much about the third.

I'm not allowed to blog about the insurance software I'm writing, as my boss has asked me not to. I am allowed to write about the development process and small areas of code that don't give away what I'm working on. This kind of narrows down the first category.

It is interesting that you commented on this post, in that it's about refactoring. I love refactoring, and I do find it quite artistic (in a craftsmanly kind of way). I like the way the code gets a lot neater, and removes the need for so many comments.

Anyway, I'm in danger of rambling, so I'll stop.

Despite sounding all defensive, I do take your point, and if you have some topics you'd like me to write about, leave a comment, and I'll have a go. Why not leave a couple of titles, and I'll see what I can come up with.

Anyway, thanks again, whoever you are...

July 08, 2006 12:58 AM  

Post a Comment

Links to this post:

Create a Link

<< Home