HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Is this a good controller?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiscontrollergood

Problem

```
model = new Model();
}

public function invoke()
{
if (isset($_GET['page']) && $_GET['page'] == "registration")
{
if(isset($_POST['username']) && isset($_POST['password']))
{
// data send for registration
$user = $this->model->userRegistration($_POST['username'], $_POST['password'], $_POST['name'], $_POST['familyname'], $_POST['country'], $_POST['age'], $_POST['degree']);
if($user->IsRegistered){
include 'view/registered.php';
}
else
{
include 'view/registration.php';
}
}
else
{
include 'view/registration.php';
}
} // if end for registration page

elseif (isset($_GET['page']) && $_GET['page'] == "login")
{
if(isset($_POST['username']) && isset($_POST['password']))
{
// data send for login
$user = $this->model->userLogin($_POST['username'], $_POST['password']);
if($user->IsLogin){
$_SESSION["username"] = $user->Username;
include 'view/home.php';
}
else
{
include 'view/login.php';
}
}
else
{
include 'view/login.php';
}
} // elseif end for login page

elseif (isset($_GET['page']) && $_GET['page'] == "edit_registered")
{
if(isset($_REQUEST['eid']))
{
// data send for edit registration for a particular user
$user = $this->model->userEditRegistration($_POST['username'], $_POST['password'], $_POST['name'], $_POST['familyname'], $_POST['country'], $_POST['age'], $_POST['degree'], $_REQUEST['eid']);
if($user->IsRegistered){

Solution

If you want an honest opinion, I'd say no, this is not a good controller. I won't bother with code styles, but as far as logic goes:

  • Conditional includes? Not a good idea. You should be writing code that could be included anywhere if necessary (but possibly not used). Using includes like this can quickly turn your project into a complete mess, especially if the other files are written without the knowledge that they're being included in such a manner.



  • A separate elseif and logic for each URL parameter? Why? There are so many better ways to route users to the correct page. Search Google and you'll find at least a dozen. (Also, you check to make sure $_GET['page'] is set as many as 9 times...)



  • Why is this in a class? In it's current state, with only one function and one instance variable, it definitely doesn't need to be. (Although I might not be seeing everything.)

Context

StackExchange Code Review Q#25917, answer score: 9

Revisions (0)

No revisions yet.