Zadanie rekrutacyjne - czy takie rozwiązanie będzie poprawne?

0

Cześć!
Biorę udział w rekrutacji na stanowisko PHP dewelopera. Zależy mi na tej pracy, ponieważ bardzo chce się rozwijać w kierunku PHP i najchętniej robiłbym to w pracy.
Napisałem jedno z dwóch zadań rekrutacyjnych - jutro wieczorem je oddaje, jednak zanim oddam chciałbym zasięgnąć języka i zapytać was czy taki sposób jakim rozwiązałem to zadanie będzie dobrze punktowany?
Pytam was, ponieważ nigdy nie miałem okazji pracować w zespole i znam wiele technik, ale nikt mi jeszcze nigdy nie powiedział, które są prawidłowe i których powinno się używać.
Treść zadania, pliki źródłowe i moje rozwiązanie zamieściłem w archiwum w załączniku.
Czekam na wasze opinie!

<?php
function findCategory($id, $list){
  foreach($list as $element){
    if($element['category_id'] == $id)
      return $element;
  }
}

function getNameFromElementInfo($translations){
  if(@!$lang = $translations["pl_P"])
    if(@!$lang = reset($translations))
      return null;

  return $lang["name"];
}

function setName($array, $list){
  foreach($array as $id=>$element){
    $categoryInfo = findCategory($element["id"], $list);
    $element['name'] = getNameFromElementInfo($categoryInfo["translations"]);
    if($childrens = $element['children']){
      $element['children'] = setName($childrens, $list);
    }
    $array[$id] = $element;
  }

  return $array;
}

function render($tree){
  foreach($tree as $el){
    $id = $el["id"];
    if(@!$name = $el["name"])
      $name = null;
    echo "$id:$name<br/>"; 
    if($childs = $el['children'])
      render($childs);
  }
}


  $treePath = './tree.json';
  $listPath = './list.json';

  if(!file_exists($treePath))
    return print("can't find file '".$treePath."'");
  if(!file_exists($listPath))
    return print("can't find file '".$listPath."'");

  $treeJson = file_get_contents($treePath);
  $tree = json_decode($treeJson,true);

  $listJson = file_get_contents($listPath);
  $list = json_decode($listJson,true);

  $treeWithNames = setName($tree, $list);
  
  render($treeWithNames);
  echo "<hr/>";
  render($tree);
?>
0

co znaczy
return print("can't find file '".$treePath."'");

0
Miang napisał(a):

co znaczy
return print("can't find file '".$treePath."'");

zwróć napisz nie można znaleźć pliku ścieżka;
Nie zadawaj głupich pytań tylko powiedz śmiało, że to jest źle - tylko uzasadnij proszę.

2
Miang napisał(a):

co znaczy
return print("can't find file '".$treePath."'");

Poprawne to jest w sumie, print zwraca zawsze 1^^

2

Nie ma głupich pytań są tylko głupie odpowiedzi (MSPANC)
https://www.php.net/manual/en/function.print.php

0
Miang napisał(a):

Nie ma głupich pytań są tylko ŋgłu[pie odpowiedzi (MSPANC)
https://www.php.net/manual/en/function.print.php

możesz rozwinąć?
Domyślam się, że skoro wystąpił błąd to powinienem zwrócić 0, tak? tylko czemu?

1

Nie używaj @! Nigdy!

0

nikt nie zwrócił uwagi na to, że jest to napisane strukturalnie. Czy ktoś czytał treść zadania? Ja nie wiem, ale sądziłem, że przyjdzie tu ktoś, kto odbył wiele takich rozmów i zadań rekrutacyjnych i powie mi np. że wszystkie zadania trzeba robić obiektowo.

1

Bo nikomu się nie chce jakiegoś nieznanego zipa ściągać.

0
serek napisał(a):

Bo nikomu się nie chce jakiegoś nieznanego zipa ściągać.

Treść zadania:

The exercise is to write a program which is adding name property into each leaf of given tree
structure from tree.json file with name from list.json file. You should correlate
structures through category_id from list.json and Id in tree.json.
The output should be similar to:
[

{“id”:19”, “name”:”Zdrowie i uroda”,”children”:[]},
...
]
Things to consider in your implementation

  • Tree nesting level - in some cases nesting level could be really deep
  • Testability
    Deliverables:
  • Code (github/gitlab repository)
  • README.md file containing instructions so that we know how to build and run your
    code
1

Na pierwszy rzut oka to napisz to obiektowo (tylko pamiętaj o SOLID). Ktoś kto będzie to sprawdzał na pewno zwróci na to uwagę.

Kolejna sprawa, to nie zakładaj, że dane które otrzymujesz są prawidłowe. Np gdy robisz json_decode, gdy plik tree.json będzie pusty, program się wywali.

Robisz dwa file_exist z tym samym komunikatem. Możesz napisać osobną funkcję np validatefile($filepath) która rzuca wyjątkami.

Dopisz testy jednostkowe.

Dodaj typowanie w funkchach z PHP 7

Powodzenia :)

0
m4niek20 napisał(a):
if($childrens = $element['children']){

poprawna forma to children, nie childrens.

if(@!$lang = $translations["pl_P"])

Literówka. Powinno być pl_PL. Co jest niepokojące, skoro uszła twojej uwadze taka literówka, to wygląda tak, jakbyś coś napisał, a nie sprawdził nawet, czy to działa.

function render($tree){

co robi funkcja render? Generujesz ręcznie JSONa? A czemu nie wygenerujesz danych w pamięci i nie użyjesz funkcji json_encode, żeby wygenerował JSONa?

Ja nie wiem, ale sądziłem, że przyjdzie tu ktoś, kto odbył wiele takich rozmów i zadań rekrutacyjnych i powie mi np. że wszystkie zadania trzeba robić obiektowo.

Ja tam nie wiem, co wymagają w PHP i jakie są tam zwyczaje, bo nie siedzę w tym języku, ale w sumie co tu chcesz obiektowo zrobić? Przecież to jest program wielkości HelloWorlda.

1

Ogólnie słabe to...

function findCategory($id, $list){
  foreach($list as $element){
    if($element['category_id'] == $id)
      return $element;
  }
}

Brakuje return null, jeśli nic nie znajdzie.

  1. Używasz @, a tak się pisało z 10 lat temu może.
  2. Mamy teraz PHP 7.4, więc wypadałoby by kod był w nim napisany.
  3. Powinieneś używać === zamiast ==, jeśli jest taka możliwość.
  4. return print("can't find file '".$treePath."'"); to jakiś koszmarek.
  5. Raz używasz klamr, a raz nie.
if($childrens = $element['children']){
      $element['children'] = setName($childrens, $list);
}

WTF?
8) Nie rób jakichś przypisań w ifach. W ifach powinny być warunki!
9) ...

Ja bym nie ocenił tego pozytywnie.

2
m4niek20 napisał(a):
Miang napisał(a):

co znaczy
return print("can't find file '".$treePath."'");

zwróć napisz nie można znaleźć pliku ścieżka;

Chodzi o to, czy to jest celowe.
Chcesz żeby skrypt w tym momencie wykonał print dla zadanych parametrów i jednocześnie wyskoczył z funkcji, zwracając 1?

Jeśli nawet faktycznie tak, to można to zapisać trochę czytelniej:

return print ("can't find file '$treePath'");

albo nawet:

return print "can't find file '$treePath'";

Ale koncepcyjnie to i tak dziwne.

Opuszczanie podmiotu w angielskim to raczej mowa nieformalna.
Ain't right bro?

0
  1. Wrażenia ogólne:
  • korzystaj z typehintów dla argumentów i dla wartości zwracanych np.

function findCategory($id, $list)
zamieni się w
function findCategory(int $id, array $list): array
(bo $element to array)

  • nie korzystaj z @ nigdy
  • w przypadku instrukcji warunkowych korzystaj z klamerek np.
  • w przypadku instrukcji warunkowych NIGDY nie korzystaj z jednego =!!!
  • po nazwie funkcji wrzucaj sobie klamerkę na wolną linię, bo ifie czy foreachu (o ile są) to na tej samej linii, ale po spacji
  • doc komentarze w przypadkach nietrywialnych są bardzo pomocne
  1. Szczegółowo:
  • findCategory mogłoby mieć parametr o nazwie $categoryId i jak obsługujesz to, gdy jej nie znajdzie? Czy możesz robić sprawdzanie ===?
  • getNameFromElementInfo - czym jest translations? Czemu nie naprawisz tych ifów i nie zrobisz go jednym przy pomocy &&? Nie może tego $lang jakoś bardziej po ludzku ogarnąć?
  • setName - czy na pewno chcesz taką watpliwą rekurencję nazywać "setName"? Ta funkcja jest koszmarna, chociaż ma tylko 10 linii...
  • render - od biedy ujdzie
  • co to jest return print... poza funkcją?

Generalnie polecam trochę się poedukować w temacie OOP, PHP 7+ i wtedy Twój kod (pal licho jakiekolwiek frameworki na tym etapie) wyglądałby dużo lepiej. Powodzenia :)

0

Poprawiłem te błędy, które dało się w miarę szybko poprawić. Jestem zdania, że warto spróbować to napisać obiektowo, ale na razie mam drugie zadanie do zrobienia i jak zostanie mi czasu to będę próbował obiektowo. Odrobinę ulepszony kod:

<?php
function findCategory(int $id, array $list)
{
  foreach($list as $element) {
    if(intval($element['category_id']) === $id) {
      return $element;
    }
  }
  return null;
}

function getNameFromElementInfo($translations)
{
  if($translations) {
    if($translations["pl_PL"]) {
      $lang = $translations["pl_PL"];
    } else {
      $lang = current($translations); // get the first element of an array
    } 
    return $lang["name"];
  } else {
    return false;
  }
}

function setName(array $tree, array $list)
{
  foreach($tree as $id=>$el) {

    $categoryInfo = findCategory($el["id"], $list);
    $el['name'] = getNameFromElementInfo($categoryInfo["translations"]);
    $el['children'] = setName( $el['children'], $list);
    $tree[$id] = $el;

  }

  return $tree;
}

function render($tree)
{
  foreach($tree as $el) {
    $id = $el["id"];
    $name = null;
    if(isset($el["name"])) {
      $name = $el["name"];
    }

    echo "$id:$name<br/>"; 

    if($el['children'] ) {
      render($el['children']);
    }
  }
}

function getFile(String $path)
{
  if(file_exists($path)) {
    $file = file_get_contents($path);
    if(json_decode($file,true)) {
      return json_decode($file,true);
    } else {
      throw new Exception("an error occurred while reading the file.");
    }
  } else {
    throw new Exception("File '$path' not found.");
  }

}


function main()
{

  $tree = getFile('./tree.json');
  $list = getFile('./list.json');

  $treeWithNames = setName($tree, $list);
  
  render($treeWithNames);
  echo "<hr/>";
  render($tree);

}
  
main();
?>
0
  1. Jeśli findCategory zwróci null, to się kod wysypie. Musisz to jakoś obsłużyć dalej:
    a) Możesz np. pominąć obliczenia w danej iteracji pętli za pomocą continue, jeśli kategoria nie została znaleziona.
    b) Możesz zamiast zwracać null rzucić wyjątek.

  2. Formatowanie nadal jest dziwne, raz dodajesz pustą linię, innym razem nie.

if(..) {
    ...
    return $lang["name"];
} else {
    return false;
}

Możesz zmienić na:

if(..) {
    ...
    return $lang["name"];
}

return false;

Podobnie z użyciem throw.

  1. Ogólnie poczytaj sobie o konwencji przy formatowaniu kodu - kiedy i jak używać spacji, kiedy enterów, klamr, itp.

  2. Poczytaj sobie o PSR.

  3. Na plus byłoby zaimplementowanie jakiegoś silnika szablonów, żeby nie łączyć htmla z php. Np. Smarty.

  4. getFile nic nie zwraca, jeśli plik nie istnieje. Powinien być null, (lub pusta tablica) albo rzucenie wyjątku.

1

To zobacz co się stanie, jeśli doprowadzisz do sytuacji, gdy findCategory zwróci null:

  foreach($tree as $id=>$el) {
    $categoryInfo = findCategory($el["id"], $list);
    $el['name'] = getNameFromElementInfo($categoryInfo["translations"]);
    $el['children'] = setName( $el['children'], $list);
    $tree[$id] = $el;
  }
  1. $categoryInfo przyjmuje null.
  2. $categoryInfo["translations"] nie istnieje, bo null nie jest tablicą.
0

no rzeczywiście dziwne ale działa z tym nullem. no nic, zrobiłem to tak:

function setName(array $tree, array $list)
{
  foreach($tree as $id=>$el) {
    $details = findCategory($el["id"], $list);
    if($details){
      $el['name'] = getNameFromElementInfo($details["translations"]);
    }
    if(count($el['children'])){
      $el['children'] = setName( $el['children'], $list);
    }
    $tree[$id] = $el;
  }
  return $tree;
}
2

Ogólnie przed rozpoczęciem implementacji musisz sobie odpowiedzieć na najważniejsze pytanie:
w opisie jest:

The exercise is to write a program which is adding name property into each leaf of given tree...

potem patrzysz na wikipedię:

Similarly, an external node (also known as an outer node, leaf node, or terminal node) is** any node that does not have child nodes.**

i wtedy pojawia się najważniejsze pytanie, które trzeba zadać przed wykonaniem tego - co autor miał na myśli?
czy:
a) chodziło o each **node **of given tree
b) czy rzeczywiście trzeba wypełnić tylko te które są liśćmi

Jeżeli:
a) to być może w tym zadaniu wcale nie chodzi o wykonanie tego tylko właśnie o to żeby zadać to pytanie.
b) to masz źle zrobioną funkcjonalność i wtedy nie ma znaczenia czy jest SOLID, KISS, DRY, KFC czy NBA.

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