Search the Community

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: 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!) 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. 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. 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?

I am refactoring a class that has multiple responsibilities. After reading Single_responsibility_principle, I am still not sure, how to factor out the class into its aspects. The methods of the class are (summarized): class constructor Calculate {General, A, B} Convert {General, A, B} Load {General, C, D} Save {General, C, D} By General I mean i.e. load() function that can load A or B or both depending on the circumstances, and by A or B I mean more specific loadA(), or loadB(). My specific questions in regards to SRP is: Do `load()` and `save()` belong to different classes, because they can in theory change at different times for different reasons, or can/should I put them into the same class, because loading/saving can be considered as "database operations" and if one changes so *may* the other, i.e. adding a new attribute to whatever is being saved/loaded? How do I organize the classes anyway? i.e. something like this? class CalculateGeneral() {}; class CalculateA() extends CalculateGeneral {}; class CalculateB() extends CalculateGeneral {}; class ConvertGeneral() {}; class ConvertA() extends ConvertGeneral {}; class ConvertB() extends ConvertGeneral {}; class LoadSaveGeneral() {}; class LoadC() extends LoadSaveGeneral{}; class LoadD() extends LoadSaveGeneral{}; Woah that's 9 classes out of 1 that it is now..

Hey, I'm a n00b on this forum, and this is my first thread/post. Hello to everyone! I just started a new position as a Senior PHP Developer to look at and improve the company's MVC application. It's a bit of a doozy, especially considering their MVC is built in-house. The controller methods, at least the key ones, are fat, fat, fat. A contract controller's edit method deals with not only prepping the page for editing, but also takes care of the saving once the form is submitted. At approximately 500 lines, it has if/for/while nesting issues, bad variable name conventions, no helpful comments. Very little of the saving logic is delegated to the models, as it should. To add to my troubles, the flow of the application itself is highly disjointed, business logic toggles I am missing are executed deep within nested if's/whiles/fors, and chews up my days quickly trying to lay breadcrumbs and track how/where my required values will be triggered. And once I figure out the ONE toggle I was missing, I get roadblocked by another one, found elsewhere, in another controller. So now the laying of breadcrumbs begins again. Frustrating. On a final note, the views themselves have a fair bit of logic, as well as deep nesting issues within them. SO I'd say they abide by MVC best practices 50% of the time. But man, that other 50%... I am about to have my first meeting with my manager to discuss the things I've seen, and managed to do, as well as have him lay out my initial tasks for improvements. My gut tells me we should start from scratch, but I doubt that will sell well. Another option would be to start fresh, in parallel with the current system. Here's a bit of logic I found in one VIEW... $i = 1; if ((is_array($contentForLayout[0]['x'])) & (!empty($contentForLayout[0]['x']))) { foreach ($contentForLayout[0]['form_config_archives'] as $EventForm) { if ($EventForm['order_id'] > 0) { echo '<tr>'; echo '<td><strong>Form # ' . $i . '</strong></td>'; $FormOpenDate = @strftime($DayTypeCall, strtotime($EventForm['form_open_date'])); if ($FormOpenDate == "December 31, 1969") { echo '<td class="red"><em>No Date Entered</em></td>'; } else { echo '<td>' . $FormOpenDate . '</td>'; } $FormCloseDate = @strftime($DayTypeCall, strtotime($EventForm['form_event_date'])); if ($FormCloseDate == "December 31, 1969") { echo '<td class="red"><em>No Date Entered</em></td>'; } else { echo '<td>' . $FormCloseDate . '</td>'; } $FormEventDate = @strftime($DayTypeCall, strtotime($EventForm['form_close_date'])); if ($FormEventDate == "December 31, 1969") { echo '<td class="red"><em>No Date Entered</em></td>'; } else { echo '<td>' . $FormEventDate . '</td>'; } if ($AllowOrderEdit == 1) { $EditOrderLink = '<a class="thickbox" href="/order/edit/' . $EventForm['order_id'] . '/1?width=1200&height=500" title="::Edit Form Order" style="text-decoration:underline;color:#0000FF">' . $EventForm['order_id'] . '</a>'; } echo '<td>'; echo $EditOrderLink; echo '</td>'; echo '<td>'; echo $EventForm['server']; echo '</td>'; echo '<td>'; echo $EventForm['peer_template']; echo '</td>'; $TotalPercent = $EventForm['donations_check'] + $EventForm['date_check'] + $EventForm['fields_check'] + $EventForm['basefee_check']; if ($TotalPercent < 50) { echo '<td style="color:#FF0000;font-weight:bold;">'; } else if (($TotalPercent > 51) && ($TotalPercent < 76)) { echo '<td style="color:#FF9900;font-weight:bold;">'; } else if (($TotalPercent > 76) && ($TotalPercent < 100)) { echo '<td style="color:#EE9900;font-weight:bold;">'; } else { echo '<td style="color:#009900;font-weight:bold;">'; } echo $TotalPercent . '%'; echo '</td>'; echo '<td>'; if ($EventForm['event_status_id'] == 0) { $TheFormStatus = '<span style="color:#FF0000;"><strong>Archive</strong></span>'; } else if ($EventForm['event_status_id'] == 1) { $TheFormStatus = '<span style="color:#009900;"><strong>Active</strong></span>'; } else if ($EventForm['event_status_id'] == 2) { $TheFormStatus = '<span style="color:#FF9900;"><strong>Pending/Test</strong></span>'; } else if ($EventForm['event_status_id'] == 3) { $TheFormStatus = '<span style="color:#B3B3A7;"><strong>Closed</strong></span>'; } echo $TheFormStatus; echo '</td>'; if ($AllowOrderEdit == 1) { $EditOrderForFormLink = '<a href="/event/forms/' . $EventForm['id'] . '/' . $EventForm['order_id'] . '/' . $contentForLayout[0]['order'][0]['contract_id'] . '/' . $contentForLayout[0]['order'][0]['event_id'] . '" title="::Edit Form Configuration"><img src="' . Utilities::getRelativePath() . 'images/icons/16/icon_quick_options_pub_16.png" /></a>'; } echo '<td>'; echo $EditOrderForFormLink . ' '; if (is_dir(ABSOLUTEROOTPATH . '/staging/' . $EventForm['order_id'])) { echo '<a title="::Staging Form" href="' . STAGINGPEER1URL . $EventForm['order_id'] . '/" target="_blank"><img src="' . Utilities::getRelativePath() . 'images/icons/16/icon_quick_testform_pub_16.png" /></a> '; } if (is_dir(ABSOLUTEROOTPATH . '/events/' . $contentForLayout[0]['subdir'] . '')) { echo '<a title="::Live Form" href="' . str_replace('events/', '', LIVEPEER1URL) . 'site/entryform/' . $contentForLayout[0]['subdir'] . '" target="_blank"><img src="' . Utilities::getRelativePath() . 'images/icons/16/icon_quick_liveform_pub_16.png" /></a> '; } if (file_exists(ABSOLUTEROOTPATH . '/events/' . $contentForLayout[0]['subdir'] . '/staff.php')) { echo '<a class="thickbox" title="::Staff Page" href="' . LIVEPEER1URL . $contentForLayout[0]['subdir'] . '/staff.php?TB_iframe=true&height=500&width=800"><img src="' . Utilities::getRelativePath() . 'images/icons/16/icon_quick_staffpage_pub_16.png" /></a>'; } echo '</td>'; echo '</tr>'; $i++; } else { echo '<td colspan="10"><br /><p>No archived forms available for this event.</p></td>'; } } } else { echo '<td colspan="10"><br /><p>No archived forms available for this event.</p></td>'; } At any rate, I figured asking for strategy advice on here may help me. Or at least get a bit of sympathy