12

Example

I came across monolithic code that does "everything" in one place - loading data from database, showing HTML markup, acting as a router/controller/action. I began applying SRP moving database code into its own file, providing better naming for things, and it all looked good, but then I began having doubts as to why I'm doing this.

Why refactor? What is the purpose? Is it useless? What is the benefit? Note that I mostly left the monolithic file as is, but refactored only the smaller portion that was relevant to the area where I needed to do some work.

Original code:

To give a concrete example, I came across this code snippet - it loads product specifications either by a known product id, or by a user-selected version id:

if ($verid)
    $sql1 = "SELECT * FROM product_spec WHERE id = " . clean_input($verid);
else
    $sql1 = "SELECT * FROM product_spec WHERE product_id = " . clean_input($productid) ;
$result1 = query($sql1);
$row1 = fetch_array($result1);
/* html markup follows */

Refactoring:

Since I'm doing some work requiring me to change things in this specific part of code, I changed it to use repository pattern and upgraded it to use object-oriented MySQL facilities:

//some implementation details omitted 
$this->repository = new SpecRepository($mysql);
if ($verid)
    $row1 = $this->repository->getSpecByVersion($verid);
else 
    $row1 = $this->repository->getSpecByProductId($productid);
/* html markup follows to be refactored or left alone till another time*/

//added new class:
class SpecRepository extends MySqlRepository
{

    function getSpecByVersion(int $verid)
    {
        return $this->getMySql()->paramQuery("
            SELECT * FROM product_spec WHERE id = ?
        ", $verid)->getSingleArray();
    }

    function getSpecByProductId(int $productid)
    {
        return $this->getMySql()->paramQuery("
            SELECT * FROM product_spec WHERE product_id = ?
        ", $productid)->getSingleArray();
    }
}

Should I do this?

Looking back at the changes, the code is still there, code with same functionality, but in different files, different names, places, using more object-oriented style rather than procedural. Actually it's funny to note that the refactored code looks a lot more bloated despite having the very same functionality.

I foresee some answers saying "if you don't know the reasons why you refactor, don't do it", and perhaps I might agree. My reasons is to improve quality of code over time (and my hope is that I will do so by following SRP and other principles).

Are those good enough reasons or am I wasting my time on "rearranging code around" this way? Overall refactoring this feels a bit like treading water to be honest - it takes time and it becomes more "separated" as far as SRP goes but despite my good intentions I do not feel like I am making amazing improvements. Hence, debating if it is best to leave code as before and not refactor.

Why did I refactor in the first place?

In my case I am adding new functionality for a new product line, so I have to either follow existing code structure for similar product lines, or write my own.

  • OT but something to consider is that Select * from .. can be considered an anti-pattern. See stackoverflow.com/q/3639861/31326 – Peter M Dec 8 '17 at 17:50
  • 1
    @PeterM: Unless you're writing an ORM, in which case select * becomes a "best practice." – Robert Harvey Dec 8 '17 at 19:32
  • @RobertHarvey In which case I am questioning why you are writing your own ORM :D – Peter M Dec 8 '17 at 20:48
  • I have done refactors for more irrelevant causes. Cutting down the technical debt is good as soon as the benefits outweigth the costs. Looks like this is your case. The question is: Do you have the right tests for you to ensure that the code still works as expected? – Laiv Dec 8 '17 at 22:32
  • 1
    Reliability should be the predominant metric when refactoring IMO, not maintainability which relates to changeability, since reliability reduces the reasons for things to change in the first place. It improves stability, and code that is perfectly stable and reliable and fulfills all it needs to do incurs little maintenance cost, since it has very few reasons to ever have to change. Seek to reduce the reasons for things to change instead of trying to make them as easy as possible to change. Seeking the former will also tend to give some of the latter. – user204677 Dec 12 '17 at 21:57
13

In the concrete example you gave, refactoring might not have been necessary, at least not yet. But you saw that there was a potential for messier code in the future which would have led to a longer and more tedious refactoring, so you took the initiative and cleaned things up now.

I would also say the advantage you gained here was code that is easier to understand, at least from my point of view, as someone who doesn't know your code base.


In general:

Does the refactoring make it easier for other developers to understand your code?

Does the refactoring make it easier to enhance the code?

Does the refactoring make it easier to maintain the code?

Does the refactoring make it easier to debug?

If the answer to any of these was "yes", then it was worth it, and it was more than just "moving code around?

If the answer to all of these was "no", then maybe it didn't need to be refactored.

The final size of the code base might be larger in terms of LoC (although some refactorings can shrink the code base by streamlining redundant code), but that shouldn't be a factor if there are other gains.

9

Refactoring does bloat if you are disassembling a monolith.

However, you already found the benefit.

"In my case I am adding new functionality for a new product line."

You can't add functionality to a monolith very easily without breaking things.

You said that the same code is getting the data and doing the markup. Great, until you want to change which columns that you are selecting and manipulating to show in certain conditions. Suddenly, your markup code stops working in a certain case and you have to refactor.

  • yes, but I can also keep adding to the monolith even for new functionality :) i.e. I would add a new if/else block and follow the original style of code to add SQL statements, and then I would put new HTML markup in the same monolith file. And to change the columns I would find the if/else block responsible for housing SQL lines, and edit that SQL to make change to the columns, without breaking that file up. – Dennis Dec 8 '17 at 16:04
  • With refactored approach, I would likely first go to the if/else block in the monolith to see that I need to go to the separate repository file to change SQL. Or if I worked on that monolith recently I'd know to go right into the repository file instead, bypassing the monolith. Or if I searched my codebase for the specific SQL the search would put me into the new repository file also bypassing the monolith – Dennis Dec 8 '17 at 16:04
2

I consider refactoring if I know that I will have to work months or even years with that project. If I have to do a change here and there, than I resist myself the urge to move code around.

The preferred way of doing refactoring is when I have a base code with some sort of automated tests. Even so I have to be very careful with what I change to make sure the things are working like before. You will lose trust very soon if you deliver bugs in other areas than the one you supposed to work.

I value refactoring for the following reasons:

  • I understand the code better
  • I remove duplicated code so if I have a bug in one place I don't have to hunt every where were the code had been copy pasted
  • I create classes that have their well defined role. For example I can reuse the same repository to hydrate a HTML page, a XML feed or a background job. This wouldn't have been possible if the database access code lives only in the HTML class
  • I rename variables, methods, classes, modules, folders if they don't match with the current reality; I comment the code via variable names, method names
  • I solve complex bugs with a combination between unit tests and refactoring
  • I have the flexibility to replace entire modules, packages. If the current ORM is outdated, buggy or un-maintained, then is easier to replace it if it is not spread all over the project.
  • I discover new capabilities or improvements that result in proposals for the customer.
  • I created reusable components. This was one of the greatest benefits because the work done in that project could have been re-used for another client/project.
  • I often start with the dumbest, hardcoded, wacky solution and I refactor it later once I learn more. This later moment might be today or maybe after several days. I don't want to spend too much time to architect the solution. This will come while writing - rewriting the code back and forth.

In a perfect world refactorings should be verified by unit tests, integration tests so on. If there are none, try add them. Also some IDE might help a lot. I usually use a plugin that costs money. I want to spend little time with refactorings and be very efficient about it.

I also introduced bugs. I can see why some QA are asking are you guys doing refactoring? because everything stopped working! This is the risk the team has to embrace and we always have to be transparent about it.

During the years I found that continuous refactoring improved my productivity. Was much easier to add new features. Also big rebuilds didn't require a whole rewrite as often happened when the code base wasn't adapted to the product's evolution. Also it's fun.

  • This is very close to a down vote - There is no 'I' in 'Computer programmer'. By 'I' do you me 'I' (down vote) or do you mean the developers working on the code now and in the future? – mattnz Dec 12 '17 at 20:08
  • I noticed it me too, but I put it on wording. Also I worked as solo developer for many years and when I wrote this answer I reiterated mostly from that experience. I could replace I with to though. – Adrian Iftode Dec 13 '17 at 6:19
1

I think the question you have to ask yourself is not so much "Is there a benefit?" but "Who will pay for this work?"

We can all argue about the perceived benefits until the end of days, but unless you have a customer that believes in those benefits and values them enough to pay for the changes you shouldn't be doing them.

It's all too easy when you are on the a dev team somewhere to see code and want to refactor it to make it 'better'. But putting a value on 'better code' when the product already works is really hard.

Things like 'faster development of new features' don't cut it because they are just reduced costs. You need to generate sales.

1

To my opinion refactoring is a good investment.

Otherwise you will soon get technical debts (unsolved problems, bad code which is only working under certain conditions, etc.). The technical debts will soon get larger and larger, until the software is no longer maintainable.

But to make refactoring work, you also have to invest in tests. You should create automated tests, ideally you should have unit tests and integration tests. This prvenets you from breaking existing code.

If you have to convince your boss or colleagues, you should read some books about agile methods (e.g. SCRUM) and test driven development.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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