2
\$\begingroup\$

A couple of weeks ago a company sent me this coding challenge:

Please write a PHP web application and send it back to me as zip file:

  • Which waits for a user action, like clicking buttons. According to these actions some data (see further below) should be:
    • either shown nicely formatted on the screen
    • or downloaded as CSV file
  • You can either download the data on each request during the runtime of your PHP program or load the data from a database (in this case do NOT provide a DB dump, but a script which automatically transfers the data from the remote location to the DB)
  • Preferably the implementation should be written in "clean code", separate concerns using pattern like MVC, be object oriented, very good testable, best even already contain Unit tests and maybe even follow the KISS and SOLID principles

AND

  • Country list
  • The data should be a list of countries with their country code
  • Please download the base data from here
  • Afterwards you will have to change the whole list from "Country code - Country name" to "CountryName - CountryCode" and sorts the list by CountryName

Is my solution okay? How can it be improved?

Controller:

<?php

defined('BASEPATH') OR exit('No direct script access allowed');

class Welcome extends CI_Controller {

    function __construct() {
        parent::__construct();
        $this->load->model('country_model');
    }

    public function index() {
        $data = array();
        if (isset($_POST['run'])) {
            $this->_save_data();
            $data['list'] = $this->country_model->get_countries(252)->result();
        }
        $this->load->helper('form');
        $this->load->view('welcome_message', $data);
    }

    private function _save_data() {
        // Get a file into an array.  In this example we'll go through HTTP to get
        // the HTML source of a URL.
        $lines = file('http://pastebin.com/raw.php?i=943PQQ0n');
        $lineNo = 0;
        $startLine = 4;
        $endLine = 255;
        // Loop through our array, show HTML source as HTML source; and line numbers too.
        foreach ($lines as $line_num => $line) {
            $lineNo++;
            if ($lineNo >= $startLine) {
                $country = explode('   ', trim(htmlspecialchars($line)));
                if (!isset($country[0]) || !isset($country[1])) {
                    continue;
                }
                $count = $this->country_model->count_by_code($country[0]);
                if (!$count) {
                    $this->country_model->insert_entry($country[0], $country[1]);
                }
            }
            if ($lineNo == $endLine) {
                break;
            }
        }
    }

}

Model:

<?php

defined('BASEPATH') OR exit('No direct script access allowed');

class Country_model extends CI_Model {

    public $table = 'country';

    public function __construct() {
        // Call the CI_Model constructor
        parent::__construct();
        $this->load->database();
    }

    public function insert_entry($country_code, $country_name) {
        $data['country_code'] = $country_code;
        $data['country_name'] = $country_name;

        $this->db->insert($this->table, $data);
    }

    function count_by_code($country_code) {
        $this->db->where('country_code', $country_code);
        return $this->db->count_all_results($this->table);
    }

    function get_countries($limit) {
        $this->db->select('country_code, country_name');
        $this->db->order_by("country_name", "asc");
        return $this->db->get($this->table, 0, $limit);
    }

}

View:

<?php
defined('BASEPATH') OR exit('No direct script access allowed');
?><!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8">
        <title>Welcome to My Task</title>
    </head>
    <body>

        <div id="container">
            <h1>Welcome to My Task!</h1>

            <div id="body">
                <?php echo form_open(); ?>
                <input type="submit" value="Get Country List" name="run"/>
                <?php echo form_close(); ?>
                <br>
                <?php
                if (isset($list)) {
                    foreach ($list as $value) {
                        echo $value->country_name . ' - ' . $value->country_code . '<br>';
                    }
                }
                ?>
            </div>

            <p class="footer">Page rendered in <strong>{elapsed_time}</strong> seconds. <?php echo (ENVIRONMENT === 'development') ? 'CodeIgniter Version <strong>' . CI_VERSION . '</strong>' : '' ?></p>
        </div>

    </body>
</html>
\$\endgroup\$
3
\$\begingroup\$

Looks pretty good. Here are just some thoughts but nothing major:

  • It would be more future-proof (and probably efficient) to use regular expressions in lieu of parsing hard-coded line numbers (in case the data has rows added later):

  • For the _save_data() you could parse like so:

    $string = '{THE CONTENT FROM THE COUNTRIES LIST PAGE}';
    $sub = preg_replace('/.+?(?=AD\s\s\s)/s', '', $string); // this strips the pre-text
    $list = preg_split('/$\R?^/m', $sub); // This splits by line
    $countries = array();
    
    foreach ($list as $item) {
        $arr = explode('   ', $item);
        $countries[$arr[0]] = $arr[1]; 
    }
    
    asort($countries); // sort by values
    
    //print_r($countries); 
    /** Array
        (
            [AF] => Afghanistan
            [AL] => Albania
            [DZ] => Algeria
            [AS] => American Samoa
            [AD] => Andorra
            ... */
    

    This way you do not need to pass limit to get_countries().

  • You could also use a view helper or template for the output section to better format. <br> can get pretty ugly so I would do a list (<ul>) or something. Overall, it seems pretty logical.

\$\endgroup\$
0
\$\begingroup\$

Feedback

The code looks good so far. There appears to be good separation between the model, the view and the controller. I like how the model methods are concise - none more than 4 lines. The controller method _save_data() is a little on the lengthy side but hopefully the feedback below and in the answer by Tomanow will allow you to improve that method.

Suggestions

Regular expression

I agree with Tomanow's answer (except that you might not need to worry about stripping the pre-text, presuming that the pattern matching only matches country codes and names). A regular expression can be used in _save_data() to match each relevant row, and using a named sub-pattern, the code and name of each country can be selected:

$pattern = '/^'.                             //beginning of line
    '(?P<country_code>[A-Za-z]{2,4})'.       // named sub-pattern for code: 2-4 alpha chars
    '\s{3}'.                                 //3 whitespace characters
    '(?P<country_name>[A-Za-z()\s\.,\'-]+)'. // named sub-pattern for name
    '$/';                                    //end of line

If that pattern matches any rows, $matches['code'] will have the country code and $matches['name'] will have the country name. In theory, $matches could be sent to the insert_entry() model method, though that might be a weird design to accept the fields to insert directly (and the numeric indexes might need to be removed).

In theory the logic involving $startLine and $endLine can be removed by simply checking of the pattern matches.

$lines = file('http://pastebin.com/raw.php?i=943PQQ0n');
$pattern = '/^(?P<country_code>[A-Za-z]{2,4})\s{3}(?P<country_name>[A-Za-z()\s\.,\'-]+)$/';

// Loop through our array, show HTML source as HTML source; and line numbers too.
foreach ($lines as $line_num => $line) {
    preg_match($pattern, $line, $matches);
    if (count($matches)) {
        $count = $this->country_model->count_by_code($matches['country_code']);
        if (!$count) {
            $this->country_model->insert_entry($matches['country_code'], $matches['country_name']);
        }
    }
}

See a demonstration of the matching here in this playground example.

Useless variable $lineNo

In your method _save_data() there is a variable $lineNo that gets incremented each time. If you needed the count of each line, the variable $line_num (from the foreach statement) could be used instead (and added 1 to)...

foreach ($lines as $line_num => $line) {
    $lineNo++;
\$\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.