MVC design to fetch, store, and present data from an external source

Question

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>

Jamal · Accepted Answer · 2016-06-06 02:49:54Z

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.

mickmackusa · Accepted Answer · 2021-02-02 20:54:38Z

This is a CodeIgniter application, so I expect its submission gathering methods to be used rather than native $_GET and $_POST. When you nominate the expected key in the $this->input->post('run') call, CI will automatically set the value to null if it does not exist.
$this->load->model('country_model'); in the constructor is appropriate while ALL methods in the class are using the model. If a controller method does not use the model, then it becomes useless to unconditionally load the model. If this eventually occurs, then the loading of the model should be done in specific controller methods. When you instantiate a class in CI, you can rename it via the second parameter so that your script can consistently use studly casing. $this->load->model('country_model', 'CountryModel'); This is just a code styling suggestion.
I recommend using the more modern square brace syntax when declaring an array instead of the longer/older form -- $data = array();. By declaring with square braces, all instances of declaring or accessing array data has the same syntax style. With functions and language constructs all using parentheses, this gives a visual point of difference which I find to make reading code easier.
$data['list'] = $this->country_model->get_countries(252)->result() should be unconditionally called from index(). Whether or not the user has clicked Run should be unimportant. If the database has any country codes/names, go ahead and present them.
Perhaps the http://pastebin.com/raw.php?i=943PQQ0n url should be a class property like $dataSourceURL. This will allow for simpler code maintenance. Then your _save_data() method can just reference $this->dataSourceURL.
The method name _save_data goes against the advice of PSR-12 coding standards. https://www.php-fig.org/psr/psr-12/#:~:text=Property%20names%20MUST%20NOT%20be%20prefixed,has%20no%20meaning. The underscore should be removed. Also, methods names must be camel-cased according to PSR-1. https://www.php-fig.org/psr/psr-1/#:~:text=Method%20names%20MUST%20be%20declared%20in,.
I think _save_data() is also performing more than one task. It is fetching external data, parsing the text, and saving the data to the database. If you don't want to separate the 3 tasks into 3 methods (for simpler unit testing), then at least give the method a more descriptive name. (fetchAndSaveCountryCodes()?)

The process of fetching and parsing the data can be streamlines more.

Fetch the whole string of text at once.
Use preg_match_all() to capture the desired substrings in pairs with just enough validation to make it reliable.
Rather than making n trips to the model to save individual rows, send all of the data to the model and let the looping happen there.

Maybe something like this:

private function fetchAndSaveCountryCodes():void
{
    preg_match_all(
        '/^([A-Z]+) {3}(\S+(?: \S+)*)/mi',
        file_get_contents($this->dataSourceURL),
        $matches
    );
    if (!$matches) {
        throw new Exception('No valid data extracted from external data source');
    }

    $this->CountryModel->insertRows(
        array_map(
            function($row) {
                return ['code' => $row[1], 'name' => $row[2]];
            },
            $matches
        )
    );
}

I think the database should have code as the PRIMARY_KEY and then name as the only other column. This way you avoid duplicate codes in the table.

The model method would then look something like the following. I prefer to always return something valuable when inserting or updating the database -- it comes in handy when debugging is necessary.

public function insertRows(array $associativeRows): array
{
    $insertedCodes = [];
    foreaach ($associativeRows as $row) {
        if ($this->db->insert($this->table, $row)) {
            $insertedCodes[] = $row['code'];
        }
    }
    return $insertedCodes;
}

Considering #8, #9, and #10, you no longer need a row counting method in your model. This saves you n trips to the model/database -- nice.
The limit on your get_countries() method is an okay consideration, but to make your method more flexible as a shared utility, perhaps just return all columns.
```
function get($limit) {
    return $this->db->order_by("name", "asc")->get($this->table, 0, $limit);
}
```
With $data['list'] being unconditionally declared, your view will no longer need the isset() check before the loop.
I like that you seized the opportunity to show off with the unnecessary {elapsed_time} placeholder feature.

Community · Accepted Answer · 2020-06-10 13:24:26Z

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++;

Stack Exchange Network

MVC design to fetch, store, and present data from an external source

Controller:

Model:

View:

3 Answers 3

Feedback

Suggestions

Regular expression

Useless variable `$lineNo`

Your Answer

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

Hot Network Questions

MVC design to fetch, store, and present data from an external source

Controller:

Model:

View:

3 Answers 3

Feedback

Suggestions

Regular expression

Useless variable $lineNo

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

Useless variable `$lineNo`

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