Please help me understand and develop a good page controller

[Anonim250]

Hello,

I have some experience in developing desktop applications, but I'm new to PHP and web programming.

 

Currently the site I'm working on (some legacy project) has scripts like index.php, about.php, news.php, customer.php (customer's control panel), etc.

 

For example, when customer logs in he can do many different things in his control panel - add orders, view orders, read messages, reply to them, etc.

So in customer control panel there are links such as customer.php?messages, customer.php?messages&replymessageid=1234, customer.php?addorder, etc.

 

In customer.php file I have code something like this:

 

<?

require_once 'bootstrap.php'; // init configuration, prepare auto load, connect to db, etc.

if(isset($_GET['messages']))
$content = GetMessagesHtml();
else if(isset($_GET['addorder']))
$content = GetAddOrderHtml();
else if ...

function GetMessagesHtml()
{
$user = $_SESSION['User'];
if(isset($_POST['postreply']))
{
	return SaveReply($user);
}
else if(isset($_GET['replymessageid'])) // show reply message form
{
	return ShowReplyForm($user, $_GET['replymessageid']);
}
else // show all messages
{
	return ShowMessages($user->ID);
}
}

function SaveReply($user)
{
$formValidator = new FormValidator($_POST);
$formValidator->addRules(...);

if($formValidator->validate())
{
	$request = $formValidator->getCleanRequest()->data;

	$replyMessage = new Message($request['txtSubject'], ...);
	MessagesService::postMessage($replyMessage);

	FlashHelper::setFlash('Your reply has been successfully posted!', FlashHelper::MESSAGE_TYPE_SUCCESS);
	header("Location: {$_SERVER['PHP_SELF']}?messages");
	exit;
}
else
{
	// return reply form with errors
	return MessagesPresenter::getReplyMessageFormHtml($user, $formValidator->getCleanRequest(), $formValidator->getErrors());
}
}

function PrepareReplyBody($messageToReply)
{
return 'You wrote:<br>' . $messageToReply->Body;
}

function PrepareReplySubject($messageToReply)
{
  return 'Re: ' . $messageToReply->Subject;
}

function ShowReplyForm($user, $replymessageid)
{
$messageToReply = MessagesAccessor::getMessageByUserAndId($user, $replymessageid);

$request = array(
	'txtSubject' => PrepareReplySubject($messageToReply),
	'txtBody' => PrepareReplyBody($messageToReply));

return MessagesPresenter::getReplyMessageFormHtml($user, new CleanRequest($request)); // uses some template engine inside
}

function ShowMessages($userid)
{
$messages = MessagesAccessor::getMessages($userid);
return MessagesPresenter::getMessagesHtml($messages);
}

function GetAddOrderHtml()
{
  ...
}

SiteRenderer::showPage($content, ...); // just loads main page template, inserts generated content where necessary

?>

 

Could you please help me improve my code? As the project is relatively large and legacy, I can't just change everything, but I can try to refactor it one step at a time to make it better. What would you recommend? What are the biggest mistakes in this code?

[scootstah]

I didn't read all of it, but I saw this: header("Location: {$_SERVER['PHP_SELF']}?messages");

 

This is a possible XSS vulnerability. You can't trust user input like that.

[ignace]

Remarks:

 

[*] Many of those functions handle too many responsibilities. GetMessagesHtml() for example save's replies, display's a reply form, and returns messages. It also relies on globals $_GET and $_POST which means that the function may return something different than what was intended and no ability to enforce any specific output.

[*] Object specific behavior is performed by external functions. For example PrepareReplySubject() is used on a $message object to prepend a 'Re: ' string to it's Subject.. IMO this should be wrapped inside a Reply() method on a DAO or Repo that loads the message from the database and already adds Re: or Fwd: to the Subject.

[*] Controller/Presenter contains model behavior. For example getReplyMessageFormHtml() returns a Form with fields that map to a model/aggregate and duplicates validation (or calls $model->getvalidator())?

[*] Poor use of OO, classes are not more than namespaces for static methods.

[*] Namespacing functions would help to identify all functions that work on the same data. Which in turn helps in converting these to models.

[Anonim250]

I didn't read all of it, but I saw this: header("Location: {$_SERVER['PHP_SELF']}?messages");

 

This is a possible XSS vulnerability. You can't trust user input like that.

Thank you.

 

 

ignace, thank you for your reply. Let me ask you some questions:

 

Many of those functions handle too many responsibilities. GetMessagesHtml() for example save's replies, display's a reply form, and returns messages.

 

It doesn't save replies. It delegates the responsibility to save reply to another function. (otherwise we may say that the Main() function handle too many responsibilities). Isn't it a controller's job?

 

It also relies on globals $_GET and $_POST which means that the function may return something different than what was intended and no ability to enforce any specific output.

 

It's a top level controller function. Someone has to read $_GET & $_POST globals to decide what to do next. Isn't it a controller's job?

 

Object specific behavior is performed by external functions. For example PrepareReplySubject() is used on a $message object to prepend a 'Re: ' string to it's Subject.. IMO this should be wrapped inside a Reply() method on a DAO or Repo that loads the message from the database and already adds Re: or Fwd: to the Subject.

 

IMO it's not a data access layer's or repository's responsibility. It's a business logic and should be placed in business logic layer (model layer). I agree that it shouldn't be in controller class, but I disagree it should be in data access layer or in repository code.

 

Controller/Presenter contains model behavior. For example getReplyMessageFormHtml() returns a Form with fields that map to a model/aggregate and duplicates validation (or calls $model->getvalidator())?

 

getReplyMessageFormHtml() does not duplicate validation. There is no validation inside. Validation is initiated in the controller code (see $formValidator->addRules() call).

Could you please explain what is exactly wrong here?

 

Poor use of OO, classes are not more than namespaces for static methods.

 

Do we always need OO? What would become better in this code if I replaced static methods with instance method? Which benefits would I get and do I need them?

 

Namespacing functions would help to identify all functions that work on the same data. Which in turn helps in converting these to models.

 

I think I will move functions to the corresponding model methods. For example PrepareReplySubject() could be moved to MessageService::PrepareReplySubject().

[ignace]

Many of those functions handle too many responsibilities. GetMessagesHtml() for example save's replies, display's a reply form, and returns messages.

 

It doesn't save replies. It delegates the responsibility to save reply to another function. (otherwise we may say that the Main() function handle too many responsibilities). Isn't it a controller's job?

 

The main() function is completely unrelated to my response, since it's a C artifact used to bootstrap an application. I was referring to the fact that your functions are described vaguely. One would assume for example that calling GetMessagesHtml() ALWAYS returns messages in HTML format while the simple existence of a _GET or _POST variable could save a reply or even return a form instead of the expected formatted html messages. Hardly a side-effect free function. GetMessagesHtml() is a very poor name if it would have to represent a Controller in your application.

 

Object specific behavior is performed by external functions. For example PrepareReplySubject() is used on a $message object to prepend a 'Re: ' string to it's Subject.. IMO this should be wrapped inside a Reply() method on a DAO or Repo that loads the message from the database and already adds Re: or Fwd: to the Subject.

 

IMO it's not a data access layer's or repository's responsibility. It's a business logic and should be placed in business logic layer (model layer). I agree that it shouldn't be in controller class, but I disagree it should be in data access layer or in repository code.

 

Actually it's presentation logic... An actual e-mail has headers to indicate this was a reply to a previous message. So your model should have a isReply(true) function if you don't have Headers.

 

Controller/Presenter contains model behavior. For example getReplyMessageFormHtml() returns a Form with fields that map to a model/aggregate and duplicates validation (or calls $model->getvalidator())?

 

getReplyMessageFormHtml() does not duplicate validation. There is no validation inside. Validation is initiated in the controller code (see $formValidator->addRules() call).

Could you please explain what is exactly wrong here?

 

A form is a UI representation of a model/aggregate to let a user input data to store it in some data source. Your model validates data as it's entered through it's mutator functions, right? So any external object that validates your model data actually copies your model validation.. Instead I would have a method on my model like so:

 

public function getForm() {
    $form = new SomeModelForm();
    $form->setValidator($this); // validate by calling set*() methods
    $form->setData($this->_data); // used to edit the model
    return $form;
}

 

Poor use of OO, classes are not more than namespaces for static methods.

 

Do we always need OO? What would become better in this code if I replaced static methods with instance method? Which benefits would I get and do I need them?

 

OO is generally advised for bigger projects since they allow you to describe things which makes it easier/intuitive to work with them. Like Struct and Enum's in C. Take for example a message array versus a Message object.

 

$message->setSubject('..'); compare this to $message['subject'] = '..';

The $message array has to be validated first every time you want to work with the contents and the code is sensitive for typo errors. Imagine what would happen in case of schema changes.. You would have to do a find/replace on your entire project instead of just having to open the Message class file and change the field name. There are more advantages than these, but are beyond the scope of this post.

 

Namespacing functions would help to identify all functions that work on the same data. Which in turn helps in converting these to models.

 

I think I will move functions to the corresponding model methods. For example PrepareReplySubject() could be moved to MessageService::PrepareReplySubject().

 

MessageService is not the proper place to put this, as you mentioned before it's business logic so it does not belong in a service instead I would opt for either a Special Case ReplyMessage which extends Message and has a isReplyFor(Message $message) or a simple $message->isReply(true); afterwards you can easily check if it's a reply and add 'Re:' in your view.