Do I make this neater? Or, any better way for this IF block.

[iarp]

I'm trying to clean up some past coding for a project of mine and this block keeps standing out like a sore thumb, but it works and i can understand it.

 

What it's doing is comparing each of the users submitted data to the admin-set data and then flagging if there any errors or not.

 

Any suggestions on a different approach?

 

foreach ($_SESSION['usersData'] as $k => $v) {
foreach ($this->info as $m => $n) {
	if ($m == $k) {
		if ( ($n['required'] && ( empty($v) || !isset($v) ) ) ) {
			$_SESSION['errors'][$k] = TRUE;
		} elseif ( (!$this->escape_data($v,$n['type'])) && ($n['required']) ){
			$_SESSION['errors'][$k] = true;
		} elseif ($n['required'] && ($v == $n['default'])) {
			if ($n['readonly'] != true)
				$_SESSION['errors'][$k] = true;
		} elseif ($n['required'] && $n['readonly'] && ($v != $n['default']) ) {		
			# Box is required, set to readonly and has a default value, if this triggers then the user has tampered with our form.
			$_SESSION['usersData'][$k] = $n['default'];
		} elseif (isset($_SESSION['errors'][$k])) {
			unset($_SESSION['errors'][$k]);# = false;
		}
	} elseif ( isset($this->info['file']) ) {
		if ($this->info['file']['required'] && $_FILES['file']['size'] == 0) {
			$_SESSION['errors']['file'] = TRUE;
		} else {
			unset($_SESSION['errors']['file']);
		}
	}
}
}

[scootstah]

Why do you need to put that information in sessions? Where did the user input come from?

 

It looks like this is in a class, which throws a red flag to me. It sounds like the class is trying to do too many things.

[iarp]

It's from a form that the user fills out, if the form fails, I've yet to find another way to pass the data back to the form so the user doesn't have to re-fill everything in, GET/POST isn't possible, cookies too unreliable.

 

I'm not sure how to respond about the class doing too much. Everything I've written so far has a flow through that requires access to all 10 functions I've written, I could attempt to separate some parts, but I'm not sure how that would help as I would still have to call all functions/classes, I figured one class would be easiest to manage.

[Mahngiel]

In 3 months will you know what $k, $m, $x, $y, $lmnop are? I wanted to read but your variable naming made it impossible.

[KevinM1]

Yeah, it's definitely not very readable.  $k and $v make sense, but $n and $m?  And using classes simply to group functions together is a waste of time.  True OOP is far more than merely using classes.

 

That said, what's stopping you from simply having the form post to itself?  That's generally how sticky forms (forms that remember field values) are made.

[iarp]

After reading your posts, it makes sense I clean up those bad variable names. Also, fixed my double redirect issue of, submit form -> process -> redirect back to form, now it posts right back to itself.

 

My only wonder is, if you think I shouldn't be running this within a class, what better way is there. I primarily did it in a class format just to the user could add $form->getForm(); as well to not have to deal with potential scoping or variable issues from other applications that might use the same variable I am.

 

The statement is still big, but i think it's easier to read.

 

foreach ($_POST['data'] as $liveField => $liveData) {
foreach ($this->info as $storedField => $storedData) {
	if ($storedField == $liveField) {
		if ( 
			( $storedData['required'] && ( empty($liveData) || !isset($liveData) ) ) ||
			( !$this->escape_data($liveData,$storedData['type']) ) ||
			( !$this->escape_data($liveData,$storedData['type']) && ($storedData['required']) )
			) {
			if ($storedData['required'] || (!$storedData['required'] && !empty($liveData)) )
				$this->errors[$liveField] = true;
		} elseif ($storedData['required'] && ($liveData == $storedData['default'])) {
			if ($storedData['readonly'] != true)
				$this->errors[$liveField] = true;
		} elseif ($storedData['required'] && $storedData['readonly'] && ($liveData != $storedData['default']) ) {
			$_POST['data'][$liveField] = $storedData['default']; # Box is required, set to readonly and has a default value, if this triggers then the user has tampered with our form.
		} elseif (isset($this->errors[$liveField])) {
			unset($this->errors[$liveField]);# = false;
		}
	} elseif ( isset($this->info['file']) ) {
		if ($this->info['file']['required'] && $_FILES['file']['size'] == 0)
			$this->errors['file'] = TRUE;
		else
			unset($this->errors['file']);
	}
}
}

 

Always learning new tricks.