Ocena kodu projektu bloga

0

Hej, tak jak już niedawno wspominałem, pracuję aktualnie nad blogiem który docelowo ma gdzieś tam w przyszłości wystartować. Przy okazji staram się stosować nowo poznane wzorce projektowe i powszechnie stosowane praktyki przy pisaniu w ASP.NET MVC, stąd prośba do Was :) Proszę o sprawdzenie czy niżej zamieszczony kod nie zawiera jakichś poważniejszych WTF'ów. Interesują mnie zwłaszcza:

Oczywiście są jeszcze jakieś modele, viewmodels ale wydaje mi się że nie da się tam za bardzo popsuć czegokolwiek. Ważniejsze biblioteki które używam to Entity Framework (ORM), Value Injecter (konwersja model <-> view model), NLog (logowanie błędów), Unity (IoC).
Pozdrawiam!

EDIT: Oczywiście projekt jest w fazie rozwojowej, napisane jest kilka podstawowych modułów, niemniej jednak to odzwierciedla jak będzie wyglądać całość.

0

Ave, przed chwilą zaktualizowałem kod bloga, więc gdyby ktoś mógł go ocenić pod względem liczby wtf'ów to bardzo bym prosił.

I jeszcze jedno pytanie, w konstruktorach kontrolerów mam następujące konstrukcje:

private ISecurityService _securityService = null;
private ILogService _logService = null;
private ISettingsService _settingsService = null;
private ICategoriesService _categoriesService = null;
private IArticlesService _articlesService = null;
private ITagsService _tagsService = null;
private ICommentsService _commentsService = null;
private ISitesService _sitesService = null;

[InjectionConstructor]
public HomeController(ISecurityService securityService,
					  ILogService logService,
					  ISettingsService settingsService,
					  ICategoriesService categoriesService,
					  IArticlesService articlesService,
					  ITagsService tagsService,
					  ICommentsService commentsService,
					  ISitesService sitesService)
{
	_securityService = securityService;
	_logService = logService;
	_settingsService = settingsService;
	_categoriesService = categoriesService;
	_articlesService = articlesService;
	_tagsService = tagsService;
	_commentsService = commentsService;
	_sitesService = sitesService;
}

Nie wydaje mi się to zbyt ładne - zna ktoś jakieś lepsze rozwiązanie na wstrzykiwanie większej liczby serwisów do kontrolera? A może rozbić jeden kontroler na kilka mniejszych, np. osobny kontroler dla artykułów, osobny dla komentarzy itp. Własna implementacja fabryki kontrolerów która odpowiada za wstrzykiwanie znajduje się tutaj: https://github.com/AlfaLeporis/blog/blob/master/Blog/App_Start/CustomControllerFactory.cs

1

#Wywal wszystkie adnotacje od DI
#Kazdy kontroler niech odpowiada za pewny fragment aplikacji: obsluga komentarzy, obsluga profilu uzytkownika, obsluga logowania/rejestracji, obluga ...
#Stworz sobie atrybut ValidateModelState
#Wprowadz potwierdzenie usuwania i returnUrl, dzieki ktoremu wrocisz na strone, z ktorej zostalo cos wykonane
#Dlaczego zakladasz, ze da sie edytowac nieistniejacy post?
#Generalnie to ja preferuje, zeby mi serwisy zwracaly wszystkie dane potrzebne do wyswietlania/przemapowania na viewmodele. Inaczej robi sie syf w tych kontrolerach.
#Wydziel osobny bootstraper do rejestrowania zaleznosci.
#Nie trzymaj konfiguracji w kontekscie.
#Zbedna konfiguracja jest naprawde zbedna. :P

0
  1. Chodzi Ci o atrybut InjectionConstructor? To akurat jest potrzebne, ponieważ moja implementacja fabryki kontrolerów szuka konstruktora z podanym atrybutem i wywołuje go z serwisami.
  2. Ok, czyli tak jak myślałem, jeden ogólny kontroler to niefajne rozwiązanie.
  3. Doczytam z czym to się je.
  4. Ok, te wpisane nazwy widoków przy returnach zamienię na mechanizm returnUrl.
  5. Ha, w niektórych serwisach nie dopisałem sprawdzania czy element istnieje. Będzie poprawione.
  6. Czyli rozumiem że konwersję Model <-> ViewModel należy wykonywać w kontrolerach, a serwisy mają przyjmować tylko modele?
  7. Efekt lenistwa z początkowych etapów pisania, poprawię.
  8. EntityTypeConfiguration? Trochę tych klas będzie, ale dopiszę.
  9. Faktycznie EF poradziłby sobie pewnie i bez manualnego wskazywania klucza. Przy najbliższej okazji wywalę to.

Dzięki serdeczne, takich właśnie wskazówek mi brakowało ;)

1
  1. To zmien implementacje. Serio, kiedys ci przyjdzie do glowy pomysl zmiany DI i bedziesz to manualnie rzezbil...
using System;
using System.Web.Mvc;
using System.Web.Routing;
using Microsoft.Practices.Unity;

namespace TestContainerLifeTimeManager.Infrastructure {
    public class UnityControllerFactory : DefaultControllerFactory {
        private readonly IUnityContainer _container;

        public UnityControllerFactory(IUnityContainer container) {
            _container = container;
        }

        protected override IController GetControllerInstance(RequestContext requestContext, Type controllerType) {
            try {
                if(controllerType == null) {
                    throw new ArgumentNullException("controllerType");
                }

                if(!typeof(IController).IsAssignableFrom(controllerType)) {
                    throw new ArgumentException(string.Format("Type request is not a controller: {0}\n", controllerType.Name), "controllerType");
                }

                return _container.Resolve(controllerType) as IController;
            } catch {
                return null;
            }
        }

        public override IController CreateController(RequestContext requestContext, string controllerName) {
            try {
                return (IController)_container.Resolve(GetControllerType(requestContext, controllerName));
            } catch {
                return base.CreateController(requestContext, controllerName);
            }
        }

        public override void ReleaseController(IController controller) {
            _container.Teardown(controller);
            base.ReleaseController(controller);
        }
    }
}

Mi takie cos wystarcza i nie wymaga niczego.

  1. Niekoniecznie. Po prostu w niektorych przypadkach te modele w warstwie serwisow, a koncowe do usera sie roznia.
  2. Dlatego nie pisz czegos, co EF z automatu ogarnia. Nazywanie tabel po swojemu tez jest... dziwne. Albo to olej jak nie masz takiego wymagania odgornie, albo po prostu zmien konwencje nazewnictwa.
0
  1. Nazewnictwo tabel wprowadziłem "swoje" z tego względu że domyślnie miałem je w bazie jako "TagsModels", "ArticleModels" itp. co kłóciło się z moją wizją czystej i czytelnej DB. Wymagań odgórnych nie mam bo projekt jest tylko i wyłącznie na osobisty użytek ;)
0

Hej, nareszcie udało mi się wyeliminować wszystkie błędy wskazane poprzednio przez Was, a także dodać kilka nowych ficzerów. I tak jak poprzednio, proszę o mały code-review, gdyby ktoś coś zauważył to pisać ;)

Oprócz powyższych fixów, dodałem serwis odpowiedzialny za wyszukiwanie stron i artykułów, obsługę błędów i kilka innych mniejszych rzeczy. Miejsca na które chciałbym zwrócić główną uwagę to:

Kontrolery po stronie klienta: https://github.com/AlfaLeporis/blog/tree/master/Blog/Controllers
Filtry: https://github.com/AlfaLeporis/blog/tree/master/Blog/Filters
Serwisy: https://github.com/AlfaLeporis/blog/tree/master/Blog/Services
DAL (customowy mechanizm nazywania tabel): https://github.com/AlfaLeporis/blog/blob/master/Blog/DAL/DatabaseContext.cs

Kwestii bezpieczeństwa i autoryzacji na razie nie ruszałem, więc tym się nie przejmować.

2

pierwszy z brzegu service: https://github.com/AlfaLeporis/blog/blob/master/Blog/Services/Articles/ArticlesService.cs

  • linie 63, 64 sa nadpisywane przez linie 65,66 przez co sa zbedne.

  • wydziel sobie ten kawalek kodu odpowiedzialny za populowanie(nie wiem czy to dobre slow) Modelu do osobnej metody i wywoluj ja z parametrem obiektu, a paczemu to dalej zobacz

  • metoda Add niech zwraca cos wiecej niz tylko boola, bo przy istniejacym artykule, lub blednym zapisie do bazy, nie wiesz nawet co sie stalo i uzytkownik tez nie wie. sam osobiscie uzywam MethodResponse, ktore moze byc accepted lub nie wraz z informacja co i gdzie sie stalo.

  • ogolnie w pozostalych metodach typu: GetByAlias, GetByTagName etc etc, sprawdzasz czy obiekt istnieje w bazie pobierajac go z niej, po czym jak istnieje to kaze go pobrac jeszcze uzywajac metody Get (notabene lamie ona SRP, bo oprocz pobierania to jeszcze konwertuje na viewmodel), czyli notabene pobierasz ten sam obiekt dwa razy z bazy uzywajac dwoch zapytan i dwoch roznych kontekstow, jak ci przyjdzie edytowac obiekt w metodzie GetByAlias i tam go zapisac a wezmiesz go z metody Get to wysypie sie niezgodnosc contextow. jak wydzielisz populowanie modelu jak wspomnialem wyzej to bedziesz mial tylko pobieranie obiektu raz i przesylanie go do metody ktory przetworzy go na viewmodel.

  • getAll jak wyzej plus w petli wywoluje za kazdym razem nowy obiekt w nowym kontekscie (zabija to idee EF :))

  • metoda GetByTagName i GetShortByTagName roznia sie wyrazem short (no ok nazwa jednej z metod) w kodzie jednej z nich, a obie maja po 17 linijek kodu. da sie to scalic.

  • usuwanie artykulow ci dziala?? bo najpierw usuwasz artykul, a potem w drugim kontekscie usuwasz tagi (funkcja RemoveByArticleID zawsze zwraca true, chyba ze sie wywali to wtedy sie wywali:)) i zapisujesz pierwszy gdzie te tagi wciaz sa.

  • co zapisujesz do bazy w GetAllShortVersion, bo jest tam SaveChanges()

Tak ogolnie to nie meczy cie przyjecie zasady nowa metoda nowy context?? Mysle ze czasem wiecej klopotu to moze sprawic niz korzysci, a lepsza bylaby np. metoda na klase ub service w tym przypadku.

0
  1. Chyba wczoraj przez przypadek to dopisałem, sprawdzę.
  2. Fakt, oddzielna metoda na tą operację będzie dobra, dopiszę.
  3. To już jest chyba bardziej kosmetyczna zmiana, aczkolwiek Twój pomysł z klasą MethodResponse, gdzie mamy jakiegoś enuma z kodem błędu i stringa z treścią gdy takowy nastąpi wydaje się ok. Dopiszę.
  4. Ha, tutaj wyszedł niezły babol :) Muszę się nad tym głębiej zastanowić jak to ładnie rozwiązać.
  5. Tak samo jak powyżej.
  6. Fakt, jeden bool i dało by się z tego zrobić ładną funkcję. Poprawię.
  7. Działa, być może dlatego że usuwam tagi przed zapisaniem kontekstu, a w metodzie RemoveByArticleID tworzę nowy. W każdym razie zdecydowanie do poprawy.
  8. Pomyłka, poprawię.

Co do kontekstu EF, rozumiem że dobrze by było stworzyć oddzielny serwis w taki sposób, że zawierałby jeden kontekst na cały program, czy oddzielny na każdy kontroler/serwis?

Dzięki serdeczne za uwagi ;)

0

Hej, dzisiaj kolejna wersja CMS'a (już oficjalnie Photon CMS, tak dumnie bardzo ;)). Oprócz poprawy błędów które mi wskazaliście, dodany został moduł paginacji, obsługa wysyłania/wybierania obrazków w CKEditorze, skrypt Google Analytics, prymitywne zabezpieczenia przed nieautoryzowanymi akcjami i inne drobiazgi.

Od dzisiaj można śledzić postępy również tutaj: http://alfaleporis.somee.com/ - to jest taka przykładowa strona oparta na tymże systemie. Co jakiś czas postaram się ją uaktualniać. Z góry przepraszam za wolne ładowanie, ale nie stać mnie obecnie na żaden płatny hosting ASP.

Do panelu administratora można dostać się przez adres http://alfaleporis.somee.com/Administrator, login "AlfaLeporis", hasło "password".

I tradycyjnie proszę Was bardzo o ocenę kodu, a zwłaszcza:

Pozdrawiam! ;)
PS. ReturnUrl miał od kilku dni klasyczny wylew, więc chwilowo jest usunięty. Postaram się to kiedyś naprawić.

1

Pobrałem aplikacje z ciekawości odpaliłem i teraz nie wiem czy to u mnie tylko tak strasznie muli jak klikam między stronami w sensie czekanie po 10 - 15 sekund aż się następna załaduje. Czyżby czekał mnie zakup nowego laptopa? :-o

0

Hehe, tak jak pisałem wyżej, darmowa taryfa somee.com niestety nie rozpieszcza :)

1

Okej zresetowałem laptopa i śmiga. :)

Tak parę rzeczy na pierwszym kilkaniu rzuciło mi się w oczy bardziej estetyczne.

  1. Czemu jak dodale nowa kategorie i chce ja zakaceptowac to mam klikać "Edytuj kategorie"
  2. Jak błędnie wypełnię dodanie wpisu na przykład bez kategorii i tematu to w ogóle się wyburacza. :P
  3. W ustawieniach na przykład ilość tagów ,elementów na stronę mogę wpisać "dasd23213dasdasdasd2" i nic złego się nie stanie. :D
    Ale jak już przejdę na stronę główną lub ErrorPage to strzela błędami na lewo i prawo. :P

Zgaduje że cała walidacja zostanie dodana później ale brakuje jej. :)

0

Ha, a ja już pół godziny szukałem w kodzie co powoduje takie zakrzywienie czasu :P

  1. Hmm, kiedyś było ale usunąłem... w sumie nie pamiętam dlaczego. Dopiszę, tak samo jest pewnie w edycji artykułów, stron itp.
    2 i 3. Walidacja faktycznie trochę leży, jeszcze na początku projektu się pilnowałem i do każdej właściwości pisałem odpowiednie atrybuty, a teraz to w ogóle lenistwo. Poprawię.

Dzięki serdeczne za uwagi ;)

2

Moim zdaniem serwis do paginacji jest średni, dlatego że korzystanie z niego wymusza pobranie wszystkich rekordów z bazy danych.

0

Hmm, faktycznie nie wygląda to zbyt dobrze. Popatrzę jeszcze na przykładowe implementacje w innych projektach i postaram się to poprawić. Dzięki!

1

Pewno jakoś tak:

public static class QueryOverExtensions
{
    public static IQueryOver<TRoot> ApplyPaging<TRoot>(this IQueryOver<TRoot> queryOver, int currentPageNumber, int pageSize)
    {
        return queryOver.Skip((currentPageNumber - 1) * pageSize).Take(pageSize);
    }
}

z dokładnością do użytego ORMa.

1

Ja bym metody serwisu które wymagają paginacji rozszerzył o dodatkowe przeładowanie, gdzie ten dodatkowy parametr by był klasą zawierającą opcje paginacji.

1

A co to jest "serwis paginacji"?

Mi serwis kojarzy się z logiką biznesową - np. fakturami albo stanem magazynu. W przypadku bloga może istnieć serwis postów, serwis komentarzy, serwis użytkowników, serwis wyszukiwarki, ale serwis paginacji?

Paginacja to operacja wykonywana na źródle danych, to element infrastruktury, nie logiki biznesowej, więc czemu serwis do tego?

Paginację zwykle obsługuje się poprzez klasę PagedRequest<T>, która zawiera informację o type encji do pobrania, liczbie encji na stronę, oraz numerze strony, który chcemy pobrać. Obiekt tego żądania tworzony jest gdzieś wysoko, np. w kontrolerze. I ten PagedRequest obsługiwany jest gdzieś głęboko, np. w bazowym serwisie albo DAO, który nakłada stronicowanie na zapytanie do źródła danych. W drugą stronę zwracany jest jakiś PagedResponse<T>, który zawiera te same informacje, co PagedRequest<T> uzupełnione oczywiście o listę pobranych encji.

0

Ok, dzięki za wyjaśnienie, w takim razie będę próbował w tym kierunku poprawić paginację. Zawsze to jeden serwis mniej do injectowania w kontrolerach ;)

0

CMS Photon v0.5

Nowa wersja Photona już dostępna ;) Przynosi ona zarówno kilka nowych ficzerów jak i sporo poprawek błędów, także tych które Wy mi wskazaliście. Lista z najważniejszymi zmianami poniżej:

  • Dodano system kopii zapasowych (pliki xml spakowane do zipa) z możliwością odtwarzania ich
  • Dodano generator mapy witryny
  • Poprawiono system paginacji (nie pobiera już całej tabeli, tylko te rekordy które są naprawdę konieczne)
  • Dodano brakującą walidację formularzy
  • Zmieniono format linków, od teraz wyglądają bardziej przejrzyście (np. zamiast Articles/Article/alias jest po prostu Article/alias)
  • Poprawiono tytuły stron
  • Dodano logowanie błędów aplikacji
  • Dodano aliasy dla kategorii
    Sama strona została przeniesiona na nowy serwer, więc od teraz można już spokojnie ją testować ekstremalnie szybko w porównaniu z somee i bez reklam :)

Adres: http://alfaleporis.cf/
Panel administratora: http://alfaleporis.cf/Administrator

Dane do logowania:
Administrator
L: AlfaLeporis
P: password

Zwykły użytkownik:
L: TestUser
P: password

Tak jak poprzednio, proszę o ocenę funkcjonowania CMSa jak i kodu, w szczególności:

1

Jedno pytanie na jakiej zasadzie jest wyświetlana kolejność postów.

  1. 2014-07-01 1447 -> istniejący
  2. 2014-07-01 1427 -> istniejący
  3. 2014-07-01 1643 -> dodałem przed chwilą
  4. 2014-06-01 1450 -> istniejący
0

Hm, teoretycznie według daty publikacji. Anyway, raczej nie taki efekt chciałem uzyskać, sprawdzę to.

1

do kilku rzeczy sie czepne, ale z frontendu:

http://alfaleporis.cf/?page=566 powinno wywalac na poczatek

usuwanie rzeczy jest proste, zbyt proste, bardzo latwo cos usunac ,a z braku opcji wersjonowania tracimy to bezpowrotnie. najlepiej nie usuwac, tylko zmieniac status , a jeszcze lepiej dac taki feature jak wersjonowanie

templatke bedziesz zmienial?? bo ta nie jest ani responsywna, ani statyczna i na komorkach sie krzaczy.

jak jestem zalogowany jako administrator to nie mam jak wejsc do panelu be znajomosci urla

nie moge zalogowac sie na zwyklego usera i hasla zmienic tez nie moge

to takie pierwsze 5 minut:)

1

do kilku rzeczy sie czepne, ale z frontendu:

http://alfaleporis.cf/?page=566 powinno wywalac na poczatek

usuwanie rzeczy jest proste, zbyt proste, bardzo latwo cos usunac ,a z braku opcji wersjonowania tracimy to bezpowrotnie. najlepiej nie usuwac, tylko zmieniac status , a jeszcze lepiej dac taki feature jak wersjonowanie

templatke bedziesz zmienial?? bo ta nie jest ani responsywna, ani statyczna i na komorkach sie krzaczy.

jak jestem zalogowany jako administrator to nie mam jak wejsc do panelu be znajomosci urla

nie moge zalogowac sie na zwyklego usera i hasla zmienic tez nie moge

to takie pierwsze 5 minut:)

0
szalonyfacet napisał(a):

http://alfaleporis.cf/?page=566 powinno wywalac na poczatek

Ok, poprawię.

szalonyfacet napisał(a):

usuwanie rzeczy jest proste, zbyt proste, bardzo latwo cos usunac ,a z braku opcji wersjonowania tracimy to bezpowrotnie. najlepiej nie usuwac, tylko zmieniac status , a jeszcze lepiej dac taki feature jak wersjonowanie

Hmm, właśnie w tej wersji dodałem do każdej tabeli kolumnę IsRemoved więc teoretycznie usunięte dane nadal istnieją w bazie - aczkolwiek nie ma opcji przywracania więc to też jest do dopisania. Nad wersjonowaniem pomyślę. A, no i gdzieś leży skrypt z javascriptowym alertem z potwierdzeniem dla jakichś akcji, to też muszę uruchomić.

szalonyfacet napisał(a):

templatke bedziesz zmienial?? bo ta nie jest ani responsywna, ani statyczna i na komorkach sie krzaczy.

Obiecuję sobie ciągle że coś z nią będę kombinował i nadal nie mogę się zebrać z tym :) Anyway, postaram się coś z tym zrobić.

szalonyfacet napisał(a):

jak jestem zalogowany jako administrator to nie mam jak wejsc do panelu be znajomosci urla

Ok, dodam linka.

szalonyfacet napisał(a):

nie moge zalogowac sie na zwyklego usera i hasla zmienic tez nie moge

Niemożliwe, na login 'TestUser' i hasło 'password' wchodzi bezproblemowo :) Zmiana hasła tak samo nie wywala jakichś błędów.

Dzięki!

0

CMS Photon v0.6

Dzisiaj kolejna wersja w której na pewno na pierwszy rzut oka rzuci się... nowa templatka ;) Oprócz tego zabezpieczenie CAPTCHA, wersjonowanie i kilka innych fajnych ficzerów.

Lista zmian:

* Dodano mechanizm wersjonowania artykułów i stron
* Dodano anty-botowe zabezpieczenie CAPTCHA
* Dodano alerty potwierdzające akcje podejmowane przez użytkownika
* Dodano możliwość wpisania nazwy autora komentarza dla niezarejestrowanych użytkowników
* Dodano możliwość przywracania usuniętych elementów
* Dodano okruszki (breadcrumbs)
* Dodano stronę ładną błędu

* Zmieniono szablon strony
* Poprawiono błąd związany z błędną kolejnością artykułów
* Poprawiono błąd związany z błędną liczbą elementów na stronie kategorii/tagu
* Poprawiono mechanizm SEO
* Poprawiono obsługe błędów
* Spolszczono linki
* Inne mniejsze poprawki

Adres: http://alfaleporis.cf/
Panel administratora: http://alfaleporis.cf/Administrator

Dane do logowania:
Administrator
L: AlfaLeporis
P: password

Zwykły użytkownik:
L: TestUser
P: password

Kod do oceny gdyby ktoś był tak dobry:

1

Zrobiłem coś takiego:

Test.png
treść skopiowałem z:
http://msdn.microsoft.com/en-us/library/gg405484(v=pandp.40).aspx

kliknąłem zapisz i wywaliło mi błąd :D

Server Error in '/' Application.

Runtime Error

Description: An exception occurred while processing your request. Additionally, another exception occurred while executing the custom error page for the first exception. The request has been terminated.

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