пришел тут ответ от одной компании, что по результатам тестового задания я не подхожу

не могли бы сказать, есть ли какие-то проблемы в коде приведенном ниже
функционал у него именно тот, что нужно — так это может быть либо проблема в коде, которую я не знаю, либо причины иные
вообще задача заключалась в переделке готового кода, чтобы сделать его правильным (на мой взгляд) с сохранением общей структуры, и добавлении нескольких фич

так что смесь PHP и HTML приведенная в одном месте + стиль именования —
это единственное что на самом деле было вообще сохранено из общей структуры, в процессе работы было переписано 95% начального кода

индентация автоматически исправляется моим редактором кода — не обращайте на нее внимания

<?php
    // !!!!!!!!!!!!!!!!! See my notes about the written code in the README file !!!!!!!!!!!!!!!!!!!!!!!!!

    $developer = 'Test'; // Please replace with your name
    
    abstract class Reader {
        const COLOR = "#cc0000";
        protected $_file = null;
        protected $_initialFilename = null;
        
        public function __construct($file) {
            $this->_initialFilename = $file;
        }
        
        public function __destruct() {
            $this->closeFile();
        }
        
        protected function openFile () {
            $file = $this->_initialFilename;
            
            $file = trim ($file);
            if (!strlen ($file)) {
                throw new Exception ("Empty filename was specified.");
            }
            
            $http = "http://";
            $dataDir = dirname (__FILE__)."/data";
            
            if (substr ($file, 0, strlen($http)) !== $http) {
                // we do the following because it is more secure than file_exists()
                $d = @opendir ($dataDir);
                if (!$d) {
                    throw new Exception ("Failed to open directory with data files.");
                }
                $isFound = false;
                while (($f = readdir ($d)) !== false) {
                    if ($f == "." || $f == "..") {
                        continue;
                    }
                    if ($f === $file) {
                        $isFound = true;
                        break;
                    }
                }
                closedir ($d);
                if (!$isFound) {
                    throw new Exception ("Failed to find the requested data file.");
                }
                $file = $dataDir."/".$file;
            }
            
            if (($this->_file = @fopen($file, 'r')) === false) {
                $this->_file = null;
                throw new Exception ("Failed to open a file.");
            }
        }
        
        protected function closeFile () {
            if (!is_null ($this->_file)) {
                @fclose ($this->_file);
                $this->_file = null;
            }
        }
        
        protected function postprocessData ($data) {
            foreach ($data as $i => &$v) {
                if (!is_array ($v) || sizeof ($v) != 2 || !is_numeric ($v[0]) || !is_numeric ($v[1])) {
                    unset ($v);
                    throw new Exception ("Invalid format of data in row #".$i.".");
                }
                $v[0] = intval ($v[0]);
                $v[1] = intval ($v[1]);
                $v[] = self::COLOR;
            }
            unset ($v);
            
            return $data;
        }
        
        abstract public function getData(&$data);
    }
    
    class CSVReader extends Reader {
        public function getData(&$data) {
            $data = array();
            $this->openFile ();
            while ($arr = fgetcsv($this->_file)) {
                $data[] = $arr;
            }
            $this->closeFile ();
            $data = $this->postprocessData ($data);
        }
    }
    
    class NewReader extends Reader {
        protected $_gradients = array ("#ff0000", "#ee0000", "#dd0000", "#cc0000", 
            "#bb0000", "#aa0000", "#990000", "#880000", "#770000", "#660000");

        public function getData(&$data) {
            $data = array();
            $str = "";
            
            $this->openFile ();
            while (!feof($this->_file)) {
                $str .= fread($this->_file, 8192);
            }
            $this->closeFile ();
            
            if (!strlen ($str)) return;
            $matches = array ();
            preg_match_all ("/(\[\"([0-9]+)\",\"([0-9]+)\"\]([,]?))/", substr ($str, 1, -1), $matches);
            if (isset ($matches[2]) && isset ($matches[3]) && sizeof($matches[2]) == sizeof($matches[3])) {
                foreach ($matches[2] as $i => $v) {
                    $data[] = array ($v, $matches[3][$i]);
                }
            }
            if (strlen ($str) && !sizeof ($data)) {
                throw new Exception ("Invalid format of input data.");
            }
            $data = $this->postprocessData ($data);
        }
        
        public static function sortByValue ($a, $b) {
            if ($a[1] == $b[1]) {
                return 0;
            }
            return $a[1] > $b[1] ? 1 : -1;
        }
        
        // we assume here that timestamps are unique
        public function setMoreGradients ($data) {
            $data2 = $data;
            usort ($data2, array ("NewReader", "sortByValue"));
            foreach ($data as &$v) {
                foreach ($data2 as $i => $v2) {
                    if ($v2[0] == $v[0]) break;
                }
                $v[2] = isset ($this->_gradients[$i]) ? 
                    $this->_gradients[$i] : $this->_gradients[sizeof($this->_gradients[$i]) - 1];
            }
            unset ($v);
            
            return $data;
        }
    }
    
    $msg = "";
    $data = array ();
    try {
        //$reader = new CSVReader('data.csv');
        $reader = new NewReader(isset ($_GET["file"]) ? urldecode($_GET["file"]) : "data");
        $reader->getData($data);
        if (!sizeof ($data)) {
            $msg = "No data was obtained.";
        } else {
            $data = $reader->setMoreGradients($data);
        }
    }
    catch (Exception $e) {
        $msg = $e->getMessage();
    }
    
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <meta name="language" content="en" />    
    <style type="text/css">
        html {
            margin: 0px;
            padding: 0px;
        }
        
        body {
            margin: 0px;
            padding: 20px;
        }
        
        #chart {
            position: relative;
            border-bottom: 1px #000 solid;
            border-left: 1px #000 solid;
            margin: 0px;
            padding: 0px;
        }
        
        #chart .bar {
            background-color: #f00;
            bottom: 0;
            position: absolute;
            padding: 2px;
            margin: 0px;
            color: #fff;
        }        
        
        <?php
        if (!strlen ($msg)) {
            foreach ($data as $i => $v) {
                // W3C validates these styles as errors, but I'm not sure that it should be fixed
                echo 
                    '#bar'.$i.' {
                        background: -webkit-gradient(linear, left top, left bottom, from('.$v[2].'), to(#000));
                        background: -moz-linear-gradient(top, '.$v[2].',  #000);
                    }';
            }
        }
        ?>
        
        #title {
            height: 14px;
            padding: 0px 0px 20px 0px;
            margin: 0px;
        }
    </style>
    <script type="text/javascript" src="http://code.jquery.com/jquery-1.4.2.js"></script>
    <script type="text/javascript" src="chart.js"></script>
    <title>Test</title>
</head>
<body>  
    <?php
    if (strlen ($msg)) {
        echo $msg;
    } else {
    ?>
        <div id="title">
        <?php
        echo 'Litres of something consumed per week by '.$developer;
        ?>    
        </div>
        <div id="chart">
        <?php
        foreach ($data as $i => &$values) {        
            $n = (time() - $values[0]) / (7 * 24 * 3600);
            $title = 
                ($n !== floor($n) ? "more than " : "").
                floor($n)." week".
                (floor ($n) == 1 ? "" : "s")." ago";
            echo 
                '<div class="bar" id="bar'.$i.'" title="'.htmlspecialchars($title).'">'.
                '<input type="hidden" class="value" value="'.$values[1].'" />'.
                strval($values[1]).
                '</div>';
        }
        ?>
        </div>
    <?php
    }
    ?>
</body>
</html>

/* // !!!!!!!!!!!!!!!!! See my notes about the written code in the README file !!!!!!!!!!!!!!!!!!!!!!!!!*/

$(document).ready(function () {
    var max_value = 0;
    var spacing_between_bars = 10;

    var padding_left = parseInt($("body").css("paddingLeft"), 10) + parseInt($("#chart").css("borderLeftWidth"), 10);
    var padding_right = parseInt($("body").css("paddingRight"), 10);
    var padding_bottom = parseInt($("body").css("paddingBottom"), 10) + parseInt($("#chart").css("borderBottomWidth"), 10);
    var padding_top = parseInt($("body").css("paddingTop"), 10);
    
    var chart_height = $(document).height() - ($("#title").outerHeight(true) + padding_top + padding_bottom);
    $('#chart').height(chart_height);
    
    var bar_width_with_spacing = Math.floor(
        ($(document).width() - (padding_left + padding_right + spacing_between_bars)) / 
        $('#chart .bar .value').length
        );
    
    $('#chart .bar .value').each(function (index, value) {
        var new_value = parseInt($(this).val(), 10);
        if (new_value > max_value) {
            max_value = new_value;
        }
    });
    
    $('#chart .bar').each(function (index, value) {
        var value = parseInt($(this).find('.value').first().val(), 10);       
        $(this).css({
            height: parseInt((value / max_value) * chart_height, 10) + 'px', 
            width: (bar_width_with_spacing - spacing_between_bars) + 'px',
            left: (index * bar_width_with_spacing + spacing_between_bars) + 'px'
            });
    });
});

а вот еще одно тестовое задание для другой компании
простенький эмулятор базы данных, запускается с консоли
что здесь может быть не так?

строго говоря от этой компании я ответа еще не получил

выскажите ваши замечания, пожалуйста

<?php
/*
Notes:
1. PHP 5.3.4 on Windows was used for development
2. I did not make beautiful error messages handling, because it is test task. So text messages used in several methods.
   Only those messages which are used several times are set in constants;
3. Only tests specified exactly in the task were performed.
4. All other notes, warnings and errors will be printed on the screen after running the script in console.
*/
class Db
{
    const WARNING_INVALID_NUMBER_OF_ARGS = "WARNING: Invalid number of arguments.";
    
    private static $data = array ();
    private static $tranLog = array ();
    
    public static function run ()
    {
        try
        {
            self::fwrite ("You may start enter commands.\nCase of commands does not matter.".
                "\nCase of keys does.\nOnly keys and values without spaces are supported.\n");
            while (true)
            {
                $line = fgets (STDIN);
                $res = self::parseCommand ($line);
                if (is_array ($res))
                {
                    $res = self::runCommand ($res);
                    if (strlen ($res)) self::fwrite ($res);
                }
                else self::fwrite ($res);
            }
        }
        catch (Exception $e)
        {
            exit ("\nERROR: ".$e->getMessage());
        }
    }
    
    private static function fwrite ($line)
    {
        if (!fwrite (STDOUT, $line."\n")) throw new Exception ("Failed to write to STDOUT.");
    }
    
    private static function parseCommand ($line)
    {
        $matches = array ();
        preg_match_all ("/([^\s]+)/", trim($line), $matches);
        if (!isset ($matches[0]) || !is_array ($matches[0]) || !sizeof ($matches[0]))
            return "WARNING: Unrecognizable empty input.";
        
        $matches[0][0] = ucfirst(strtolower ($matches[0][0]));
        $rc = new ReflectionClass ("Db");
        if (!$rc->hasMethod ("command".$matches[0][0])) 
            return "WARNING: Non-supported command.";
        
        return $matches[0];
    }

    private static function runCommand ($cmd)
    {
        $method = "command".$cmd[0];
        return self::$method(array_slice ($cmd, 1));
    }
    
    private static function commandSet ()
    {
        $args = func_get_args();
        if (!sizeof ($args) || !is_array ($args = $args[0]) || sizeof ($args) != 2) return self::WARNING_INVALID_NUMBER_OF_ARGS;
        
        if (!sizeof (self::$tranLog)) self::$data[$args[0]] = $args[1];
        else self::$tranLog[sizeof (self::$tranLog) - 1][$args[0]] = $args[1];
    }
    
    private static function commandEnd ()
    {
        exit("Bye!");
    }
    
    private static function commandUnset ()
    {
        $args = func_get_args();
        if (!sizeof ($args) || !is_array ($args = $args[0]) || sizeof ($args) != 1) return self::WARNING_INVALID_NUMBER_OF_ARGS;
        
        if (!sizeof (self::$tranLog)) unset (self::$data[$args[0]]);
        else self::$tranLog[sizeof (self::$tranLog) - 1][$args[0]] = null;
    }

    private static function commandGet ()
    {
        $args = func_get_args();
        if (!sizeof ($args) || !is_array ($args = $args[0]) || sizeof ($args) != 1) return self::WARNING_INVALID_NUMBER_OF_ARGS;
        
        $noKey = "NULL";
        
        if (!sizeof (self::$tranLog))
        {
            if (!isset (self::$data[$args[0]])) return $noKey;
            else return self::$data[$args[0]];
        }
        else
        {
            for ($i = sizeof (self::$tranLog) - 1; $i >= 0; $i--)
                if (isset (self::$tranLog[$i][$args[0]])) return self::$tranLog[$i][$args[0]];
            if (!isset (self::$data[$args[0]])) return $noKey;
            else return self::$data[$args[0]];
        }
    }

    private static function commandBegin ()
    {
        $args = func_get_args();
        if (!sizeof ($args) || !is_array ($args = $args[0]) || sizeof ($args) != 0) return self::WARNING_INVALID_NUMBER_OF_ARGS;
        
        self::$tranLog[] = array();
    }
    
    private static function commandRollback ()
    {
        $args = func_get_args();
        if (!sizeof ($args) || !is_array ($args = $args[0]) || sizeof ($args) != 0) return self::WARNING_INVALID_NUMBER_OF_ARGS;
        
        if (!sizeof (self::$tranLog)) return "WARNING: Invalid rollback.";
        unset (self::$tranLog[sizeof (self::$tranLog) - 1]);
    }
    
    private static function commandCommit ()
    {
        $args = func_get_args();
        if (!sizeof ($args) || !is_array ($args = $args[0]) || sizeof ($args) != 0) return self::WARNING_INVALID_NUMBER_OF_ARGS;
        
        $arr = array ();
        foreach (self::$tranLog as $v) $arr = array_merge ($arr, $v);
        
        self::$data = array_merge (self::$data, $arr);
        foreach (self::$tranLog as $k => $v)
            if (is_null ($v)) unset (self::$data[$k]);
        
        self::$tranLog = array ();
    }
}

Db::run();

Здравствуйте, a.k.m., Вы писали:

AKM>пришел тут ответ от одной компании, что по результатам тестового задания я не подхожу


Хмм ну вообще я бы тоже завернул. Дикая смесь стилей, HTML и кода в одном файле — 100% пролет мимо.

Здравствуйте, eRRoRR, Вы писали:

RRR>Здравствуйте, a.k.m., Вы писали:

AKM>>пришел тут ответ от одной компании, что по результатам тестового задания я не подхожу


RRR>Хмм ну вообще я бы тоже завернул. Дикая смесь стилей, HTML и кода в одном файле — 100% пролет мимо.

это не моя смесь
это они в таком виде дали
более того, там был пункт в задании

Fix all the bugs, optimize and improve everything that can be. However don't
rewrite the application from scratch, we want you to keep the bulk of the
existing code and leave the overall structure as it is.

RRR>>Хмм ну вообще я бы тоже завернул. Дикая смесь стилей, HTML и кода в одном файле — 100% пролет мимо.

AKM>это не моя смесь
AKM>это они в таком виде дали
AKM>более того, там был пункт в задании

AKM>Fix all the bugs, optimize and improve everything that can be. However don't
AKM>rewrite the application from scratch, we want you to keep the bulk of the
AKM> existing code and leave the overall structure as it is.


я сам с нуля никогда так не пишу

обычно я посылаю всех работодателей, которые просят сделать неоплачиваемое тестовое задание,
поскольку по моему опыту в итоге с этими работодатели либо ничего не выходит либо они оказываются "чудаками"

но на этот раз сел и сделал сразу для 3х компаний

и вот сейчас просто интересно понять, мог ли я где-то что-то упустить

3е задание не выкладываю поскольку там были правки в достаточно большом наборе файлов и в целом то, как я обычно такие задания пишу можно понять уже по выложенным двум

Я бы рекомендовал продолжить следовать вашему правилу неустанно. Сам так давно делаю и никогда не жалел

Здравствуйте, a.k.m., Вы писали:

AKM>обычно я посылаю всех работодателей, которые просят сделать неоплачиваемое тестовое задание,

Здравствуйте, a.k.m., Вы писали:

AKM>пришел тут ответ от одной компании, что по результатам тестового задания я не подхожу

AKM>не могли бы сказать, есть ли какие-то проблемы в коде приведенном ниже
AKM>функционал у него именно тот, что нужно — так это может быть либо проблема в коде, которую я не знаю, либо причины иные
AKM>вообще задача заключалась в переделке готового кода, чтобы сделать его правильным (на мой взгляд) с сохранением общей структуры, и добавлении нескольких фич

AKM>так что смесь PHP и HTML приведенная в одном месте + стиль именования —
AKM>это единственное что на самом деле было вообще сохранено из общей структуры, в процессе работы было переписано 95% начального кода

AKM>индентация автоматически исправляется моим редактором кода — не обращайте на нее внимания


AKM>

AKM><?php
AKM>    // !!!!!!!!!!!!!!!!! See my notes about the written code in the README file !!!!!!!!!!!!!!!!!!!!!!!!!

AKM>    $developer = 'Test'; // Please replace with your name
    
AKM>    abstract class Reader {
AKM>        const COLOR = "#cc0000";
AKM>        protected $_file = null;
AKM>        protected $_initialFilename = null;
        
AKM>        public function __construct($file) {
AKM>            $this->_initialFilename = $file;
AKM>        }
        
AKM>        public function __destruct() {
AKM>            $this->closeFile();
AKM>        }
        
AKM>        protected function openFile () {
AKM>            $file = $this->_initialFilename;
            
AKM>            $file = trim ($file);
AKM>            if (!strlen ($file)) {
AKM>                throw new Exception ("Empty filename was specified.");
AKM>            }
            
AKM>            $http = "http://";
AKM>            $dataDir = dirname (__FILE__)."/data";
            
AKM>            if (substr ($file, 0, strlen($http)) !== $http) {
AKM>                // we do the following because it is more secure than file_exists()
AKM>                $d = @opendir ($dataDir);
AKM>                if (!$d) {
AKM>                    throw new Exception ("Failed to open directory with data files.");
AKM>                }
AKM>                $isFound = false;
AKM>                while (($f = readdir ($d)) !== false) {
AKM>                    if ($f == "." || $f == "..") {
AKM>                        continue;
AKM>                    }
AKM>                    if ($f === $file) {
AKM>                        $isFound = true;
AKM>                        break;
AKM>                    }
AKM>                }
AKM>                closedir ($d);
AKM>                if (!$isFound) {
AKM>                    throw new Exception ("Failed to find the requested data file.");
AKM>                }
AKM>                $file = $dataDir."/".$file;
AKM>            }
            
AKM>            if (($this->_file = @fopen($file, 'r')) === false) {
AKM>                $this->_file = null;
AKM>                throw new Exception ("Failed to open a file.");
AKM>            }
AKM>        }
        
AKM>        protected function closeFile () {
AKM>            if (!is_null ($this->_file)) {
AKM>                @fclose ($this->_file);
AKM>                $this->_file = null;
AKM>            }
AKM>        }
        
AKM>        protected function postprocessData ($data) {
AKM>            foreach ($data as $i => &$v) {
AKM>                if (!is_array ($v) || sizeof ($v) != 2 || !is_numeric ($v[0]) || !is_numeric ($v[1])) {
AKM>                    unset ($v);
AKM>                    throw new Exception ("Invalid format of data in row #".$i.".");
AKM>                }
AKM>                $v[0] = intval ($v[0]);
AKM>                $v[1] = intval ($v[1]);
AKM>                $v[] = self::COLOR;
AKM>            }
AKM>            unset ($v);
            
AKM>            return $data;
AKM>        }
        
AKM>        abstract public function getData(&$data);
AKM>    }
    
AKM>    class CSVReader extends Reader {
AKM>        public function getData(&$data) {
AKM>            $data = array();
AKM>            $this->openFile ();
AKM>            while ($arr = fgetcsv($this->_file)) {
AKM>                $data[] = $arr;
AKM>            }
AKM>            $this->closeFile ();
AKM>            $data = $this->postprocessData ($data);
AKM>        }
AKM>    }
    
AKM>    class NewReader extends Reader {
AKM>        protected $_gradients = array ("#ff0000", "#ee0000", "#dd0000", "#cc0000", 
AKM>            "#bb0000", "#aa0000", "#990000", "#880000", "#770000", "#660000");

AKM>        public function getData(&$data) {
AKM>            $data = array();
AKM>            $str = "";
            
AKM>            $this->openFile ();
AKM>            while (!feof($this->_file)) {
AKM>                $str .= fread($this->_file, 8192);
AKM>            }
AKM>            $this->closeFile ();
            
AKM>            if (!strlen ($str)) return;
AKM>            $matches = array ();
AKM>            preg_match_all ("/(\[\"([0-9]+)\",\"([0-9]+)\"\]([,]?))/", substr ($str, 1, -1), $matches);
AKM>            if (isset ($matches[2]) && isset ($matches[3]) && sizeof($matches[2]) == sizeof($matches[3])) {
AKM>                foreach ($matches[2] as $i => $v) {
AKM>                    $data[] = array ($v, $matches[3][$i]);
AKM>                }
AKM>            }
AKM>            if (strlen ($str) && !sizeof ($data)) {
AKM>                throw new Exception ("Invalid format of input data.");
AKM>            }
AKM>            $data = $this->postprocessData ($data);
AKM>        }
        
AKM>        public static function sortByValue ($a, $b) {
AKM>            if ($a[1] == $b[1]) {
AKM>                return 0;
AKM>            }
AKM>            return $a[1] > $b[1] ? 1 : -1;
AKM>        }
        
AKM>        // we assume here that timestamps are unique
AKM>        public function setMoreGradients ($data) {
AKM>            $data2 = $data;
AKM>            usort ($data2, array ("NewReader", "sortByValue"));
AKM>            foreach ($data as &$v) {
AKM>                foreach ($data2 as $i => $v2) {
AKM>                    if ($v2[0] == $v[0]) break;
AKM>                }
AKM>                $v[2] = isset ($this->_gradients[$i]) ? 
AKM>                    $this->_gradients[$i] : $this->_gradients[sizeof($this->_gradients[$i]) - 1];
AKM>            }
AKM>            unset ($v);
            
AKM>            return $data;
AKM>        }
AKM>    }
    
AKM>    $msg = "";
AKM>    $data = array ();
AKM>    try {
AKM>        //$reader = new CSVReader('data.csv');
AKM>        $reader = new NewReader(isset ($_GET["file"]) ? urldecode($_GET["file"]) : "data");
AKM>        $reader->getData($data);
AKM>        if (!sizeof ($data)) {
AKM>            $msg = "No data was obtained.";
AKM>        } else {
AKM>            $data = $reader->setMoreGradients($data);
AKM>        }
AKM>    }
AKM>    catch (Exception $e) {
AKM>        $msg = $e->getMessage();
AKM>    }
    
AKM>?>
AKM><!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
AKM><html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
AKM><head>
AKM>    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
AKM>    <meta name="language" content="en" />    
AKM>    <style type="text/css">
AKM>        html {
AKM>            margin: 0px;
AKM>            padding: 0px;
AKM>        }
        
AKM>        body {
AKM>            margin: 0px;
AKM>            padding: 20px;
AKM>        }
        
AKM>        #chart {
AKM>            position: relative;
AKM>            border-bottom: 1px #000 solid;
AKM>            border-left: 1px #000 solid;
AKM>            margin: 0px;
AKM>            padding: 0px;
AKM>        }
        
AKM>        #chart .bar {
AKM>            background-color: #f00;
AKM>            bottom: 0;
AKM>            position: absolute;
AKM>            padding: 2px;
AKM>            margin: 0px;
AKM>            color: #fff;
AKM>        }        
        
AKM>        <?php
AKM>        if (!strlen ($msg)) {
AKM>            foreach ($data as $i => $v) {
AKM>                // W3C validates these styles as errors, but I'm not sure that it should be fixed
AKM>                echo 
AKM>                    '#bar'.$i.' {
AKM>                        background: -webkit-gradient(linear, left top, left bottom, from('.$v[2].'), to(#000));
AKM>                        background: -moz-linear-gradient(top, '.$v[2].',  #000);
AKM>                    }';
AKM>            }
AKM>        }
AKM>        ?>
        
AKM>        #title {
AKM>            height: 14px;
AKM>            padding: 0px 0px 20px 0px;
AKM>            margin: 0px;
AKM>        }
AKM>    </style>
AKM>    <script type="text/javascript" src="http://code.jquery.com/jquery-1.4.2.js"></script>
AKM>    <script type="text/javascript" src="chart.js"></script>
AKM>    <title>Test</title>
AKM></head>
AKM><body>  
AKM>    <?php
AKM>    if (strlen ($msg)) {
AKM>        echo $msg;
AKM>    } else {
AKM>    ?>
AKM>        <div id="title">
AKM>        <?php
AKM>        echo 'Litres of something consumed per week by '.$developer;
AKM>        ?>    
AKM>        </div>
AKM>        <div id="chart">
AKM>        <?php
AKM>        foreach ($data as $i => &$values) {        
AKM>            $n = (time() - $values[0]) / (7 * 24 * 3600);
AKM>            $title = 
AKM>                ($n !== floor($n) ? "more than " : "").
AKM>                floor($n)." week".
AKM>                (floor ($n) == 1 ? "" : "s")." ago";
AKM>            echo 
AKM>                '<div class="bar" id="bar'.$i.'" title="'.htmlspecialchars($title).'">'.
AKM>                '<input type="hidden" class="value" value="'.$values[1].'" />'.
AKM>                strval($values[1]).
AKM>                '</div>';
AKM>        }
AKM>        ?>
AKM>        </div>
AKM>    <?php
AKM>    }
AKM>    ?>
AKM></body>
AKM></html>
AKM>

AKM>

AKM>/* // !!!!!!!!!!!!!!!!! See my notes about the written code in the README file !!!!!!!!!!!!!!!!!!!!!!!!!*/

AKM>$(document).ready(function () {
AKM>    var max_value = 0;
AKM>    var spacing_between_bars = 10;

AKM>    var padding_left = parseInt($("body").css("paddingLeft"), 10) + parseInt($("#chart").css("borderLeftWidth"), 10);
AKM>    var padding_right = parseInt($("body").css("paddingRight"), 10);
AKM>    var padding_bottom = parseInt($("body").css("paddingBottom"), 10) + parseInt($("#chart").css("borderBottomWidth"), 10);
AKM>    var padding_top = parseInt($("body").css("paddingTop"), 10);
    
AKM>    var chart_height = $(document).height() - ($("#title").outerHeight(true) + padding_top + padding_bottom);
AKM>    $('#chart').height(chart_height);
    
AKM>    var bar_width_with_spacing = Math.floor(
AKM>        ($(document).width() - (padding_left + padding_right + spacing_between_bars)) / 
AKM>        $('#chart .bar .value').length
AKM>        );
    
AKM>    $('#chart .bar .value').each(function (index, value) {
AKM>        var new_value = parseInt($(this).val(), 10);
AKM>        if (new_value > max_value) {
AKM>            max_value = new_value;
AKM>        }
AKM>    });
    
AKM>    $('#chart .bar').each(function (index, value) {
AKM>        var value = parseInt($(this).find('.value').first().val(), 10);       
AKM>        $(this).css({
AKM>            height: parseInt((value / max_value) * chart_height, 10) + 'px', 
AKM>            width: (bar_width_with_spacing - spacing_between_bars) + 'px',
AKM>            left: (index * bar_width_with_spacing + spacing_between_bars) + 'px'
AKM>            });
AKM>    });
AKM>});
AKM>

Тест. задание делал бесплатно? Молодец.

Здравствуйте, a.k.m., Вы писали:
AKM>Fix all the bugs, optimize and improve everything that can be. However don't
AKM>rewrite the application from scratch, we want you to keep the bulk of the
AKM> existing code and leave the overall structure as it is.
Как это согласуется с

так что смесь PHP и HTML приведенная в одном месте + стиль именования —
это единственное что на самом деле было вообще сохранено из общей структуры, в процессе работы было переписано 95% начального кода

?
По коду:

            if (($this->_file = @fopen($file, 'r')) === false) {
                $this->_file = null;
                throw new Exception ("Failed to open a file.");
            }

Очень оригинальная конструкция. Я, конечно, от php далек, но чет мне подсказывает, что тут что то не то написано.

if (!is_array ($v) || sizeof ($v) != 2 || !is_numeric ($v[0]) || !is_numeric ($v[1])) {

Это даже комментировать не хочется — атас еще тот. А такой красоты до черта:

                $v[0] = intval ($v[0]);
                $v[1] = intval ($v[1]);
                $v[] = self::COLOR;

Ужас виден почти в каждой строке, а я смотрел немного и очень мальком. Лично я б может структуру и оставил как есть, вот только переменные б назвал по нормальному, выделил методы с осмысленными названиями плюс в html если б и оставил php код, то только в виде небольших компактных вставок вроде <?php displayDeveloperName();?>. Плюс те места, которые лень было править сейчас, пометил бы TODO.
Итого, если тебя просили только пофиксить баги, то сделал слишком много. Если тебя просили навести порядок в этом ужасе, то сделал слишком мало. Лично я может и оставил все в одном файле, вот только от оригинального кода там бы осталось очень и очень мало, плюс бы добавил кучу TODO. При этом послал бы сообщение первоначально с пояснениями, что я собираюсь делать и допустимо ли это. Если не допустимо, и я должен что пофиксить и оптимизировать, оставив черти какой мегаюниорский стиль — я б сказал, что извините, но за работу в нечистотах без возможности вычисления я хочу в 5 раз больше денег.

Здравствуйте, elmal, Вы писали:

E>Здравствуйте, a.k.m., Вы писали:
AKM>>Fix all the bugs, optimize and improve everything that can be. However don't
AKM>>rewrite the application from scratch, we want you to keep the bulk of the
AKM>> existing code and leave the overall structure as it is.
E>Как это согласуется с
E>

E>так что смесь PHP и HTML приведенная в одном месте + стиль именования —
E>это единственное что на самом деле было вообще сохранено из общей структуры, в процессе работы было переписано 95% начального кода

?

это согласуется таким образом, что я сохранил основные логические блоки на тех местах, где они были ранее
если бы я убрал эту смесь php и html/css, то по большому счету все вообще нужно было бы разложить по разным файлам, избавившись от html и превратив его в шаблон, потому что все промежуточные решения между тем что есть и раскладкой кода на разных языках по разным файлам в принципе неверные


E>По коду:

            if (($this->_file = @fopen($file, 'r')) === false) {
                $this->_file = null;
                throw new Exception ("Failed to open a file.");
            }

E>Очень оригинальная конструкция. Я, конечно, от php далек, но чет мне подсказывает, что тут что то не то написано.

там все правильно написано
fopen без @ выбрасывает warning-и
единственно правильная проверка на неверный результат это === false
а исходя из общей концепции класс не должен выбрасывать ворнинги, а только наши собственные эксепшны

if (!is_array ($v) || sizeof ($v) != 2 || !is_numeric ($v[0]) || !is_numeric ($v[1])) {

E>Это даже комментировать не хочется — атас еще тот. А такой красоты до черта:

объясняю зачем это нужно
исходные данные — это csv файл либо файл в формате аналогичном csv состоящий из 2х колонок с цифрами
поскольку данные в ридере CSV считываются fgetcsv,
то указанная вами строка — это всего лишь проверка считанных данных на корректность
как вы могли судить по
throw new Exception ("Invalid format of data in row #".$i.".");
который выбрасывается в результате

                $v[0] = intval ($v[0]);
                $v[1] = intval ($v[1]);
                $v[] = self::COLOR;

в этом месте я просто расширяю начальные данные, чтобы потом вывести колонки разными цветами
и блокирую возможность ошибочной обработки за счет преобразований к целому числу

E>Ужас виден почти в каждой строке,

пожалуйста вышлите еще примеры ужаса
пока все что вы указали нормально и описано выше почему это нормально

Здравствуйте, a.k.m., Вы писали:

AKM>Здравствуйте, elmal, Вы писали:

E>>Здравствуйте, a.k.m., Вы писали:
AKM>>>Fix all the bugs, optimize and improve everything that can be. However don't
AKM>>>rewrite the application from scratch, we want you to keep the bulk of the
AKM>>> existing code and leave the overall structure as it is.
E>>Как это согласуется с
E>>

E>>так что смесь PHP и HTML приведенная в одном месте + стиль именования —
E>>это единственное что на самом деле было вообще сохранено из общей структуры, в процессе работы было переписано 95% начального кода

?

AKM>это согласуется таким образом, что я сохранил основные логические блоки на тех местах, где они были ранее
AKM>если бы я убрал эту смесь php и html/css, то по большому счету все вообще нужно было бы разложить по разным файлам, избавившись от html и превратив его в шаблон, потому что все промежуточные решения между тем что есть и раскладкой кода на разных языках по разным файлам в принципе неверные


другими словами вот эта смесь языков и расположение логических блоков на тех местах где они сейчас есть — это единственное что осталось от оригинального кода вообще

Здравствуйте, a.k.m., Вы писали:

AKM>пока все что вы указали нормально и описано выше почему это нормально

        protected $_file = null;
        protected $_initialFilename = null;

По существу это глобальные переменные. Нахрена они в классе, не понимаю. Лично я в аттрибуты класса б такое не выносил. Я потому и обратил внимание на строку открытия файла, которое в самом конце, и кидание исключение.
Нахрена там наследование я тоже не понимаю, кстати.
sortByValue — неправильное имя. Более корректное название — compare.
Самый низ с мешаниной html и php — ужас. И вообще общий стиль — ну совсем программа не похожа на читаемый английский текст. Напрягаться приходится при чтении, а лично я предпочитаю читать без напряга. Регулярные выражения, а далее matches[3] и тому подобное.
Лично я, как программист, знаю 3 числа — 0, 1 и N. Ну хоть переменную б выделить, с нормальным именем, указывающей что там хранится.
Общее мнение видно — тяжело читается. А конкретику даже комментировать не хочется — там слишком много.

Здравствуйте, elmal, Вы писали:

E>Здравствуйте, a.k.m., Вы писали:

AKM>>пока все что вы указали нормально и описано выше почему это нормально

        protected $_file = null;
        protected $_initialFilename = null;

E>По существу это глобальные переменные. Нахрена они в классе, не понимаю. Лично я в аттрибуты класса б такое не выносил. Я потому и обратил внимание на строку открытия файла, которое в самом конце, и кидание исключение.

поскольку они используеются несколькими методами двух классов

E>Нахрена там наследование я тоже не понимаю, кстати.

изначально у них было 2 класса — базовый ридер и csv ридер
в задаче было указано сделать новый ридер и исправить ошибки в имеющихся
поэтому там теперь 2 класса

E>sortByValue — неправильное имя. Более корректное название — compare.

здесь согласен

E>Самый низ с мешаниной html и php — ужас. И вообще общий стиль — ну совсем программа не похожа на читаемый английский текст. Напрягаться приходится при чтении, а лично я предпочитаю читать без напряга. Регулярные выражения, а далее matches[3] и тому подобное.
E>Лично я, как программист, знаю 3 числа — 0, 1 и N. Ну хоть переменную б выделить, с нормальным именем, указывающей что там хранится.
E>Общее мнение видно — тяжело читается. А конкретику даже комментировать не хочется — там слишком много.

это просто потому что без цветовой разметки
в цветовой разметке все нормально
но мешанина изначально была у них — и это единственное что я оставил по причине указанной ранее, а именно все попытки устранить мешанину в итоге закончиваются полным изменением файловой структуры, а это то что они сказали не делать

Здравствуйте, a.k.m., Вы писали:

AKM>не могли бы сказать, есть ли какие-то проблемы в коде приведенном ниже

Люблю критиковать.

1. А зачем там эпичный if с комментарием внутри "// we do the following because it is more secure than file_exists()"? Причем такая хитрая и бесполезная проверка и неверный комментарий. Например, файл может быть удален в интервале между проверкой и открытием. Причем подобное удаление между проверкой и работой (а также модификация) — один из типичных типов уязвимостей для приложений. Какое же оно secure? Файл нужно открывать сразу и проверять, открылся он или нет. И это не говоря уже о гораздо менее экзотическом случае, когда файл может существовать, а прав на чтение каталога не будет.
   
2. Я тоже не понимаю, зачем там наследование. Посмотрев на такую задачу и требование "сохранить старую структуру" я бы просто к ним не пошел.
   
3. Если оставлять наследование, стоит сделать его более осмысленным, что-ли... readData в Reader должна: открывать файл, вызывать readDataImpl (с открытым файлом в качестве параметра), затем вызывать postprocessData на результате и закрывать файл. А так в ридерах какая-то копипаста, которую хочется на статические утилитные методы вместо наследования переделать.
   
4. Не вижу обработки ошибок при чтении файла (т.е. файл открылся, внутри произошла ошибка). Хотя бы закрытие файла должно быть. Т.е. ожидается конструкция вроде try/finally или как она там в PHP зовется.
   
5. Что-то я опять про эпичный if подумал... А что, если там будет https://...?
   
6. openFile должен проверять, не открыт ли файл сейчас. closeFile же зачем-то проверяет правильность вызова. Вот и openFile должна. Или, хотя бы, должна закрывать ранее открытый файл.

Здравствуйте, maxkar, Вы писали:

M>Здравствуйте, a.k.m., Вы писали:

AKM>>не могли бы сказать, есть ли какие-то проблемы в коде приведенном ниже

M>Люблю критиковать.
M>

M>
1. А зачем там эпичный if с комментарием внутри "// we do the following because it is more secure than file_exists()"? Причем такая хитрая и бесполезная проверка и неверный комментарий. Например, файл может быть удален в интервале между проверкой и открытием.
   
   а я им в отдельном ридми файле так и написал — никакого стрессового тестирования не проводилось, поскольку это тестовая задача
   проверкой я лишь закрыл самую большую уязвимость
   
   M>
2. Если оставлять наследование, стоит сделать его более осмысленным, что-ли... readData в Reader должна: открывать файл, вызывать readDataImpl (с открытым файлом в качестве параметра), затем вызывать postprocessData на результате и закрывать файл. А так в ридерах какая-то копипаста, которую хочется на статические утилитные методы вместо наследования переделать.
   M>
3. Не вижу обработки ошибок при чтении файла (т.е. файл открылся, внутри произошла ошибка). Хотя бы закрытие файла должно быть. Т.е. ожидается конструкция вроде try/finally или как она там в PHP зовется.
   
   а при ошибке чтения просто false вернется и при конкатенации в строку либо сравнении на валидность в цикле все сработает корректно
   исключения эта функция не выкидывает
   
   M>
4. Что-то я опять про эпичный if подумал... А что, если там будет https://...?
   
   это же тестовое задание
   изначально вопрос про ssl даже не стоял
   это кстати к вопросу о правильном чтении ТЗ — нсли человека попросили сделать какую-то фичу и подчистить код, а он начал городить проверки на все и вся, то это плохо
   ТЗ надо писать и читать формально
   
   
   M>
5. openFile должен проверять, не открыт ли файл сейчас. closeFile же зачем-то проверяет правильность вызова. Вот и openFile должна. Или, хотя бы, должна закрывать ранее открытый файл.
   
   в этом вы правы
   
   M>

Здравствуйте, a.k.m., Вы писали:

AKM>а вот еще одно тестовое задание для другой компании
AKM>простенький эмулятор базы данных, запускается с консоли
AKM>что здесь может быть не так?

1. Какой требуемый уровень безопасности? По информации об ошибках ничего не понятно. Если требуется скрыть от пользователя способы работы и api, возможно, стоит выдавать еще меньше информации. Если же требуется удобство, требуется выдавать больше информации. Например, в сообщении об аргументах стоило бы указать, сколько их было и сколько ожидалось. В неопознанной команде привести и саму команду (без аргументов). Это вроде как автоматический навык, даже для отладки. При этом, допустим, особо красивых сообщений, локализации и т.п. не нужно.
   
2. Проверку на количество аргументов нужно бы вынести в один метод, получающий аргументы и требуемое количество. Вероятно, он бы кидал исключение, которое приходилось бы ловить. Это нормально (ошибки не предполагаются частыми).
   
3. Проверку на существование логичнее было бы, наверное, перенести в runCommand.
   
4. А там правильно вложенные транзакции поддерживаются? Коммит коммитит все, а rollback откатывает только последнюю операцию?
   
5. Может быть, TransactionLog стоило бы перенести в отдельный класс. С методами вроде addItem/removeItem/findItem и beginNested/rollbackNested/commit, например.

Здравствуйте, maxkar, Вы писали:

M>

M>
1. Какой требуемый уровень безопасности? По информации об ошибках ничего не понятно. Если требуется скрыть от пользователя способы работы и api, возможно, стоит выдавать еще меньше информации. Если же требуется удобство, требуется выдавать больше информации. Например, в сообщении об аргументах стоило бы указать, сколько их было и сколько ожидалось. В неопознанной команде привести и саму команду (без аргументов). Это вроде как автоматический навык, даже для отладки. При этом, допустим, особо красивых сообщений, локализации и т.п. не нужно.
   M>
2. Проверку на количество аргументов нужно бы вынести в один метод, получающий аргументы и требуемое количество. Вероятно, он бы кидал исключение, которое приходилось бы ловить. Это нормально (ошибки не предполагаются частыми).
   M>
3. Проверку на существование логичнее было бы, наверное, перенести в runCommand.
   M>
4. А там правильно вложенные транзакции поддерживаются? Коммит коммитит все, а rollback откатывает только последнюю операцию?
   M>
5. Может быть, TransactionLog стоило бы перенести в отдельный класс. С методами вроде addItem/removeItem/findItem и beginNested/rollbackNested/commit, например.
   M>

* требований по безопасности никаких не было — сделал минимум
* насчет аргументов — да — была такая мысль — просто объем кода будет почти тот же самый
* да, вложенные поддерживаются — хотя проверял конечно только на наборе тестов выданных в задании
* не надо усложнизмов

Здравствуйте, a.k.m., Вы писали:


AKM>а я им в отдельном ридми файле так и написал — никакого стрессового тестирования не проводилось, поскольку это тестовая задача
AKM>проверкой я лишь закрыл самую большую уязвимость
Не закрыли, а чуть не создали! Какая там была уязвимость, если файл не существовал? Файл бы взял и не открылся. Это вы вполне корректно проверяете. Что дает еще одна дополнительная проверка (кроме чуть более детальной диагностики) — не понятно. Именно лишние проверки приводят к уязвимости, а не их отсутствие.

M>>

Что-то я опять про эпичный if подумал... А что, если там будет https://...?

AKM>это же тестовое задание
AKM>изначально вопрос про ssl даже не стоял
Ну да. Вот поэтому и вопрос. Вы написали проверку, которая мало того, что лишняя (без нее все работает корректно), так еще и сама по себе работает некорректно в некоторых случаях. Без нее вопрос и не стоял бы. Кстати, без if'а https имел шансы отработать корректно. Да и ftp тоже, например. А так нужно будет править проверку. И при необходимости — еще что-то туда детально добавлять (как наличие файла на ftp проверить и зачем?)

Здравствуйте, maxkar, Вы писали:

M>Здравствуйте, a.k.m., Вы писали:


AKM>>а я им в отдельном ридми файле так и написал — никакого стрессового тестирования не проводилось, поскольку это тестовая задача
AKM>>проверкой я лишь закрыл самую большую уязвимость
M>Не закрыли, а чуть не создали! Какая там была уязвимость, если файл не существовал? Файл бы взял и не открылся. Это вы вполне корректно проверяете. Что дает еще одна дополнительная проверка (кроме чуть более детальной диагностики) — не понятно. Именно лишние проверки приводят к уязвимости, а не их отсутствие.

а если на входе было бы "../../somefolder/passwd" ?

Здравствуйте, a.k.m., Вы писали:

AKM>Здравствуйте, maxkar, Вы писали:

AKM>* да, вложенные поддерживаются — хотя проверял конечно только на наборе тестов выданных в задании
Ну они необычно у вас поддерживаются. Стандартный commit закрывает только самую вложенную транзакцию. Возможно, у вас все реализовано согласно заданию.

AKM>* насчет аргументов — да — была такая мысль — просто объем кода будет почти тот же самый
AKM>* не надо усложнизмов

А здесь возникает вопрос о предназначении тестового задания. Требуется ли "минимальный рабочий вариант" aka "код на выброс" или требуется код, похожий на решение большой задачи. Сами интервьюверы обычно не утруждают конкретизировать, что именно им нужно в ответ на задание. Вариант "на выброс" обычно подходит только для юниоров. Он проверяет способность писать минимально рабочий код (алгоритм) и знание конструкций языка. Для остальных уровней я бы смотрел на декомпозицию, стиль кодирования и т.п. (хотя я бы тоже тестового задания не давал). Вот здесь с этой точки зрения транзакции вынести стоило бы (ну или на собеседовании об этом поговорить, например). Вообще, это претензия не к вашему коду, сколько к нечеткой постановке задачи (назначение и время жизни кода).