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.
Select * from ..can be considered an anti-pattern. See stackoverflow.com/q/3639861/31326select *becomes a "best practice."