Best Ways To Refactoring An Existing, Messy Mvc

[KinoScope]

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

[xylex]

Have you read Fowler's Refactoring book? That provides a bit of a framework for tackling these types of issues in a safe and iterative manner.

[KinoScope]

Yes, it sits on my desk as we speak. It's the first and only book I brought here when I started 3 weeks ago.