PHP - Refactoring Question

Imagine 6 PHP classes (one each for a product line), that have very similar coding structures, that go like this:

//function that computes stuff inside each of 6 files:
//they vary slightly from file to file but essentially it is this:
function computeFunction
{
    $this->x = new X();

    $this->x->calcD();

    if ($this->x->dOk)
    {
        $this->x->calcE();
        $this->x->calcN();   
    }

    //more complicated logic that is essentially like above

    //and by the way!
    print $this->x->someVarThatIsUsedLater;
}

Then there is a single class like so :

class X
{
    function calcD()
    {
        //compute some condition

        if (<computed condition is met>) $this->dOk = true;
        else $this->dOk = false;

        //and by the way
        $this->someVarThatIsUsedLater = 4;
    }
}

Just to bring your attention to it, none of these functions return any result or value, but they nevertheless operate on variables of key interest via side-effects.  That is, they modify variables that essentially act like globals, and then use those variables later ($this->dOk and $this->someVarThatIsUsedLater are one more prominent examples).   I need to untangle this mess.  And make it clean and clear again, and make sense.  How do I best proceed?   I have been wrestling with some ideas... like $this->dOk, can within reason be turned into a return variable of calcD() function, and then be tested against like if ($this->x->calcD()) and I think it will be reasonable enough.  But then there are other functions that don't return anything and just act on variables via side-effects anyway so $this->dOk is one of the lesser troubles...   Other than that, what I am thinking of doing is getting rid of these mini-functions (calcE(),  calcN(), etc.), removing them as a funciton, and putting their body directly into the code, as a first step to refactor.  Many of the computations done inside are just a few lines of code anyway, and the functions kind of hide a lot of side-effects that happen, instead of actually encapsulating the behavior.  So while it may be counter-intuitive to dismantle the functions that appear to be doing something that normally can be encapsulated (computing key variables E, N, etc), I think dismantling them will actually clean things up as far as collecting all the side-effects inside a single parent function thereby making them more visible.   Caveat:  while doing so I will end up with 6 copies of untangled dismantled functions, because dismantling class X and putting its content into each of the 6 product line classes will have that effect.   But my hope is that from that point I will see more clearly to start identifying places where I can start to truly encapsulating the behavior via various structures, instead of masking it.   Problems / Questions: I would like to but I am not entirely sure that I can skip that step of dismantling functions & the 6x multiplying effect. It's probably the same like skipping steps in solving polynomial equations.  Some can do it and some need to list each step of their work. And I am not entirely sure what structures I can replace it with in the end after I dismantle the functions. It also looks like a lot of work.  Is there a better way? P.S. I already put tests on computeFunction() for each product line so I can be less paranoid about hacking stuff up.
Edited by dennis-fedco, 19 January 2015 - 03:06 PM.

Hi all

I have some code I'd like to re-factor

Code: [Select]
<?php

         $question_1   =  $form->getObject()->getQuestion1();
         $question_2   =  $form->getObject()->getQuestion2();
?>

<div class="admin">
    <div>
    <?php
                echo $form['question_1']->renderLabel();
                echo $question_1;
    ?>
    </div>
</div>

<div class="adminl">
    <div>
<?php
                echo $form['question_2']->renderLabel();
                $q2 = str_replace("-", '<br />',$question_2);
                echo $q2;
                ?>
   </div>
</div>

I'd preferably like some kind of for loop, that will just increment the _1, _2 values.

Can anyone offer some help?

Thanks

Hi

I have the following code:


    $image_1 = $value->getElementsByTagName("Image1");
    $image1  = $image_1->item(0)->nodeValue;

    $image_2 = $value->getElementsByTagName("Image2");
    $image2  = $image_2->item(0)->nodeValue;

    $image_3 = $value->getElementsByTagName("Image3");
    $image3  = $image_3->item(0)->nodeValue;

    $filename_1 = basename($image1);
    $ch = curl_init ($image1);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_1,'w');
    fwrite($fp, $rawdata);
    fclose($fp);

    $filename_2 = basename($image2);
    $ch = curl_init ($image2);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_2,'w');
    fwrite($fp, $rawdata);
    fclose($fp);

    $filename_3 = basename($image3);
    $ch = curl_init ($image3);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_3,'w');
    fwrite($fp, $rawdata);
    fclose($fp);


Would someone help me to refactor this, as I can seem needing $image_4, $image_5 at a later date.

Thank you

Hi

I have a method, which is pretty large.

It has a lot of 'duplicate' code and I'd like to refactor it:


   protected function doSave($con = null)
   {

        $choices_1 = $this->getValue('question_1');
        $choices_2 = $this->getValue('question_2');
        $choices_3 = $this->getValue('question_3');
        $choices_4 = $this->getValue('question_4');

        $serialized_ = "";
        $first = true;

            foreach($choices_1 as $choice_1)
            {
                if(!$first)
                {
                    $serialized_1 .= ",";

                }
                $first = false;
                $serialized_1 .= $choice_1;
            }

            foreach($choices_2 as $choice_2)
            {
                if(!$first)
                {
                    $serialized_2 .= ",";

                }
                $first = false;
                $serialized_2 .= $choice_2;
            }

            foreach($choices_3 as $choice_3)
            {
                if(!$first)
                {
                    $serialized_3 .= ",";

                }
                $first = false;
                $serialized_3 .= $choice_3;

            }

            foreach($choices_4 as $choice_4)
            {
                if(!$first)
                {
                    $serialized_4 .= ",";

                }
                $first = true;
                $serialized_4 .= $choice_4;

            }

         $this->setValue('question_1', $serialized_1);
         $this->setValue('question_2', $serialized_2);
         $this->setValue('question_3', $serialized_3);
         $this->setValue('question_4', $serialized_4);

         return parent::doSave($con);
   }


You can see most of the arrays have a '_1', '_2' after them, the same goes for the $serialized variables.

What I'd like is to maybe have some kind of loop, that will reduce my code down, and rather than me typing all the code above, possibly increment the '_1', '_2' part upto the 5 iterations.

Can anyone provide some help/code for me to carry this out?

Regards

I was working on this project earlier on in the year, I have not posted here much.  I would like to get it over with and start something else. I have other files which look like the excerpt of code from a file below. I personally think that my code lacks structure and could be organized in a better fashion. I lost most of my progress and am having to backtrack and restore code. Is there a way to re-write this code and make it more readable? Please and thank you!

 

<?php
include('header.php');
require_once('dbcon/dbcon.php');
//include('functions.php');

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
	// sanitize values before entering them into db, no bad seeds.
	$username = mysqli_real_escape_string($conn, $_POST['username']);
	$password = mysqli_real_escape_string($conn, $_POST['password']);
	$bio = mysqli_real_escape_string($conn, $_POST['bio']);
	$hashed_password = mysqli_real_escape_string($conn, password_hash($password, PASSWORD_DEFAULT));
	$email = mysqli_real_escape_string($conn, $_POST['email_address']);
	$confirmation_status = 0;

/*	function sanitizeValues($x, string $postString) {
		$x = mysqli_real_escape_string($conn, $_POST[$postString]);
	}*/

	$username_query = "SELECT * from profiles001 WHERE username='$username'";
	$result = mysqli_query($conn, $username_query);

	// if username exists do not continue...
	if (mysqli_num_rows($result) > 0) {
		header('Location: /soapbox/signup.php');
		// let user know that username is taken...
	} else {
		// file upload stuff...
		$file = $_FILES['file'];
		$fileName = $_FILES['file']['name'];
		$fileTmpName = $_FILES['file']['tmp_name'];
		$fileSize = $_FILES['file']['size'];
		$fileError = $_FILES['file']['error'];
		$fileType = $_FILES['file']['type'];
		$fileExt = explode('.', $fileName);
		$fileActualExt = strtolower(end($fileExt));

		$allowed = array('jpg', 'jpeg', 'png');

		// avatar file constraints checks...
		if (in_array($fileActualExt, $allowed)) {
			if ($fileError === 0) {
				if ($fileSize < 1000000) {
					$fileNameNew = uniqid($_SESSION['username'], true) . "." . $fileActualExt;
					$fileDestination = 'uploads/' . $fileNameNew;
					move_uploaded_file($fileTmpName, $fileDestination);

				} else {
					echo "Your file is too big!";
				}
			} else {
				echo "There was an error uploading your file" . $fileError . $fileSize;
			}
		} else if (!(empty(in_array($fileActualExt, $allowed))) && !($allowed)) {
			echo "Cannot upload file of this type!";
		}

		mkdir("channel/" . $username);
		mkdir("channel/" . $username . "/videos");
		fopen("channel/" . $username . "/index.php", "w");

		$account_open_date = date("Y-m-d h:i:s");
		$current_date = date("Y-m-d h:i:s");
		//$account_open_date_retrieval_sql_select = "SELECT account_open_date from profile0";
		//$account_age = date_diff($row, $current_date); // acct open date - current date = account age
		//$account_age_result = mysqli_query($conn, $account_open_date_retrieval_sql_select);
		//$row = mysqli_fetch_assoc($account_age_result);

		// if-then-else-if statement to get rid of the fileDestination var undefined error when avatar photo is not submitted....
		if (!(empty($fileDestination))) {
			$sqlinsert = "INSERT INTO profiles001 (username, password, email, c_status, doc, avatar, bio) VALUES ('$username', '$hashed_password', '$email', '$confirmation_status', '$account_open_date', '$fileDestination', '$bio')";
		} else if (empty($fileDestination)) {
			$fileDestination = "assets/soap.jpg";
			$sqlinsert = "INSERT INTO profiles001 (username, password, email, c_status, doc, avatar, bio) VALUES ('$username', '$hashed_password', '$email', '$confirmation_status', '$account_open_date', '$fileDestination', '$bio')";
		}

		$result = mysqli_query($conn, $sqlinsert);
	}
}
?>