how to best refactor and phpunit test this code

[dennis-fedco]

I am refactoring legacy code.  I have a function called "load" that inside had a messy function called "loadProduct".  I refactored "loadProduct" to look nice and clean (with some exception code put in place, and now code looks something like this:

Design
{
    function load()
    {
        loadProduct();
        ...  //lots of other stuff, legacy code, etc. etc.
    }
    
    function loadProduct()
    {
        try
        {
            //these two line below replaced what used to be about 30 lines of legacy code with SQL and DB specific code
            //those lines are now inside Product()->loadSpecs() method
            $this->product = new Product();
            $this->product->spec = $this->product->loadSpecs($this->a, $this->b, $this->input->c);
        }
        catch (\BadFunctionCallException $e)
        {
            $this->error = true;
            $this->emsg = $e->getMessage();
        }
    }
}  

Now, here is my "big" problem and a list of questions that I do not know how to figure out best:

1. Before I refactored, I wrote some fairly extensive Unit Tests for loadProduct() function.  My tests verify various paths, like when the product is Loaded, when it is not loaded, and ALSO they verify that parameters inside that catch block above are set accordingly - that is they exercise the entire loadProduct() function as-is.  But here is what I want to do next:  I want to take the code inside of function called "loadProduct()", and move it into function "load".  Note that existing code (indicated by dots ...) inside the "load" function will stay in place and hence be after the code I will move there.  Then, since "loadProduct" function will be empty, I can remove it entirely.  Of course this will mean that:
   • My tests will no longer run (I removed the function tests were testing!)
2. Well, that is a bummer!   What I can do perhaps is I can modify my tests to instead of testing the old function code ["loadProduct()"], to test the the new class ["Product()"]and its method ["loadSpecs()"] that I have created.  Note that if I do so, I will no longer be able to test the parameters being set outside of the Product class.
   • So even after doing work to repurpose my tests to the Product() class, I will lose parts of my tests that verified things like parameters inside the catch block above, because those did not make it into the new Product() class.
3. So what to do?  Do I just leave the code as is as I printed above and "be happy" with that my tests work and that my loadMotor() function is being thoroughly tested?  The reason that I wanted to move the loading code out of "loadProduct()" in the first place is/was a part of my refactoring effort.  You see ... loadProduct() had SQL code inside of it, and loadProduct() was part of a bigger class [called Design()] that does NOT deal with SQL or DB-functionality.  So I moved the DB-SQL-functionality into a DAO pattern class which I called Product().  So I figured if I can move DB-loading part into a separate class, maybe I can remove the function that did the loading as I now have a class I can call instead of it as part of the more general "load" function.  I may be wrong.
4. I can also do what's described in #3 - leave code as-is, but now repurpose my tests to tests "load()" function instead.  This means repurposing tests to add whatever else load() function does.  I don't think it is a good solution as load() does a lot of stuff (as indicated by dots ...) in my original code above.  There is all kinds of legacy code in there that I did not yet wrap my mind about, nor wrote tests fo.....

So I come to you, the resident forum folks, to maybe guide me with your wisdom    Much thanks for reading all this of course!

What path, if any I shall take in my legacy code refactoring efforts? 

Edited April 2, 2014 by dennis-fedco