• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

prośba o code-review

Object Storage Arubacloud
0 głosów
179 wizyt
pytanie zadane 11 lipca 2018 w PHP przez sapero Gaduła (4,100 p.)

Hej, proszę o ocenę kodu, dopiero uczę się php i nie wiem czy dobrze to zaprojektowałem. Formularzem przesyłam plik który odbiera kontroler, który korzysta z klasy odpowiadającej za obsługę żądania, następnie ta klasa korzysta z serwisu obsługującego dodawanie zipów, rozpakowywanie, itp..

kontroler:

<?php

use App\Validations\ZipFileValidator;
use App\Models\File\ZipFile;
use Symfony\Component\HttpFoundation\Request;
use App\Requests\AddAppRequest;

class testController extends Controller
{

    private $request;
    public $errors;

    /**
     * STRONA
     */
    public function indexAction()
    {
        $this->request = Request::createFromGlobals();

        #Request - create new app
        if ($this->request->get('action') == 'app_add') {
            //create data file from input
            $zipFileInputName = 'fileToUpload';

            //try add file
            $AddAppRequest = new AddAppRequest($zipFileInputName);
            $this->errors = $AddAppRequest->getErrors();
            $this->feedback = $AddAppRequest->getFeedback();
        }
        //widok
        $this->render('test/index.twig', ['errors' => $this->errors, 'feedback' => $this->feedback]);
    }
}

Klasa obsługująca formularz przesyłająca plik zip:

<?php
namespace App\Requests;

use Symfony\Component\HttpFoundation\Request;
use Entity\Apps;
use App\Models\App\App;
use App\Validations\AppValidator;
use App\Validations\ImageValidator;
use App\Services\Files\IconServices;
use App\Services\Files\ZipFileServices;
use App\Validations\ZipFileValidator;
use App\Models\File\ZipFile;

class AddAppRequest
{

    private $appsEntity;
    public $errors = [];
    public $feedback = [];
    public $zipFileServices;

    public function __construct($zipFileInputName)
    {
        $this->request = Request::createFromGlobals();
        $this->appsEntity = new Apps();
        //
        $this->zipFileServices = new ZipFileServices($zipFileInputName);
        $this->zipFileServices->validZipFile();

        $this->zipFileServices->moveFile();
        $this->zipFileServices->unzipFile();

        $this->mergeErrors();
        $this->mergeFeedbacks();
    }


    public function getErrors()
    {
        return $this->errors;
    }

    public function getFeedback()
    {
        return $this->feedback;
    }

    private function mergeErrors()
    {
        $this->errors = array_merge($this->errors, $this->zipFileServices->getErrors());
    }
    private function mergeFeedbacks()
    {
        $this->feedback = array_merge($this->feedback, $this->zipFileServices->getFeedback());
    }
}

Serwis Zip:

<?php
namespace App\Services\Files;

use App\Validations\ZipFileValidator;
use Symfony\Component\HttpFoundation\Request;
use ZipArchive;
use App\Models\File\ZipFile;

class ZipFileServices
{
    private $request;
    private $zipFileName;
    private $zipArchive;
    public $errors = [];
    public $feedback = [];

    public function __construct($zipFileInputName)
    {
        $this->request = Request::createFromGlobals();
        $this->zipArchive = new ZipArchive;
        $this->zipFile = $this->request->files->get($zipFileInputName);
        $this->zipFileName = $this->request->files->get($zipFileInputName)->getClientOriginalName();
        $this->zipFileInputName = $zipFileInputName;
    }

    public function moveFile($dir = ROOT_ZIP_FOLDER)
    {
        if (empty($this->errors)) {
            //OK
            if ($this->request->files->get('fileToUpload')->move($dir, $this->zipFileName)) {
                $this->feedback[] = 'Plik został przesłany do :' . $dir;
                return true;
            } else {
                $this->errors[] = 'Błąd podczas przenoszenia!';
            }
        } else {
            $this->errors[] = 'Plik nie został przesłany, ponieważ nie spełnił warunków!';
        }
    }

    public function validZipFile()
    {
        $zipFileValidation = new ZipFileValidator(new ZipFile($this->zipFileInputName, $this->zipFile));
        $this->errors = $zipFileValidation->getErrors();
    }

    public function unzipFile($dir = ROOT_ZIP_FOLDER)
    {
        $this->zipArchive->open($dir . $this->zipFileName);
        $this->zipArchive->extractTo($dir . '/nazwa_pliku/');
        $this->zipArchive->close();
        $this->feedback[] = 'Plik został rozpakowany w katalogu ' . $dir . $this->zipFileName;
    }

    public function getErrors()
    {
        return $this->errors;
    }

    public function getFeedback()
    {
        return $this->feedback;
    }
}

Walidacja pliku:

<?php

namespace App\Validations;

use App\Models\File\ZipFile;

class ZipFileValidator
{
    //Modificators
    private $fileMaxSize = 23000000;//23MB
    private $maxCountChar = 30;
    //Zip File
    private $zipFile;
    private $fileName;
    private $fileSize;
    //Errors after validation
    private $errors = [];

    public function __construct(ZipFile $zipFile)
    {
        $this->zipFile = $zipFile;
        $this->fileName = $this->zipFile->getFileName();
        $this->fileSize = $this->zipFile->getFileSize();
        //sprawdź walidacje
        $this->validFile();
    }

    public function validFile()
    {
        if (file_exists(ROOT_ZIP_FOLDER . $this->fileName)) {
            $this->errors[] = "Plik o nazwie ". $this->fileName . ' już istnieje w ' . ROOT_ZIP_FOLDER;
        }
        if (mb_strlen($this->fileName) >= $this->maxCountChar) {
            $this->errors[] = "Nazwa przesyłanego pliku jest zbyt długa. Plik ma w swojej nazwię za dużo znaków";
        }
        if (strpos($this->fileName, '.zip') == false) {
            $this->errors[] = "To nie jest plik *.zip";
        }
        if ($this->fileName == null) {
            $this->errors[] = "Nie przesłałeś pliku!";
        }
        if ($this->fileSize > $this->fileMaxSize) {
            $this->errors[] = "Plik jest za duży ma: " . $this->fileSize . "kb";
        }
    }

    public function getErrors()
    {
        return $this->errors;
    }
}

model pliku zip:

<?php

namespace App\Models\File;

use Symfony\Component\HttpFoundation\Request;

class ZipFile
{
    private $request;

    private $fileName;
    private $fileSize;

    public function __construct($inputName, $requestFile)
    {
        $this->request = Request::createFromGlobals();
        $this->inputFile = $requestFile;

        $this->fileName = $this->request->files->get("$inputName")->getClientOriginalName();
        $this->fileSize = $this->request->files->get("$inputName")->getClientSize();
    }

    public function getFileName()
    {
        return $this->fileName;
    }

    public function getFileSize()
    {
        return $this->fileSize;
    }
}

ogólnie przepływ danych wygląda tak:

Kontroler <-> Obsługa zapytania <-> (Serwis zip -> walidacja) <-> model. 

ps. napisałem własne mvc w którym korzystam z Twiga do wyświetlania widoku..

Czy to wogóle ma sens?

1 odpowiedź

0 głosów
odpowiedź 11 lipca 2018 przez Mariusz08 Maniak (62,300 p.)
  1. Nazwa klasy ma mówić o tym, co dana klasa wykonuje (testController mało mówi)
  2. Zamiast tworzyć Requesta z metody createFromGlobals() możesz w argumencie wpisać Request $request i będziesz miał do dyspozycji cały obiekt (coś takiego: public function x(Request $request){} ) (oczywiście jeżeli korzystasz z Sf)
  3. Komentarz STRONA mało mówi
  4. $this->errors = $AddAppRequest->getErrors();
                $this->feedback = $AddAppRequest->getFeedback();

    Dlaczego tworzysz właściwości z errorami? Nie prościej wepchać to do normalnej zmiennej?

  5. if ($this->request->get('action') == 'app_add') {

    Zastosuj silne typowanie === oraz (jeśli chcesz) yoda conditions

  6. #Request - create new app

    To chyba komentarz - nie tworzymy ich w taki sposób

  7. //widok

    Wszystkie komentarz angielskie a ten polski? Poza tym tutaj nie musisz dawać komentarza - programista wie że ta linijka zwraca widok

  8. public function indexAction()

    Możesz określić typ zwracanej wartości

  9. <?php
     
    use App\Validations\ZipFileValidator;
    use App\Models\File\ZipFile;
    use Symfony\Component\HttpFoundation\Request;
    use App\Requests\AddAppRequest;
     
    class testController extends Controller
    {
     
        private $request;
        public $errors;
     
        /**
         * STRONA
         */
        public function indexAction()
        {
            $this->request = Request::createFromGlobals();
     
            #Request - create new app
            if ($this->request->get('action') == 'app_add') {
                //create data file from input
                $zipFileInputName = 'fileToUpload';
     
                //try add file
                $AddAppRequest = new AddAppRequest($zipFileInputName);
                $this->errors = $AddAppRequest->getErrors();
                $this->feedback = $AddAppRequest->getFeedback();
            }
            //widok
            $this->render('test/index.twig', ['errors' => $this->errors, 'feedback' => $this->feedback]);
        }
    }

    Brak namespace

  10. namespace App\Requests;

    Musi być jedna linijka odstępu pomiędzy <? a namespacem

  11. Poza tym tak jak wyżej - errory przechowywane jako właściwość obiektu, tworzenie Requesta przez createFromGlobals

  12. public function moveFile($dir = ROOT_ZIP_FOLDER)
        {
            if (empty($this->errors)) {
                //OK
                if ($this->request->files->get('fileToUpload')->move($dir, $this->zipFileName)) {
                    $this->feedback[] = 'Plik został przesłany do :' . $dir;
                    return true;
                } else {
                    $this->errors[] = 'Błąd podczas przenoszenia!';
                }
            } else {
                $this->errors[] = 'Plik nie został przesłany, ponieważ nie spełnił warunków!';
            }
        }

    Jeśli masz return true, to w innych przypadkach daj return false + określ typ zwracanej wartości jako bool

  13. //Modificators
        private $fileMaxSize = 23000000;//23MB
        private $maxCountChar = 30;
        //Zip File
        private $zipFile;
        private $fileName;
        private $fileSize;
        //Errors after validation
        private $errors = [];

    Komentarze są zbędne

  14. //sprawdź walidacje

    IMO komentarz trochę bez sensu, poza tym po angielsku

  15.  Następnym razem przy tak długim kodzie wstaw ten kod na githuba

komentarz 11 lipca 2018 przez sapero Gaduła (4,100 p.)
Dziękuję za odpowiedź, myślałem że będzie gorzej:) jak na początkującego programistę ten kod ma w ogóle jakiś poziom? czy źle to zaprojektowałem? są jakieś standardy i sposoby projektowania aplikacji czy to ma sens co napisałem czy kompletnie źle? czy trzymam się zasad SOLID tutaj?
komentarz 11 lipca 2018 przez Mariusz08 Maniak (62,300 p.)

Ten kod ma poziom :) Co do standardów, na pewno przyjrzyj się standardowi PSR-2 oraz innym standardom z rodziny PSR (Ciebie będą interesowały PSR-4 i PSR-1, reszta to standardy o np. budowie Containerów, których raczej nie będziesz tworzył ;) ), stosuje się Yoda Conditions oraz różne wzorce projektowe(Builder, Factory, DependencyInjection itp. - uważaj na Singletona)

komentarz 11 lipca 2018 przez sapero Gaduła (4,100 p.)
Dziękuje! kamień spad mi z serca:D poczytam o Yoda Conditions, o PSR wiem troche mam CS do phpstorm to mi zaznacza na czerwono:)
komentarz 11 lipca 2018 przez Mariusz08 Maniak (62,300 p.)
Yoda Conditions są nawet proste, ale musisz ogarnąć implementację wzorców projektowych, kiedy ich używać i jak - żadna wtyczka Ci tego nie podpowie.

Podobne pytania

+1 głos
4 odpowiedzi 685 wizyt
pytanie zadane 2 sierpnia 2016 w JavaScript przez Mawii0 Nowicjusz (170 p.)
+1 głos
0 odpowiedzi 263 wizyt
pytanie zadane 9 kwietnia 2021 w PHP przez Lopus Początkujący (360 p.)
0 głosów
0 odpowiedzi 349 wizyt
pytanie zadane 30 maja 2020 w PHP przez Mr. PanKrok Nowicjusz (230 p.)

92,576 zapytań

141,426 odpowiedzi

319,652 komentarzy

61,961 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Kolejna edycja największej imprezy hakerskiej w Polsce, czyli Mega Sekurak Hacking Party odbędzie się już 20 maja 2024r. Z tej okazji mamy dla Was kod: pasjamshp - jeżeli wpiszecie go w koszyku, to wówczas otrzymacie 40% zniżki na bilet w wersji standard!

Więcej informacji na temat imprezy znajdziecie tutaj. Dziękujemy ekipie Sekuraka za taką fajną zniżkę dla wszystkich Pasjonatów!

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...