Anonim250

So, as I understand you would still throw an exception, catch it on a highest level and show it nicely?

No, I think I don't want to use exceptions for non-fatal errors. Here is an example: User gets an activation email with an activation links. Clicks a link and activates his account. But he may click that link second time and we should show him an error that his account was already activated earlier. I don't think this is an exceptional situation, I think this is an expected situation that must be handled differently. How would you make such check? Maybe my option number 1 (see the starting post) or maybe you know a better way?

I've just made that name up Well, in this case - yes, maybe just throwing an exception and logging+showing it in the uncaught exception handler or at the highest level (a front or page controller) is a right way. But what would you do if an error was expected (so it would not an exceptional situation to throw an exception in), for example checking that the user was not already processed and displaying a nice-looking error if it was.

Well, it's something like this (I believe it's called a page controller, but I'm not sure, because I'm not a web programmer): <?php require_once 'bootstrap.php'; // init configuration, database connection, autoload, etc. function GetPageContent() { ... } $pageVars = array(); $pageVars['CONTENT'] = GetPageContent(); ... PageRenderer::RenderPage($pageVars); // fills main template and outputs it ?>

smoseley, 1) How is your code better than my option 2? 2) Why do you need to return boolean (or anything at all) from the checkUserAndKey() function if it either succeeds or throws an exception?

Hello, Here is my initial code: function GetPageContent() { if(!isset($_GET['userid']) || !isset($_GET['key'])) return 'Wrong parameters!'; $userID = $_GET['userid']; $user = UserAccessor::GetById($userID); if($user == null) return 'Wrong user id!'; if($_GET['key'] != GenerateKeyFromUser($user)) return 'Wrong key!'; // main action starts here I don't like that the function is bloated with such checks. It becomes harder to read and I think that these checks should be extracted to a separate function. So I think it should be something like this: function GetPageContent() { if(!ParametersAreValid()) return $error; // main action starts here 2 options pop up on my mind: 1. function GetPageContent() { list($parametersAreCorrect, $errorMsg) = CheckParameters(); if(!$parametersAreCorrect) return $errorMsg; // main action starts here 2. function GetPageContent() { try { CheckParameters(); } catch(Exception $ex) { return $ex->getMessage(); } // main action starts here But I don't like option 2 because I think that in this situation exception is used to control program flow. And even if we say that wrong parameters is an exceptional situation here, this code will not be generic, because in another situation wrong parameters could be expected and using exception would be inappropriate. What do you recommend? Maybe there is some pattern for such situations?

Thank you. ignace, thank you for your reply. Let me ask you some questions: 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'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? 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. 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? 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? I think I will move functions to the corresponding model methods. For example PrepareReplySubject() could be moved to MessageService::PrepareReplySubject().

Here is how I would do it (pseudo code): $imageFilename = sprintf('%s_%s.%s', $realImageName, $guid, $extension); SaveImage($tmpFileName, $imageFilename); // save to /images/myimage_GUID.jpg for example $sql = "INSERT INTO Item SET ..., item_image = {$db->Quote($imageFilename)}, ..."; $db->Execute($sql);

Here is how it is typically done (simplified): ... $message = "Nieuw onderzoek ".$id." is aangemaakt!"; FlashHelper::setFlash($message, FlashHelper::MESSAGE_TYPE_SUCCESS); header("Location: GroepAanmaken.php" ); exit; ... inside GroipAanmaken.php: $flash = FlashHelper::flash(); if($flash != null) echo $flash; FlashHelper.php: class FlashHelper { const MESSAGE_TYPE_DEFAULT = 1; const MESSAGE_TYPE_SUCCESS = 2; const MESSAGE_TYPE_ERROR = 3; public static function setFlash($message, $messageType = self::MESSAGE_TYPE_SUCCESS) { $classCode = ''; switch($messageType) { case self::MESSAGE_TYPE_DEFAULT: $classCode = ''; break; case self::MESSAGE_TYPE_SUCCESS: $classCode = 'class="success"'; break; case self::MESSAGE_TYPE_ERROR: $classCode = 'class="errorMessage"'; } $_SESSION['flash_message'] = sprintf('<p %s>%s</p>', $classCode, htmlspecialchars($message)); } public static function flash() { if(!isset($_SESSION['flash_message'])) return null; $message = $_SESSION['flash_message']; unset($_SESSION['flash_message']); return $message; } }

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?