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
4
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:
- 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
-
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) - 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:
- 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.