1
\$\begingroup\$

I have ridden to much about MVC and SOLID, so I have a mess in my head. I have written a small MVC application and I need your experience and sober mind.

index.php - Start point of my app

<?php
require 'libs/Bootstrap.php';
$app = new Bootstrap();
$rover = new Index();
$rover->create('input.txt');

Bootstar.php - mini rules fo url

<?php
require 'libs/Model.php';
require 'libs/Controller.php';
require 'libs/View.php';
require 'controllers/Error.php';
require 'controllers/Index.php';

class Bootstrap {

  function __construct() {

    $url = isset($_GET['url']) ? $_GET['url'] : null;
    $url = rtrim($url, '/');
    $url = explode('/', $url);

    if(($url[0] !== 'index' && !empty($url[0])) || !empty($url[1])){
        $error = new Error();
        return false;
    }
  } 
}

Model

<?php

class Help_model {

function __construct() {

}

 function getSide($x, $y, $d, $instruction){    
    if($instruction != ''){
        $instruction_str = str_split(trim($instruction));
        $prev = '';
        $res = [];

        foreach($instruction_str as $cmd){  
            if($d == 'N'){
                if($cmd == 'M'){
                    $y += 1;
                    $prev = '';
                }
                else if($cmd == 'R'){
                    $d = 'E';
                    $prev = 'R';
                }
                else if($cmd == 'L'){
                    $d = 'W';
                    $prev = 'L';
                }
            }
            else if($d == 'E'){
                if($cmd == 'M'){
                    $x += 1;
                    $prev = '';
                }
                else if($cmd == 'R'){
                    $d = 'S';
                    $prev = 'R';
                }
                else if($cmd == 'L'){
                    $d = 'N';
                    $prev = 'L';
                }
            }
            else if($d == 'S'){
                if($cmd == 'M'){
                    $y -= 1;
                    $prev = '';
                }
                else if($cmd == 'R'){
                    $d = 'W';
                    $prev = 'R';
                }
                else if($cmd == 'L'){
                    $d = 'E';
                    $prev = 'L';
                }
            }
            else if($d == 'W'){
                if($cmd == 'M'){
                    $x -= 1;
                    $prev = '';
                }
                else if($cmd == 'R'){
                    $d = 'N';
                    $prev = 'R';
                }
                else if($cmd == 'L'){
                    $d = 'S';
                    $prev = 'L';
                }
            }
        }

        array_push($res, $x, $y, $d);
        return $res;
    }
 }

 function readFromFile($file){
    if(file_exists($file)){
        $input = file($file);
        $output = [];

        for($i = 1; $i < count($input); $i++){
            $start_info = explode(' ', $input[$i]);

            if($i % 2 != 0){
                $x = 0;
                $y = 0;
                $d = '';
                $position = $start_info;
                list($x, $y, $d) = $position;
                $d = trim($d);
            }
            else{
                $instruction = '';
                $instruction = $input[$i];
                array_push($output, $this->getSide($x, $y, $d, $instruction));
            }   
        }
        return $output;
      }
   }
}

Controller

<?php

require 'models/Help_model.php';

class Index extends Controller{

  function __construct() {
    parent::__construct();  
  }

  function create($media) {
    $model = new Help_model();      
    $file = $model->readFromFile($media);
    $this->view->render('index/index', array('file' => $file));
  }
}

View

<?php require 'views/header.php'; ?>
<?php 
$count = 0;
foreach ($array['file'] as $files) {
  foreach ($files as $file) {
    $count++;

    echo $file;
    if ($count % 3 == 0){
        echo '<br/>';
    }
  }
}
?>
<?php require 'views/footer.php'; ?>
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Please tell us what the purpose of your application is, and retitle the question accordingly. See How to Ask. \$\endgroup\$ – 200_success Apr 15 '15 at 5:47
  • \$\begingroup\$ It's MVC application that calculate moving algorytm. My question is: вoes my code clean? I need code review. \$\endgroup\$ – Kevin7 Apr 15 '15 at 10:46
1
\$\begingroup\$

Your model isn't really modelling (what in the world is a Help_model?). It would help a lot if it were actually a useful object. In fact, I had to struggle to figure out what the purpose of this code was, even after I explicitly asked you. The only hint was the variable name $rover, which was barely mentioned in the question.

Here is a proper model:

<?php

class Rover {
    function __construct($x=0, $y=0, $dir='N') {
        $this->x = $x;
        $this->y = $y;
        $this->dir = $dir;
    }

    function moveForward() {
        switch ($this->dir) {
                            case 'N': $this->y++; break;
          case 'W': $this->x--; break;          case 'E': $this->x++; break;
                            case 'S': $this->y--; break;
        }
    }

    function turnLeft() {
        switch ($this->dir) {
                            case 'N': $this->dir = 'W'; break;
          case 'W': $this->dir = 'S'; break;    case 'E': $this->dir = 'N'; break;
                            case 'S': $this->dir = 'E'; break;
        }
    }

    function turnRight() {
        switch ($this->dir) {
                            case 'N': $this->dir = 'E'; break;
          case 'W': $this->dir = 'N'; break;    case 'E': $this->dir = 'S'; break;
                            case 'S': $this->dir = 'W'; break;
        }
    }

    function doInstructions($instructions) {
        foreach (str_split($instructions) as $cmd) {
            switch ($cmd) {
                case 'M': $this->moveForward(); break;
                case 'L': $this->turnLeft();    break;
                case 'R': $this->turnRight();   break;
            }
        }
    }

    static function processFile($filename) {
        $rovers = array();
        $lines = file($filename);

        # Ignore first line.  Every subsequent pair of lines contains a
        # starting configuration and set of instructions.
        for ($i = 1; $i + 1 < count($lines); $i += 2) { 
            list($x, $y, $dir) = explode(' ', $lines[$i]); 
            $rovers[] = $rover = new Rover(intval($x), intval($y), trim($dir));
            $rover->doInstructions($lines[$i + 1]);
        }       
        return $rovers;             
    }             
}

Notable changes:

  • Each Rover object actually represents the state of a rover.
  • The processFile() function is static; you call it as Rover::processFile($filename). What you had before…

    function create($media) {
      $model = new Help_model();      
      $file = $model->readFromFile($media);
      $this->view->render('index/index', array('file' => $file));
    }
    

    … was senseless instantiation of an object just to call a function.

  • I've reorganized the if statements as case blocks, and split them up into one method per command. The code is easier to follow if you branch primarily based on the current instruction rather than the current orientation.
  • $prev is a pointless variable, which you assign to but never use.
  • for ($i = 1; $i < N; $i++) needs a comment due to the weirdness of starting at 1.
  • I got rid of a lot of senseless assignment of variables in readFromFile(), where you immediately overwrite the value afterwards.
  • Wrapping your entire functions in if ($instruction != '') and if (file_exists($file)) is a bad idea — it just defers crashes and makes bugs harder to trace. You also end up returning a value in some cases and not returning a value in other cases, which puts a burden on the caller to handle both cases.
  • There is no need to trim($instruction). Whitespace will be ignored as unrecognized commands anyway.

The Rover objects also make it possible to write a sane view that isn't trying to make sense of a linear array in chunks of three elements:

<?php
require 'views/header.php';
foreach ($array['result_states'] as $rover) {
    printf("%d %d %s<br/>\n", $rover->x, $rover->y, $rover->dir);
}
require 'views/footer.php';
?>
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.