Zadanie rekrutacyjne

Odpowiedz Nowy wątek
2018-08-15 10:59

Rejestracja: 5 lat temu

Ostatnio: 1 godzina temu

Lokalizacja: Nowa Ruda

4

Witam, ostatnio aplikowałem na jedna pracę zdalną. Dostałem zadanie do zrobienia (treść w załączniku). Niestety zostało ocenione negatywnie (nie spełnia oczekiwań,). Prosiłbym o takie code review. Co poprawić, co wyrzucić, czego nie stosować, jakby ktoś miał chwilę zerknąć.
https://github.com/szydlaczek/Company

Pokaż pozostałe 4 komentarze
Nie licząc studiów 1.5 roku. - szydlak 2018-08-15 16:23
Zadanie na staż, czy na juniora? - Aisekai 2018-08-15 17:32
Zdalnie to chyba nie na Juniora. - ._. 2018-08-15 17:33
Kompletnie, z jakiegoś powodu, zignorowałem słówko "zdalnie". Dzięki :) - Aisekai 2018-08-15 18:39
stanowisko regular? - feni000 2018-08-16 19:13

Pozostało 580 znaków

2018-08-15 12:22

Rejestracja: 4 lata temu

Ostatnio: 13 godzin temu

0

Aplikacja powinna być odporna na przesłanie brakujących lub nieprawidłowych danych w
zapytaniach i odpowiadać odpowiednim statusem HTTP wraz z informacją o powodzie błędu.

Twoje API zawsze zwraca odpowiedź ze statusem 200?

No raczej nie. Testowałem i miałem 400 w przypadku nieprawidłowych danych. - szydlak 2018-08-15 13:06

Pozostało 580 znaków

2018-08-15 13:00

Rejestracja: 3 lata temu

Ostatnio: 10 godzin temu

1

Imo fajnie by było zapytać co im nie pasuje, pokazać ze faktycznie zależy ci na pracy i że chcesz się uczyć czy dalej rozwijać.

Wiem. Ale byłem na urlopie i praktycznie nie miałem zasięgu żeby pisać do nich. Generalnie to napisali, że mam potencjał ale małe doświadczenie i zostawią sobie kontakt na przyszłość. A pisała do mnie osoba nie techniczna i przekazywała zadanie osobom technicznym, więc nie wiem czy będzie łatwo uzyskać info co jest nie tak. - szydlak 2018-08-15 13:10

Pozostało 580 znaków

2018-08-15 13:09

Rejestracja: 16 lat temu

Ostatnio: 1 godzina temu

Lokalizacja: Kraków

4

Cóż, na początek rzuca się mały chaos w rozmieszczeniu plików w solucji, i brak konsekwencji w zapisie kodu, zignorowałeś guidline odnośnie zapisu kodu, a ten wymagany gudiline i tak jest wyjątkowo krótki.
Wyjątki są dla sytuacji wyjątkowych a już w szczególności użycie ich do sterowania przepływem logiki w aplikacji śmierdzi, w szczególności gdy w webapi masz dostępne Model Validation.
Użyłeś kontenera Ioc i automappera, które w tak prostej aplikacji są kompletnie niepotrzebne, znacznie bardziej by się przydało logowanie błędów czy chociażby Fluent Validation.
Robisz jakieś dziwne rzeczy:

 .ForMember(dest => dest.FirstName, s => s.MapFrom(em => em.FirstName))
.Select(pi => pi)
if (UserName.Equals(user) && Password.Equals(password)) // to chyb naleciałość z javy ?

które raczej świadczą o tym że nie wiesz jak te rzeczy działają.

A najbardziej to mnie przestraszyło to:

 private Func<Company, bool> GetFilterCondition(SearchCriteria searchCriteria)
        {
            List<JobTitle> jobs = Jobs.GetJobsList(searchCriteria.EmployeeJobTitles);
            Expression<Func<Company, bool>> exp = d => true;

            if (searchCriteria.Keyword.Length > 0)
            {
                exp = d => d.Name.Contains(searchCriteria.Keyword) ||
                d.Employees.Any(e => e.FirstName.Contains(searchCriteria.Keyword) ||
                e.LastName.Contains(searchCriteria.Keyword));
            }
            Func<Company, bool> keywordExp = exp.Compile();
            if (jobs.Count > 0)
                exp = i => keywordExp(i) && i.Employees.Any(e => jobs.Contains(e.JobTitle));
            Func<Company, bool> compiled = exp?.Compile();
            if (searchCriteria.EmployeeDateOfBirthFrom.Length > 0)
            {
                exp = i => compiled(i) && i.Employees.Any(e => e.DateOfBirth >= Converters.GetDateTime(searchCriteria.EmployeeDateOfBirthFrom));
            }
            Func<Company, bool> com = exp.Compile();
            if (searchCriteria.EmployeeDateOfBirthTo.Length > 0)
                exp = i => com(i) && i.Employees.Any(e => e.DateOfBirth <= Converters.GetDateTime(searchCriteria.EmployeeDateOfBirthTo));
            return exp.Compile();
        }

to jest jakiś smok, którego się boję i którego tam nie powinno być i którego utrzymanie będzie masakrą.

A tak pzt to nie masz testów.

Ogólnie kwiatków jest mnóstwo (użycie refleksji, brak async, nazywanie zmiennych, itd), a wystarczyło wziąć jakieś przykładowe webapi i je przerobić, to strasznie przekombinowałeś prostą sprawę...


It's easy to hate code you didn't write, without an understanding of the context in which it was written.
edytowany 1x, ostatnio: neves, 2018-08-15 13:11
Dzięki wielkie za odpowiedź. A masz przykład takiego poprawnego Api? Tak mi się wydawało, że przekombinowałęm. Jak rozwiązać sprawę tych kryteriów wyszukiwania ? - szydlak 2018-08-15 13:16
@neves: z tą naleciałością chodzi Ci o Equals zamiast == ? Z ciekawości pytam, bo wcześniej też pisałem tylko Java a po przerzuceniu na .NET zdarza mi się używać Equals do porównywania stringów. - lukaszek016 2018-08-15 14:08
@lukaszek016 dokładnie o to - neves 2018-08-15 16:03

Pozostało 580 znaków

2018-08-15 14:18
Moderator

Rejestracja: 16 lat temu

Ostatnio: 1 godzina temu

C# to nie moja działka ale kilka ogólnych uwag:

  1. https://github.com/szydlaczek/Company/blob/master/Converters.cs raz że to powinien być jakiś DateConverter jeśli w ogóle, a dwa ze rzucenie gołego Exception to słaby pomysł. Jak już musisz rzucać, to jakimś wyjątkiem który mówi co się stało. Teraz próba złapania i obsłuzenia tej sytuacji złapie przy okazji milion innych rzeczy
  2. https://github.com/szydlaczek[...]ob/master/EnumJobExtension.cs ktoś wyżej wspomniał -> sterowanie przepływem za pomocą wyjątków to zło. Wyjątki są jak dzieje się coś wyjątkowego a nie jak nie umiesz zrobić if... albo zwrócić z funkcji jakiegoś Optional czy Either albo customowego NullObjecta (jak ten twój Job.None)
  3. https://github.com/szydlaczek[...]er/Services/CompanyService.cs -> klasyczny bląd, czyli upchniecie całej logiki aplikacji w jedną biedną klasę.

Masz problem? Pisz na forum, nie do mnie. Nie masz problemów? Kup komputer...

Pozostało 580 znaków

2018-08-15 14:30

Rejestracja: 3 lata temu

Ostatnio: 3 godziny temu

Lokalizacja: UK

1

Przejrzalem to powierzchownie i coz, przerost formy nad trescia. @neves dobrze to wypunktowal. Warto podpatrzyc repozytoria na GitHub, np:

https://github.com/jbogard/ContosoUniversityDotNetCore
https://github.com/dotnet-architecture/eShopOnWeb

Nie mowie aby do tak trywialnego problemu jak Twoj stosowac wszystkie praktyki z tych repozytoriow. Bardziej chodzi mi o to zebys zobaczyl jak podchodzi sie do pewnych problemow.


Na każdy złożony problem istnieje rozwiązanie które jest proste, szybkie i błędne.
edytowany 1x, ostatnio: Aventus, 2018-08-15 15:53

Pozostało 580 znaków

2018-08-15 15:29

Rejestracja: 5 lat temu

Ostatnio: 23 minuty temu

Lokalizacja: Rzeszów

1

No i stosujesz Func<cośtam> zamiast Expression<Func<cośtam>> w Entity Frameworku, przez co ładujesz całą tabelę do pamięci i dopiero wtedy filtrujesz, zamiast filtrować na bazie.

Pozostało 580 znaków

2018-08-15 16:00

Rejestracja: 3 lata temu

Ostatnio: 11 godzin temu

0
Shalom napisał(a):

C# to nie moja działka ale kilka ogólnych uwag:

  1. https://github.com/szydlaczek/Company/blob/master/Converters.cs dwa ze rzucenie gołego Exception to słaby pomysł. Jak już musisz rzucać, to jakimś wyjątkiem który mówi co się stało. Teraz próba złapania i obsłuzenia tej sytuacji złapie przy okazji milion innych rzeczy

No ok, czyli rezygnujemy z wyjątków i obsługi ich w https://github.com/szydlaczek[...]/GlobalExceptionMiddleware.cs,

to aby uniknąć robienia niesławnego logika w kontrolerze, która determinują czy ma polecieć np. status code 200, 401 czy cokolwiek innego, to docelowo chcemy zrobić coś takiego?

public class User
{
    public Guid Id { get; protected set; }
    private decimal _balance;
    public decimal getBalance => _balance;
    private static readonly object _lock = new object();

    public void Withdraw(decimal value)
    {
        lock (_lock)
        {
            if (value >= balance)
            {
                _balance -= value;
            }
        }
    }
}

public static class UserService
{
    public static ActionResult Perform_Withdraw_Logic(User user, decimal value)
    {
        if (value <= 0)
        {
            return new BadRequestResult("Amount of money to withdraw must be greater than 0");
        }
        if (value > user.getBalance)
        {
            return new BadRequestResult("You've not enough money");
        }

        user.Withdraw(value);

        return new OkRequestResult("Money has been withdrawn successfully");
    }
}

public class Controller
{
    public HomeController : Controller
    {
        [Authorize]
        public ActionResult WithdrawMoney([FromBody] WithdrawCommand)
        {
            var user = db.Single(c => c.Id == WithdrawCommand.Id);

            if (user == null)
            {
                return new BadRequestResult("User not found");
            }

            return UserService.Perform_Withdraw_Logic(user, WithdrawCommand.Amount);
        }
    }
}
edytowany 13x, ostatnio: WeiXiao, 2018-08-15 21:53
@somekind: czy da się to zrobić lepiej? - WeiXiao 2018-08-15 21:51
Żadnych ActionResult w serwisach. - somekind 2018-08-15 22:17
@somekind: Czyli jeszcze 1 "layer" pomiędzy kontrolerem, a serwisem? - WeiXiao 2018-08-15 22:18
Ale w jakim celu? Niech serwis zwraca jakiś swój obiekt wynikowy (u mnie np. wszystkie implementują jeden interfejs), a potem w warstwie web można go na odpowiedni result przetworzyć, np. w jakimś bazowym kontrolerze albo globalnym filtrze. - somekind 2018-08-16 00:11
@somekind: Że miałbym stworzyć sobie typ reprezentujący ServiceResult "respektowany" w całej aplikacji oraz jakiś converter np. ServiceResult To ActionResult? No bo aktualnie w w/w kodzie problemem jest to, że jakbym z innego miejsca w kodzie chciał skorzystać z tego kodu, to też dostałbym ActionResult, a tego raczej bym nie chciał gdziekolwiek poza kontrolerem. - WeiXiao 2018-08-16 00:21
Ogólnie tak, mi właśnie o to mi chodzi, żeby ActionResult nie wypuszczać poza warstwę web. - somekind 2018-08-16 00:48

Pozostało 580 znaków

._.
2018-08-15 17:29
._.

Rejestracja: 1 rok temu

Ostatnio: 1 rok temu

0

Z zadania:

oraz z możliwością zarządzania poprzez RESTful Web API

Niby jak w CRUD'ie masz zaimplementować REST FULL ?

Dane należy przechować w dwóch tabelach z 64-bitowym kluczem głównym (ID), połączonych ze
sobą relacją jeden do wielu.

Ale przecież tabele to są relacje ...

@Nevas

A tak pzt to nie masz testów.

A niby jakie ma mieć testy ? albo smoke albo E2E jeśli mu bardzo zależy i wsio....

Ja bym się na twoim miejscu nie przejmował za bardzo. Pójdź gdzieś na Juniora ktoś z tobą posiedzi pierwszy tydzień i powinno być ok.

Pozostało 580 znaków

2018-08-15 18:42

Rejestracja: 11 lat temu

Ostatnio: 3 dni temu

2

Nie zagłębiałem się w temat, ale zwróciłem uwagę na odpowiedzi dotyczące wyjątków / błędów, napiszę Wam ciekawostkę. U mnie w pracy błędy użytkowników, np. wprowadzenie liter w polu tekstowym jest zabezpieczone oczywiście po stronie klienta przez javascript, ale jak ktoś mimo wszystko wykona błędny request do API np. postmanem, fiddlerem czy czymś tam to ZAWSZE zwracamy kod błędu 400, a nie żadnych innych typu: 404 (not found), 403 itd. - zwracanie różnych kodów błędu z API jest niebezpieczne. Przykładowo wysyłając tysiące requestów do API gdyby jakaś metoda API zwracała 404, że czegoś nie ma albo 401, że nie mamy do czegoś dostępu to atakujący miałby łatwiejsze zadanie znalezienia jakiejś luki, bo wiedziałby, że np. dane id klienta nie istnieje, a inne istnieje, ale nie ma do niego dostępu, a skoro istnieje to może spróbować teraz dla niego wywołać jakąś inną metodę gdzie to zabezpieczenie może nie być sprawdzane zamiast wysyłać tysiąc losowych requestów co może zostać zauważone.
U mnie w firmie 2 razy w roku są przeprowadzane przez zewnętrzne firmy testy penetracyjne i często we wnioskach pojawia się właśnie tego typu uwaga.

Pokaż pozostałe 3 komentarze
@WeiXiao: Tak. Ale raczej rzucasz 403 Forbidden, a nie 400 Bad Request, bo błąd 400 oznacza, że coś jest nie tak z żądaniem HTTP (np. niezrozumiały format). Oczywiście zależy to od poziomu "czystości" REST-a ;-) - Ktos 2018-08-15 20:37
@Ktos: No dobra, a co mam rzucić jeżeli wszystko jest OK poza inputem od usera (niekoniecznie podczas logowania)? nic mi nie pasuje poza 400 / 406 - WeiXiao 2018-08-15 20:40
@WeiXiao: Ja nie mówię, że 400 albo 406 w ogóle są złe (bo zły input od usera bym traktował jako 400), tylko, że zwracanie jednego kodu dla każdego możliwego błędu niezbyt mi się podoba :-) (BTW, co do rate limit: 429 Too Many Requests) - Ktos 2018-08-15 20:44
@Ktos: Przez limity na API miałem na myśli ustawienie limitów, co zapobiega temu, że ktoś będzie skanował nasze api z np. 1k req/10sec. Ja nie mówię, że 400 albo 406 myślałem, że przez napisanie błąd 400 oznacza, że coś jest nie tak z żądaniem HTTP chciałeś powiedzieć, że 400 nie jest do takich błędów (input) :P Wydaje mi się, że większość rzucanych przez nas kodów to głównie: OK/unauthorized/notfound/badrq do inputów, bo reszta to tak średnio. No może teapot :D - WeiXiao 2018-08-15 20:47
@WeiXiao: Fuck, masz rację, sam się zagalopowałem w kozi róg. - Ktos 2018-08-15 20:59

Pozostało 580 znaków

Nadziany Szczur
2018-08-16 11:06
Nadziany Szczur
1

Co za głupota.

Zadanie powinno brzmieć co masz zrobić.
A tutaj jest opisane wytyczne w jaki sposób to zrobić i jak pisać. Zero kreatywności.
Narzucają architekturę i implementacje.

Nie można później porozmawiać czemu użyłeś 64 bitowego klucza zamiast 32? Czemu GUID a nie long?
Po co ORM?

Czuć jakimś rygorystycznym team leaderem, który narzuca swój sposób pisania. Omijałbym takie firmy.

Pozostało 580 znaków

Odpowiedz

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