Ocena dwóch projektów webowych

0

Witam
Chciałbym wam pokazać moje dwa projekty, które wykonałem za pomocą
Razor page:
https://github.com/daniel500013/notepad_razor
oraz przy użyciu
MVC:
https://github.com/daniel500013/shop_mvc
Chciałbym, abyście ocenili bardziej backend, ponieważ jeżeli chodzi o frontend to nie zwracałem na to zbytniej uwagi więc może być tam trochę błędów (choć oczywiście przyjmę jakąś porade jeżeli będzie jakiś rażący błąd).

Od razu mogę zaznaczyć, że nie korzystałem tutaj z gotowych szablonów logowania lub rejestracji, które są wbudowane, ponieważ uznałem, że to by było trochę bez sensu gdybym korzystał z gotowych rozwiązań, ponieważ nic bym się zbytnio z tego nie nauczył (jedynie co to wykorzystywałem generowanie widoku i czasem kontrollerów). EntityFramework'a uczę się dopiero od ~1 miesiąca więc mam świadomość, że nie wszystko jest doskonałe w moim kodzie jednak mam nadzieje, że wskażecie mi moje błędy oraz może dacie jakieś rady jak usprawnić kod lub czego omijać

3

Największym problemem jaki widzę, jest to, że trzymasz logikę w kontrolerze, zamiast w osobnej klasie, którą sobie wstrzykniesz do konstruktora kontrolera.
Dzięki przeniesieniu logiki z kontrolera, możesz stworzyć testy jednostkowe, nie będzie nieładu w kodzie(dobra przesadzam tutaj), łatwiej(i szybciej) przerobić apkę na REST API i wystawić endpointy i to jest ogólnie dobre podejście w programowaniu. Do widoczków w Razorze się nie odnoszę, ostatni raz w nich pisałem z 5/6 lat temu.

Edit: Jeszcze jedna rzecz jakiej nie lubię to wysyłanie całego modelu bazodanowego na widok. @model shop_mvc.Models.ProductModel
W większych apkach trzymasz więcej danych w bazie(dany powstania, modyfikacji, kto stworzył, kto zmienił ostatnio itp. Ogólnie metadane, oraz dane, które przydane są administracji systemu, ale nie userowi), dlatego ja zawsze stosuję ViewModels, który leci do usera. Są tam tylko niezbędne dane potrzebne, aby wyświetlić userowi to co trzeba.
Zamiast public int Id { get; set; } zobacz na coś co się nazywa UUID, w C# nazywa się to GUID.

1

Na plus, że screenshot w readme :)

lekcij
linij
migracij

bawisz się w Korwina, czy robisz błędy ortograficzne? ;)

0

Nie wstrzykuj kontekstu EF do kontrolerów.

0

@somekind: A możesz mi dać jakiś przykład? Bo nie za bardzo wiem, jak inaczej mógłbym dodać kontekst EF i użyć go w kontrolerze

3

@daniel500013: nie używać go w kontrolerze w ogóle. Użyj w innej klasie, której dopiero użyjesz w kontrolerze. Kontrolery powinny być proste i nie zawierać logiki biznesowej ani operować na bazie danych.

0

Wielkie dzięki za tą poradę oraz artykuł, w którym zostało wszystko bardzo fajnie wyjaśnione na temat wzorca mvc :D. Powiem szczerze, że początkowo myślałem, że wstrzykiwanie kontekstu EF do kontrolera jest normalną praktyką, ponieważ gdy zaczynałem się uczyć, tworzyć aplikacje w tym wzorcu to właśnie w dokumentacji Microsoftu był przykład ze wstrzykniętym kontekstem do kontrollera.

  • Starałem zastosować się do wskazówek i usunąłem kontekst EF w kontrolerze oraz rozdzieliłem logikę na oddzielne klasy i zaimplementowałem je w kontrolerze.
  • Nie zmapowałem jeszcze danych za pomocą ViewModelu, ale niedługo postaram się to zrobić.
  • Dodałem do projektu ze wzorcem MVC kilka testów jednostkowych jednak nie jestem pewny co do ich poprawności, ponieważ są to jedne z pierwszych testów jednostkowych, jakie kiedykolwiek tworzyłem, ale mam nadzieje, że tak napisałem te testy, że mają jakikolwiek sens, ale oczywiście chętnie dowiem się, na co zwracać uwagę w testach jednostkowych jeżeli coś pominąłem
2

Te Twoje testy to raczej nie są testy jednostkowe, skoro pod spodem używają bazy danych.

Zamiast

[InlineData(1)]
[InlineData(2)]
[InlineData(3)]
[InlineData(4)]

Użyj [MemberData].

Serwisy powinny być wstrzykiwane do kontrolerów, a kontekst EF do serwisów. Nie powinieneś sobie tworzyć instancji serwisów w każdej metodzie kontrolera, to powinno być pole tej klasy. Analogicznie z kontekstem w serwisach.

1

Według mnie powinieneś wstrzykiwać DbContext do serwisów poprzez konstruktor zamiast kodu tego typu w każdej metodzie:

using (var context = new ShopDbContext())
{
...
}

AccountService:

if (checkPassword == PasswordVerificationResult.Success)
{
	var claims = new List<Claim>
	{
		new Claim(ClaimTypes.NameIdentifier, (context.User.ToList().Count+1).ToString()),
				new Claim(ClaimTypes.Email, user.Email),
				new Claim(ClaimTypes.Role, user.Role),
	};
...

Linia 5 w powyższym wycinku. Dla mnie to nie ma sensu. Według mnie chcesz tam umieścić ID. Dodatkowo w takim wypadku nie musisz już odpytywać dbContextu bo pobrałeś Usera kilka linii wyżej.

https://stackoverflow.com/questions/5814017/what-is-the-purpose-of-nameidentifier-claim

Nie możesz ufać danym przesyłanym do serwera. Obecnie w Twojej aplikacji możliwe jest podszywanie pod innych użytkowników i kupowanie na "ich konto".

<a class="btn btn-success" asp-controller="Order" asp-action="Order" asp-route-userID="@User.FindFirst(ClaimTypes.NameIdentifier).Value">Kup</a>
Zamiast przesyłania asp-route-userID w widoku pobierz wartość w kontrolerze: var userId = User.FindFirst(ClaimTypes.NameIdentifier);

Mógłbyś też pozbyć się przekazywania UserId w metodach serwisów poprzez wstrzyknięcie serwisu, który miałby wstrzyknięty IHttpContextAccessor. Przykładowy kod poniżej:

public sealed class UserContext
    {
        public int UserId { get; private set; }
        public UserContext(IHttpContextAccessor httpContextAccessor)
        {
            UserId = GetUserId(httpContextAccessor.HttpContext);
        }
        public static int GetUserId(HttpContext? httpContext)
        {
            int result = -1;

            if (httpContext?.User?.Identity is ClaimsIdentity identity)
            {
                string? nameIdentifierValue = identity.FindFirst(ClaimTypes.NameIdentifier)?.Value;
                int.TryParse(nameIdentifierValue, out result);
            }

            return result;
        }
    }

Zamiast:

public async Task<IActionResult> Delete (int productID, int userID)
{
    await Task.Run(() =>
    {
        var orderService = new OrderService();
        return orderService.DeleteProduct(productID, userID);
    });

    return RedirectToAction("Index", new { id = userID });
}

Lepiej tak:


// GET /OrderController/Create
public async Task<IActionResult> Delete(int productID, int userID)
{
    var orderService = new OrderService();
    await orderService.DeleteProduct(productID, userID);
    return RedirectToAction("Index", new { id = userID });
}

1

Poprawiłem już serwisy i teraz pobierają kontekst bazy danych z konstruktora oraz zmieniłem przekazywanie serwisów do kontrolerów, zamiast tworzyć je pojedynczo w każdej funkcji i teraz serwisy pobierane są również z konstruktora. Mam nadzieje, że nic nie pomyliłem i że teraz wygląda to w miarę sensownie.

Co do tego pobierania ID w AccountService to ten błąd jest akurat trochę nieumyślnie zrobiony, ponieważ przy rejestracji nie mogłem pobrać ID użytkownika z bazy danych, ponieważ dopiero co tworzyłem samego użytkownika w bazie danych dlatego użyłem tego tak, a nie inaczej 😅 oczywiście przy logowaniu nie ma to sensu dlatego szybko naprawiłem mój błąd.

Poprawiłem również przekazywanie ID usera przy przycisku kup i już chyba nie powinno być tak dużych błędów bezpieczeństwa jak wcześniej

Jeszcze została sprawa samych testów, które przestały mi działać po wprowadzeniu do konstruktora serwisu kontekstu bazy danych, ale posprawdzałem trochę i znalazłem w dokumentacji Microsoftu, że można to chyba rozwiązać za sprawą biblioteki Moq jeszcze za bardzo się w to nie zagłębiałem, ale dziś wieczorem będę sobie czytał dokumentacje jak tego w ogóle używać

0

Zdecyduj się na jedną konwencję:

_logger = logger;
this.homeService = homeService;

Interfejsy do serwisów - co mają na celu?

Weź rób Pull Requesty, będzie łatwiej oglądać co zmieniłeś. :)

0

@somekind: Poprzez użycie interfejsów mogę przenieść funkcje z serwisów do konstruktora w kontrolerze i wywołać je tam.

W pliku Program.cs przenosze te wszystkie funkcje za pomocą interfejsów.
Tutaj kod odpowiedzialny za to:

builder.Services.AddScoped<IHomeService, HomeService>();
builder.Services.AddScoped<IAccountService, AccountService>();
builder.Services.AddScoped<IOrderService, OrderService>();
builder.Services.AddScoped<IProductService, ProductService>();
3

Ale nie potrzebujesz do tego interfejsów. builder.Services.AddScoped<HomeService>(); też zadziała.

2
public async Task OrderProduct(HttpContext httpContext)
{
    int userID;

    var userIDString = httpContext.User.FindFirst(ClaimTypes.NameIdentifier)?.Value;
    int.TryParse(userIDString, out userID);
    ...
}

Nie podoba mi się przekazywanie HttpContextu jako parametru. Jeżeli nie chcesz korzystać z UserContext z mojej poprzedniej odpowiedzi to z poziomu kontrolera dziedziczącego po ControllerBase masz dostęp do właściwości User o typie ClaimsPrincipal, możesz sobie napisac extensions method do wyciągania ID.

//czemu GET a nie POST?
public async Task<IActionResult> Order()
{
    var userId = User.GetId();
    await orderService.OrderProduct(userId);

    return LocalRedirect("/Home/Index");
}

// ClaimsPrincipalExtensions.cs:
public static int GetId(this ClaimsPrincipal cp) => ...

Jednak dalej uważam, że przekazywanie ID jako parametru nie jest najlepszym rozwiązaniem. I rozwiązanie tego typu uważam za lepsze:

public class OrderService
{
    private UserContext userContext;
    public OrderService(..., UserContext userContext)
    {
        ...
        this.userContext = userContext;
    }
    

    public async Task DeleteProduct(int productID)
    {
        var userId = userContext.UserId;
        ...
    }

    public async Task OrderProduct()
    {
        var userId = userContext.UserId;
        ...
    }

}

Żeby to działało trzeba dodać services.AddHttpContextAccessor(); w metodzie ConfigureServices w Startup.cs

0

@somekind: Usunąłem te interfejsy z serwisów oraz zmieniłem AddScoped nie było tam zbyt wiele rzeczy do zmiany, ale mam nadzieje, że jakoś to wpływa na jakość :D

@Kokoniłaj: Zaimplementowałem twój kod dotyczący odczytywania id użytkownika i muszę przyznać się, że dzięki temu dowiedziałem się, że jest takie coś jak IHttpContextAccessor za co dziękuję, bo wydaję się to bardzo przydatne :D

1

No to jeszcze wymyśl jakieś ficzery biznesowe do tego projektu wymagające jakiejś logiki biznesowej, aby napisać trochę sensownych testów.

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