V
V
v1ad13r2018-10-25 13:35:28
PHP
v1ad13r, 2018-10-25 13:35:28

Please rate my poor OOP?

<?php

Class MyParser{	

  function __construct(){
    $this->regxp = '/(^\d+.\d+.\d+.\d+)\s*- -\s*([[]\d+\/\S+\s*[-|+]\d+])\s*"((POST|GET) \S+\s*\S+)"\s(\d+)\s*(\d+|-)\s*(\d+ \d+)?\s*"(http:\/\/\S+)"\s*"(\S+)\s*\(((compatible;\s*\S+ \d+.\d;))?(\s*\S+\s*\S+\s*\d+.\d+; \S+\s*\S+)\s*(\(\S+\s*\S+\s*\S+)?(\s*\D+)?/';
    $this->path = 'http_access.log';
  }

  // Читаем файл и достаем строки из $this->path
  function read_file()
  {
    if($handle = fopen($this->path, "r")){
      while(!feof($handle)) {
        yield trim(fgets($handle));
      }

      fclose($handle);
    }
  }

  // Получаем интересующие нас значения файла с помощью регулярки
  function get_log_array()
  {
    $iterator = $this->read_file();
    
    foreach ($iterator as $v => $k) {
      if(preg_match($this->regxp, $k, $m)){
        $ips[] 			= $m[1];
        $traffic[]  	= $m[6];
        $statusCode[] 	= $m[5];
        $urls[]         = $m[12];

        $main = array(
          'ips'        => $ips,
          'traffic'    => $traffic,
          'statusCode' => $statusCode,
          'urls'       => $urls,
        );
      }
    }
    return $main;
  }

  // Получаем кол-во ip
  function get_views()
  {
    return count($this->get_log_array()['ips']);	
  }

  // Получаем трафик 
  function get_traffic()
  {
    $traffic_arr = $this->get_log_array()['traffic'];
    for ($i = 0; $i < $this->get_views(); $i++) { 
      $traffic += (int)$traffic_arr[$i];
    }
    return $traffic;
  }

  // Получаем кол-во уникальных url
  function get_urls(){
    $urls = $this->get_log_array()['urls'];
    for ($i = 0; $i < $this->get_views(); $i++) { 
      $urls[] .= $urls[$i];
    }
    return count(array_unique($urls));
  }

  // получаем статус-код
  function get_status_codes(){
    $status_codes = $this->get_log_array()['statusCode'];
    $a = $b = 0;
    for ($i = 0; $i < count($status_codes); $i++) {
      if($status_codes[$i] == '200'){
        $a++;
      }
      elseif($status_codes[$i] == '301'){
        $b++;
      }
    }
    $sc = array(
      '200' => $a,
      '301' => $b,
    );

    return $sc;
  }


  // Формируем массив и json файл с этим массивом
  function create_json(){
    $file = file_get_contents('result.json');
    $log = json_encode($file, TRUE);
    unset($file);
    $log = array(
      'views' => $this->get_views(),
      'urls'	=> $this->get_urls(),
      'traffic' => $this->get_traffic(),
      'crawlers' => [
        'Google' => 2,
        'Bing' => 0,
        'Baidu' => 0,
        'Yandex' => 0,
      ],
      'statusCodes' => $this->get_status_codes('http_access.log'),
    );
    file_put_contents('result.json', json_encode($log, JSON_PRETTY_PRINT));
    unset($log);
  }
}

$obj = new MyParser();
$obj->create_json();

The bottom line is that I was given the task to parse a file in which the following lines:
84.242.208.111- - [11/May/2013:06:31:00 +0200] "POST /chat.php HTTP/1.1" 200 354 " bim-bom .ru ""Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0"
is one line.
It was necessary to get the information of interest from the string and send it to the json file.
{
    "views": 16,
    "urls": 4,
    "traffic": 187990,
    "crawlers": {
        "Google": 2,
        "Bing": 0,
        "Baidu": 0,
        "Yandex": 0
    },
    "statusCodes": {
        "200": 14,
        "301": 2
    }
}

I did as requested, everything works. But for some reason I was denied this decision. I would love to know why.
In general, we need criticism of the code and oop)

Answer the question

In order to leave comments, you need to log in

3 answer(s)
S
Stanislav, 2018-10-25
@v1ad13r

First: start studying the architectural part of programming, study design patterns, study SOLID, DRY, KISS and other buzzwords, try to understand all this, or, at least, memorize it. Everything will come with experience, initially everyone did not understand why everything is so complicated, but this complexity is due to innumerable liters of tears and spent nerves, everything is not just like that.
Apparently this is a test or educational task. You were required to overengineer a simple task. Let's try:
The essence of the problem - there is a file with a certain data storage structure, the structure is string. It is required to convert this file into another data structure and output this structure in json format. The task is clear.
Let's break the problem into separate independent stages:
1) Converting one data structure (text file) to another ( an object understood by PHP , for example)
2) Converting this data structure to Json format.
Now let's consider this task from the point of view of OOP, let's start thinking not from a specific implementation, but from an interface and abstraction ( we do not parse a specific file, we just parse a file, we do not translate it into a specific json representation, we simply translate it into a representation ):
We need 2 classes - directly the class that reads the file and converts it to the simplest data type (for example, PHP array). The second class is a converter of the simplest data type of the parser into some specific type:

  1. LogFileReaded implements/extends FileReaderContract (interface, possibly an abstract class if pre-installed logic is needed)
    Directly our converter (do not forget that you need to pass the path to the file and all settings from outside, for example, to the constructor. You can’t, like you, hardcode everything inside file, in extreme cases - you can use the configuration) , receives a file of a certain type with a certain known structure as input, outputs information from the file converted to the base type (an array, in our case). If we ever need to parse another data type or the data structure is changed, we can write a separate class for this logic, and not destroy the integrity of the already running code, adding new logic there. Remember - you need to strive not to change, but to supplement .
    The abstraction contains a contract for the output() method, and the constructor accepts the original data. In the specific implementation of JsonPresenter, output() will have a banal json_encode() (yes, this is normal, no, the class is not superfluous and no, json_encode() cannot be shove into the parser itself) And now to the question - why not just shove it all into the parser and instead of an array, give json: in the future, when the system expands - we will need to present the data in the form of XML - what will we do then - rewrite the entire parser code to add a switch case "json", etc.? What if something breaks in the whole system? And if there are so many presentation options that the file will simply not be readable? And with this approach, it will be enough just to write a new XMLPresenter class, or even ExcelPresenter, which will not output a line on the output, but the whole file (let's omit the output typing for now)). This class can also be implemented asdecorator (pattern), and much more.
    As a result, we get a completely tolerable and, more importantly, extensible (within reasonable limits) architecture , do not forget to register the client class and define specific implementations in one place - you can stumble and add a dependency container here.
    For example: in the end, if you have already been promoted, and instead of parsing you have begun to deal with higher matters - a new programmer, in order to add data conversion logic in Excel , does not need to know how exactly you converted this data to json, he does not need to debug your code, it is enough for him to look at the interface - inherit from it and write his own conversion method and then use it in the right place.
    PSIn this implementation, some points are omitted and simplified for clarity.

A
Adamos, 2018-10-25
@Adamos

Firstly, it's hard to believe that there are not a bunch of ready-made solutions that parse the Apache log.
So the task is obviously educational, on the use of the language and understanding what OOP is.
So, OOP in PHP is to do the dirty work once, and not look into it again, using a ready-made and, if possible, obvious class interface.
You have a one-time footcloth, in which even file names are hardcoded in the code, poor comments instead of PHPDoc, and in general the feeling that you started doing OOP yesterday and consider it just an opportunity to drive more functions into one class.
Well, the result is correct. You don't need to fix this decision, you need to practice OOP in PHP for a while and come up with an appropriate paradigm in your thinking. And you can just throw out this class.

E
Eugene Pedya, 2018-11-01
@fpinger

There are "a bunch" of independent tasks:
1. Reading a file (log) line by line.
2. Parsing the string according to the rules.
3. Saving the result of string parsing in some intermediate form.
4. Saving in the desired text format.
5. Saving text to a file.
6. Assembly of all tasks into one simple algorithm.
All of them are best dealt with separately.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question