Zadanie rekrutacyjne

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

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?

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ć.

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ę...

1

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/Company/blob/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/Company/blob/master/Services/CompanyService.cs -> klasyczny bląd, czyli upchniecie całej logiki aplikacji w jedną biedną klasę.
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.

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.

1
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/Company/blob/master/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);
        }
	}
}
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.

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.

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.

0

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

No jeśli narzuca to chyba dobrze? Sam czasem nie lubię jakiegoś podejścia czy standardów ale i tak wolę dyscyplinę w kodzie niż samowolke. Od swojego sposobu pisania sa wlasne projekty.

0

Fakt w zespole powinien być standard.

Ale w zadaniu rekrutacyjnym? Przecież tu chodzi o to by zobaczyć jak kandydat pisze. Narzucając mu styl ogranicza się go. Ba może sprawdzający boi się, że innego kodu nie zrozumie? :)

1
Nadziany Szczur napisał(a):

Fakt w zespole powinien być standard.

Ale w zadaniu rekrutacyjnym? Przecież tu chodzi o to by zobaczyć jak kandydat pisze. Narzucając mu styl ogranicza się go. Ba może sprawdzający boi się, że innego kodu nie zrozumie? :)

Bycie dobrym programistą według ortodoksyjnego ruchu craftsmanship, to bycie rzemieślnikiem a nie artystą który ma swój styl ;). W Facebooku mają trochę odmienne zdanie, bo tam mantrą jest one hacker way...

Ale wracając do tematu, dawanie style guidu na zadaniu rekrutacyjnym ma jak najbardziej sens.
Dla rekrutującego, bo pokazuje czy kandydat jest się w stanie podporządkować panującym zasadom w firmie.
Dla kandydata, bo może się ustosunkować czy mu pasuje dany sposób zapisu kodu czy nie, i czy chce pracować z takim kodem, dla niektórych osób naprawdę sprawą życia i śmierci jest używanie tabów zamiast spacji :

Zresztą ten style guide tutaj wymagany to i tak bardzo delikatny jest, bo zwykle to są opasłe tomiska, w stylu tego z googla:
https://google.github.io/styleguide/cppguide.html

0

Może jakość kodu nie była na wystarczająco "dobra"? Co od siebie, żółtodzioba mogę powiedzieć, to tutaj Code review. Hcubyc przekonał mnie do pakietowania per funkcjonalność. Dodatkowo,pod koniec jest bardzo fajny wykład na temat czystości kodu. Zacząłem przerabiać książkę "Czysty kod" i tam było napisane, że lepiej zamiast ISomeInterface już zrobić SomeInterfaceImpl (chociaż jedno i drugie nie jest zalecane).

0

(...)lepiej zamiast ISomeInterface już zrobić SomeInterfaceImpl (chociaż jedno i drugie nie jest zalecane).

Czyli że co?

0

Źle napisałem. Chodziło mi o to, że jeżeli jest sytuacja gdy ma się do wyboru nazwać coś IFoo jako interfejs i Foo jako klasa implementująca ten interfejs, to lepiej jest nazwać Foo - interfejs, a FooImpl klasę implementującą ten interfejs. https://www.investigatii.md/uploads/resurse/Clean_Code.pdf strona 55 "Interfaces and Implementations".

0

@Aisekai, autor napisał ten podrozdział bardzo ostrożnie. W pozostałych pisze o tym, co czytelnik powinien robić i często używa słów you need to albo you should, a w tym odniósł się tylko do własnych odczuć i preferencji i nie za bardzo stara się przekonać do używania tej zasady. Wydaje mi się, że przestawienie się przez jednego oświeconego programistę na takie nazewnictwo w projekcie tworzonym przez więcej niż jedną osobę wprowadziłoby trochę chaosu.

These are sometimes a special case for encodings. For example, say you are building an
ABSTRACT FACTORY for the creation of shapes. This factory will be an interface and will
be implemented by a concrete class. What should you name them? IShapeFactory and
ShapeFactory? I prefer to leave interfaces unadorned. The preceding I, so common in
today’s legacy wads, is a distraction at best and too much information at worst. I don’t
want my users knowing that I’m handing them an interface. I just want them to know that
it’s a ShapeFactory. So if I must encode either the interface or the implementation, I choose
the implementation. Calling it ShapeFactoryImp, or even the hideous CShapeFactory, is preferable
to encoding the interface.

0

a co jeżeli jedna klasa implementuje kilka interfejsów, lub kilka klas jeden interfejs?

3
Aisekai napisał(a):

Źle napisałem. Chodziło mi o to, że jeżeli jest sytuacja gdy ma się do wyboru nazwać coś IFoo jako interfejs i Foo jako klasa implementująca ten interfejs, to lepiej jest nazwać Foo - interfejs, a FooImpl klasę implementującą ten interfejs.

@Aisekai: nie, po prostu nie. Tak sobie można w Javie pisać. W C# zaczynanie interfejsu od I jest standardową i powszechną konwencją, jeśli ktoś się z niej wyłamuje, to rodzi podejrzenia, że w ogóle C# nie zna.
Argumentum ad Martinum też nietrafione, bo on o Javie, a nie C# pisał.

1

@somekind będę wiedział na przyszłość :)

2

Ciesz się że Cie odrzucili. Nie chcesz tam pracować. 2 lata później dostałem to samo zadanie kropka w kropke (widocznie ni chce im się modyfikować zadania...). I zrobiłem wg ich wytycznych i mojego 3 letniego doświadczenia. Byłem zadowolony z zadania bo zrobiłem też wg najnowszych standardów i zrobiłem to w Net Core 3.1. Mało tego dałem kumplowi który posiada ponad 10 lat doświadczenia do oceny i stwierdził, że jest idealnie.
A teraz: W emailu z zadaniem oznajmili, że pozytywny odbiór zadania wpłynie na zatrudnienie. Mówie "Super w końcu porządna firma bo stawia na doświadczenie i to jak ktoś wykona zadanie a nie to czy ma wykute na blache losowe pojęcia". Wysłałem. Zadanie im się spodobało ale chcą jeszcze rozmowe na skype 30 min. Pomyślałem, że może chcą o projekcie pogadać i upewnić się że ja go zrobiłem.
Masakra. Jakbym w ogóle tego zadania nie robił i jakby w ogóle go nie było. Ani słowa nie wspomnieli o nim. Rekruter opowiadał o firmie a potem programista zaczął mi zadawać pytania z teorii. I nie takie ogólne gdzie są wszędzie indziej które badają czy ma ktoś pojęcie o programowaniu np o generyki, klasy abstrakcyjne itd itp. Nie on zaczął mi zadawać pytania o konkretne, indywidualne funkcjonalności, które oni używają w swoim projekcie i szukali na zasadzie klucza jak na maturze. Ktoś może się wstrzeli i też korzystał z tego co oni. Ale nie mówie tu np o frameworku konkretnym. Tylko konkretna metoda np od microsoftu...kiedy nie kojarzyłem to próbowałem na logikę odpowiadać. A programista do mnie, że nie chodzi o logiczne odpowiedzi i myślenie tylko żebym odpowiedział zgodnie z definicją...aha. Rozmowa zakończona. Negatywny feedback dostałem w piątek o godz 22:00 !!! O czymś to świadczy...dostałem odpowiedź że wg nich obiektywnie nie ma wystarczającego doświadczenia.
No tak nie mam bo widocznie oni nie szukają programisty kreatywnego z logicznym myśleniem i chętnego do nauki tylko statysty, który szczęśliwie wykuje odpowiednie teorie na blache a zadanie zerżnie z neta albo ktoś inny mu zrobi i gra gitara...
Nie polecam pracy w tej firmie w dziale IT...
Dawno się tak nie wkurzyłem bo zmarnowali mój czas na zrobienie zadania. Skoro tak chcą postępować niech najpierw zadają durne pytania osobie i już wtedy eliminują. A nie dają najpierw zadanie, niech zmarnuje siły i czas a potem zalejemy go pytaniami. W pewnym momencie miałem wrażenie, że zaraz programista mnie zapyta "Wg Ciebie jak sądzisz, jak nazywamy zmienne w kodzie? NP mamy listę pracowników i masz 3 szanse na zgadnięcie jak nazywamy taką zmienną"...

0

No ja aplikowałem tam drugi raz, mając coś ponad 2 lata expa. Zadanie też zrobilem idealnie według nich ale na rozmowie z programistą trochę dałem ciała, ale raczej pytali o normalne rzeczy. Ale w sumie cieszę się bo teraz pracuje za lepszą stawkę niż tam oferowali. Skoro tam nie pracowales to tak naprawdę nie wiesz jak jest. Ja też nie. Może i sposób rekrutacji nie jest idealny ale czy tak chyba w wielu miejscach jest. Ja jak podejmuje się zadania to liczę się z tym, że mimo pozytywnie zrobionego nie zostanę zatrudniony. Miałem tak, już w innej firmie. Zadanie super, jakiś prosty crud, a potem pytania o mikroserwisy itp

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