Zadanie rekrutacyjne - co można zrobić lepiej?

1

Cześć,
ostatnio wrzuciłem swój projekt z zadania rekrutacyjnego i wiele się nauczyłem dzięki samym odpowiedziom. Dlatego wrzucam gotowe zadanie z innej rekrutacji, jestem bardzo ciekaw co mógłbym zrobić lepiej. Ogólnie jestem zadowolony z tego co zrobiłem. Ciekaw jestem waszej opinii.

Treść zadania:
Proszę o przygotowanie skryptu linii poleceń (Shell), którego zadaniem będzie konwersja plików JSON na CSV i
CSV na JSON.
Wymagania:

  1. Nazwa skryptu konwertującego: convert.php
  2. Ścieżka do konwertowanego pliku powinna być przekazywana jako pierwszy argument skryptu:
    ./convert.php file.csv
    ./convert.php file.json
  3. Skonwertowane dane należy wyświetlić na standardowy strumień wyjścia (stdout).
  4. Komunikaty błędów należy wyświetlać na standardowym strumieniu błędów (stderr).
  5. Logikę konwertującą należy zaprogramować w klasie Converter.
  6. Separatorem kolumn w plikach CSV może być przecinek lub średnik. Klasa konwertująca musi wykryć
    zastosowany separator.
    Informacje dodatkowe
  7. Do zadania dołączone są dwa pliki CSV oraz jeden plik JSON.
  8. Pliki CSV zakodowane są w UTF-8.
  9. Pierwszy wiersz pliku CSV to nagłówek z nazwami kolumn.
  10. Proszę wykonać zadanie w PHP 7.3 lub 7.4.

Kod:

<?php
class Converter {

    private $path;
    private $data;
    private $convertedData = "";
    private $extension;
    private $convertedExt;
    private $allowedExt = ['json','csv'];

    function __construct($argv)
    {
        if(count($argv) > 1){
            $this->path = $argv[1];
        } else {
?>
This is a command line PHP script with one option.

Usage:
<?php echo $argv[0]; ?> <path>

<path> Path to file you would
like to convert - may be the json 
or csv file. 

<?php
        }
    }

    public function fileValidator($path)
    {
        if(file_exists($path)){
            $this->extension = pathinfo($path, PATHINFO_EXTENSION);
            if(in_array($this->extension, $this->allowedExt)){
                if(file_get_contents($path)){
                    $this->data = file_get_contents($path);
                    return 1;
                } else {
                    fwrite(STDERR, "the file is empty.");
                }
            } else {
                fwrite(STDERR, "File extension not allowed.");
            }
        } else {
            fwrite(STDERR, "file not found.");
        }
        return 0;
    }

    public function saveFile(){
        if( $this->convertedData ){
            $newFile =  pathinfo($this->path, PATHINFO_FILENAME)."-".$this->convertedExt.".".$this->convertedExt;
            file_put_contents($newFile, $this->convertedData);
            echo "\nThe data has been saved to a file named ".$newFile."\n";
        } else {
            fwrite(STDERR, "First use the convert function. the file was not saved.\n");
        }
    }

    public function json2csv()
    {
        if( json_decode($this->data, true) ){
            $jsonDecoded = json_decode($this->data, true);
            $csv = fopen('php://temp/maxmemory:'. (5*1024*1024), 'r+'); // max size of the csv file: 5MB
            
            // find all headers
            $headers = [];
            foreach($jsonDecoded as $row){
                $keysInRow = array_keys($row);
                foreach($keysInRow as $keyId=>$key){
                    if(!in_array($key,$headers)){ // check if there is no such an element in the array
                        if($keyId === 0){         // if this is first element   
                            $headers[] = $key;    // just add this element
                        } else {                  // else add the item in the right place
                            array_splice( $headers, $keyId--, 0, $key ); 
                        }
                    }
                }
            }

            // set the header line
            $lastHeader = count($headers); // count the headers and set iterator
            $i = 0;                       // just to avoid giving a comma in the last element
            foreach($headers as $header){
                $i++;
                $this->convertedData .= $header;
                if($i != $lastHeader){
                    $this->convertedData .= ',';
                }
            }
            $this->convertedData .= "\n";

            // convert data to csv
            foreach($jsonDecoded as $row){
                $i = 0;
                foreach($headers as $header){
                    $i++;
                    if(isset($row[$header])){ // if is set data for this header in this row
                        $this->convertedData .= $row[$header];
                    } 
                    if($i != $lastHeader){
                        $this->convertedData .= ",";
                    }
                }
                $this->convertedData .= "\n";
            }
            $this->convertedExt .= "csv";
        } else {
            fwrite(STDERR, "Wrong file structure.");
        }
    }

    public function csv2json()
    {
        $file = fopen($this->path, 'r');

        // determine the division sign
        if(substr_count($this->data, ',') > substr_count($this->data, ';')){
            $dist = ',';
        } else {
            $dist = ';';
        }

        // get headers
        $keys = fgetcsv($file,"1024",$dist);
        // eliminate the bom appearing at the beginning of the file
        $keys[0] = preg_replace('/^[\pZ\p{Cc}\x{feff}]+|[\pZ\p{Cc}\x{feff}]+$/ux','',$keys[0]); 
        $json = array();
        while ($row = fgetcsv($file,"1024",$dist)) { // set the each element from row
            $json[] = array_combine($keys, $row);   // key from keys array
        }
        
        // encode array to json
        $this->convertedData = json_encode($json, JSON_UNESCAPED_UNICODE ); // fix the utf-8 encoding
        $this->convertedExt .= "json";
    }

    public function convert()
    {
        if($this->fileValidator($this->path)){
            if($this->extension === "json"){
                echo "converting json to csv...\n";
                $this->json2csv();
                echo $this->convertedData;
            } else if( $this->extension === "csv"){
                echo "converting csv to json...\n";
                $this->csv2json();
                var_dump($this->convertedData);
            } else {
                fwrite(STDERR, "Something went wrong.");
            }
        } else {
            return 0;
        }
    }

}

$converter = new Converter($argv);
$converter->convert();
$converter->saveFile();

?>
0

Oczywiście, że można lepiej.
Zawsze można.

Do Ciebie się przyczepić może bardziej spostrzegawczy rekruter o łamanie SRP.

1

Spojrzałem sobie na json2csv i tak na szybko:
na samym początku podwójnie dekodujesz tego samego jsona, co przy dużych plikach jest 2/10.
Dane pokroju (510241024) możesz machnąć nawet do jakiejś stałej, żeby nie przeszukiwać potem kodu.
hedersy możesz skleić bez problemu za pomocą implode,
same hedersy mogą być wydzielone do osobnej metody,
nazwy zmiennych pokroju $i, $j idealnie pasują do jednolinijkowych iteracji, a nie do większych pętli. Przy czymś takim warto już nazywać zmienne poprawnie,
same rozszerzenia też mógłbyś wywalić do jakiejś stałej - trudniej popełnić błąd korzystając z nich potem,
W sumie to nawet do obsługi błędów możesz przygotować osobną metodę, żeby nie pisać za każdym razem stderr. Ułatwiłoby to w przyszłości zmianę strumienia błędów - zamiast przerabiać cały plik, przerabiasz tylko jedną metodę.
Z tego co widzę to powyższe można powiedzieć też do innych metod, które tu stworzyłeś.

1
public function fileValidator($path)
    {
        if(file_exists($path)){
            $this->extension = pathinfo($path, PATHINFO_EXTENSION);
            if(in_array($this->extension, $this->allowedExt)){
                if(file_get_contents($path)){
                    $this->data = file_get_contents($path);
                    return 1;
                } else {
                    fwrite(STDERR, "the file is empty.");
                }
            } else {
                fwrite(STDERR, "File extension not allowed.");
            }
        } else {
            fwrite(STDERR, "file not found.");
        }
        return 0;
    }
  1. Zwracaj true lub false
  2. Zamień te wszystkie fwrite na rzucanie wyjątku. Potem w catch złapiesz ten wyjątek. Dlaczego? Ponieważ gdy będziesz chciał zmienić obsługę błędów (np. zamiast używać fwrite będzie chciał użyj echo lub print) będziesz to musiał robić w kilkunastu miejscach (!a to tylko jedna klasa!). Gdy zrobisz to za pomocą rzucania wyjątków w jednej linii będziesz mógł zmienić sposób obsługi błędów. Będzie to też wyglądało dużo estetyczniej.
  3. Robisz komunikaty błędu typu "plik nie znaleziono", :rozszerzenie pliku nie dozwolone" itp. ale nie dajesz info np jaki to plik, jaka ścieżka, jakie rozszerzenie.
  4. Gdzie typowanie z php7?

    public function json2csv()
    {
        if( json_decode($this->data, true) ){
            $jsonDecoded = json_decode($this->data, true);
            $csv = fopen('php://temp/maxmemory:'. (5*1024*1024), 'r+'); // max size of the csv file: 5MB

            // find all headers
            $headers = [];
            foreach($jsonDecoded as $row){
                $keysInRow = array_keys($row);
                foreach($keysInRow as $keyId=>$key){
                    if(!in_array($key,$headers)){ // check if there is no such an element in the array
                        if($keyId === 0){         // if this is first element   
                            $headers[] = $key;    // just add this element
                        } else {                  // else add the item in the right place
                            array_splice( $headers, $keyId--, 0, $key ); 
                        }
                    }
                }
            }

            // set the header line
            $lastHeader = count($headers); // count the headers and set iterator
            $i = 0;                       // just to avoid giving a comma in the last element
            foreach($headers as $header){
                $i++;
                $this->convertedData .= $header;
                if($i != $lastHeader){
                    $this->convertedData .= ',';
                }
            }
            $this->convertedData .= "\n";

            // convert data to csv
            foreach($jsonDecoded as $row){
                $i = 0;
                foreach($headers as $header){
                    $i++;
                    if(isset($row[$header])){ // if is set data for this header in this row
                        $this->convertedData .= $row[$header];
                    } 
                    if($i != $lastHeader){
                        $this->convertedData .= ",";
                    }
                }
                $this->convertedData .= "\n";
            }
            $this->convertedExt .= "csv";
        } else {
            fwrite(STDERR, "Wrong file structure.");
        }
    }
  1. Zmień tą nazwę funkcji w stylu 3maj się na normalną.
  2. Rzucaj wyjątkami, to samo co w poprzedniej funkcji
  3. Dodałeś komentarze do funkcji dzieląc jest na fragmenty kodu odpowiedzialne za coś. Podziel to na funkcje. Jeśli Twoja funkcja ma więcej niż 10 linii kodu to wiedz że coś się dzieje :D
  4. Dwa razy json_decode możesz wykonać raz, przypisując to do zmiennej

Te same punkty do funkcji csv2json

Fajnie że piszesz już obiektowo, wygląda to lepiej niż poprzednio, ale przed Tobą sporo pracy. 3mam kciuki :)

0

Nim się spojrzy na kod to najważniejsze co trzeba zrobić, to sprawdzić czy program działa prawidłowo, bo nawet jeśli w kodzie wszystkie zmienne nazywasz $a1, $a2, $a3 , a program działa to jego wartość jest 100 razy większa niż takiego który nie działa, a ma ładnie nazwane zmienne.

W tym zadaniu zacząłbym od sprawdzenia czy program spełnia relację przechodniości
czyli biorę plik:
test1.csv -> test1.json
test1.json -> test2.csv
i sprawdzam czy test1.csv == test2.csv

aby to zrobić zapisuję sobie w excelu plik (jak na zdjęciu):
screenshot-20200825111916.png

Wynikowy plik (test1.csv):

Kategoria,Opis,Cena
Używane,"Sprzedam, mało chodzone ale dobre, może nosić ślady użytkowania, ale nie musi","1354,34"

i sprawdzam czy po przekonwertowniu test2.csv zawiera to samo co plik test1.csv
Jeżeli nie, to olewam zmienne, stałe, SOLID, KISS, SMR i inne tego typu rzeczy i poprawiam funkcjonalność.

Sprawdź, proszę czy Twój program zadziała dla mojego pliku test1.csv, i powiedz jakie Ty pliki testowe przepuściłeś przez ten skrypt?

0

Dziękuje wszystkim za uwagi - są dla mnie bardzo cenne.
Jestem już po rozmowie rekrutacyjnej, więc opowiem jakie były uwagi rekrutera do tego zadania.

  1. W linii 64 została zmienna $csv, która jest nieużywana - została po próbie użycia innej metody rozwiązania tego problemu.
  2. W funkcji csv2json na początku gdy otwieram plik csv podaje parametr "r". Podobno lepiej dodać jeszcze parametr "b", czyli "rb". Nie wiem czemu, może ktoś wie.
  3. Sprawdzanie w pliku CSV jaki znak jest znakiem podziału można było zrobić lepiej. Ja liczę wszystkie ";" i "," i sprawdzam czego jest więcej. Rekruter robi to w ten sposób, że próbuje odczytać plik z ";" i sprawdza długość otrzymanej wartości, jeśli wychodzi jednoelementowa tablica to znaczy, że jednak znakiem podziału jest ",".
  4. Błąd gdy nie podamy nazwy pliku lub plik jest pusty - wykona się funkcja saveFIle() - dziwne żeby nie :D błąd w linii 161

Poza powyższymi wytknięciami błędów, o które bardzo rekrutera prosiłem, zadanie jest zrobione dobrze.
Bardzo podobały się komentarze, nie dałem się nabrać na dwie pułapki przygotowane w tym zadaniu, czyli w linii 127 usunięcie BOM czyli znacznika kodowania pliku, oraz na drugą pułapkę, która polegała na tym, że gdy użyjemy zwykłego(funkcja dostępna w php) konwertowania json na csv z pliku json, w którym niektóre obiekty nie mają pewnych wartości, to w pliku csv już nagłówki nie pasują do określonych wartości - żeby to rozwiązać nie używałem wbudowanych funkcji, tylko sam zebrałem z pliku wszystkie nagłówki i sam wygenerowałem plik CSV.

4

Szczerze mówiąc ten kod by nie przeszedł CR w żadnej firmie. Pomijam kwestie działania/nie działania, bo to już koledzy wyżej analizowali, ale już na poziomie kodu jest straszne spaghetti.
Takie rzeczy na które musisz zwrócić uwagę na przyszłość:

  • PSR - to jest must have w każdym komercyjnym kodzie
  • miałeś wyraźnie napisane w zadaniu aby użyć PHP 7.3/7.4 - nie widzę tutaj elementów typu typowanie parametrów funkcji/typowania wartości zwracanych przez funkcje. W PHP 7.4 możesz nawet typować zmienne klasowe. Dlaczego z tego nie korzystasz?
  • ZAWSZE używaj w kodzie strict_types - dzięki temu masz o wiele większą kontrolę nad typami danych i szybciej pojawi się błąd (to jest wbrew pozorom dobre, zgodnie z zasadą fail early).
  • echo w konstruktorze?
  • dlaczego nie używać const dla rzeczy, które się nie zmieniają np. $allowedExt, czy magic numbers w kodzie?
  • dlaczego skracasz zmienne typu $allowedExt? Jak to mi kiedyś powiedział kolega robiący mi CR - jak Ci brakuje literek to pożyczę Ci moje ;p A tak serio jak czytasz długi kod to nie masz ochoty się zastanawiać co autor miał na myśli. Im bardziej kod jest jednoznaczny tym lepiej. Długie zmienne też mają to do siebie, że łatwiej je się wyszukuje - np. jak pamiętasz, że zmienna miała w nazwie Extensions to dostaniesz mniej wyników niż dla Ext, gdzie będą też np. External, Extra itp.
  • komentarze w kodzie? Jeśli musisz użyć komentarza to na 99% coś powinno być wyekstrahowane do osobnej funkcji - kod musi się dokumentować sam. W normalnym, komercyjnym kodzie prawie nigdy nie zobaczysz takich komentarzy
  • czemu twoje funkcje są taaaakie długie. Większość z nich spokojnie można podzielić na 2-3 mniejsze bo robią po prostu za dużo
  • NIGDY nie zagnieżdżaj if''ów, pętli itp. To jest typowy kandydat do przerobienia na funkcję
  • NIGDY nie używaj else, a już NIGDY NIGDY nie używaj if else if else - czegoś takiego nie da się czytać. W małym skrypcie to się wydaje niewinne, ale jak będziesz musiał przeczytać tysiąc linii takiego kodu aby znaleźć buga to będziesz wyrywał włosy z głowy
  • Dlaczego wszystko jest w jednej klasie? Masz różne typu konwerterów to powinieneś użyć polimorfizmu. Co się stanie jak dojdzie kolejny konwerter np. z XML? Dodasz kolejnego IF? Powinien być jakiś interfejs z funkcją convert() i kilka konwerterów, które go implementują
  • Dlaczego klasa ma same metody publiczne? Funkcja publiczna to jest dług technologiczny, bo musisz je później utrzymywać i do tego jasny sygnał że najprawdopodobniej gwałcisz SRP. Co śmieszne nawet połowy z nich nie używasz w kodzie. Funkcje powinny być private by default, za każdym razem jak zmieniasz na protected albo public to Twoim obowiązkiem jest wyjaśnić czemu to robisz i co ważne nie może to być "bo kiedyś się może przydać" - tutaj rządzi zasada YAGNI - kod piszesz dopiero jak faktycznie jest przypadek wymagający jego użycia. Nie używasz czegoś na zewnątrz klasy to zrób jako private. Będzie kiedyś potrzeba zmiany na protected to zmienisz. W większości przypadków finalnie okaże się, że takie przypadku nie będzie.

Nie pisze tego aby Cię zrażać, bo rozumiem, że szukasz pierwszej pracy i każdy taki kod pisał kiedyś. Ale ogólnie to co musisz zrozumieć to treścią takich zadań jak to nie jest sprawdzenie czy umiesz wykonać samo zadanie tylko właśnie sprawdzenie jak pracujesz z kodem. Mój przełożony kiedyś powiedział, że działający kod to jest taki początek - jak nie umiesz napisać czegoś co działa to nie będziesz developerem. Ale dopiero to w jaki sposób to napiszesz robi z Ciebie specjalistę. Mały kod może być brzydki, ale im jest on większy tym bardziej mają znaczenie rzeczy takie jak opisałem bo inaczej szybko dojdziesz do granicy, gdzie kodu nie da się dalej rozwijać.

Powodzenia!

1 użytkowników online, w tym zalogowanych: 0, gości: 1