Prośba o code review.

Witam,

Ostatnio trochę popisałem sobie w c# i chciałbym prosić o sprawdzenie/porady co do mojego kodu.
Napisałem stronę w ASP.NET i aplikację do sprawdzania pogody z OpenWeatherMap.


http://matthewpopielarz.azurewebsites.net

GitHub:
https://github.com/pixellos/WeatherChecker/tree/master/WeatherChecker
https://github.com/pixellos/MyWebpage/tree/New

Wiem że to nie miesjce na to ale grafika jest okropna... zarówno na stronie jak i w aplikacji.
Jeśli nie masz talentu graficznego to polecam minimalizm lub/i bootstrap, bo ciężko się to ogląda.

Drugą rzecz o którą się przyczepie to "code review kodu". Masło jest maślane ;]

Na szybko rzuciłem okiem.

To co zauważyłem:

void BackgroundThreat()
{
       while (true)
       {
           Thread.Sleep(1000);
           WeatherData = _weatherConnection.GetWeatherData();
           OnPropertyChanged("");
       }
}

Nazwa metody nic nie mówi, kod w tej metodzie pobiera jedynie co sekundę dane pogodowe(w jakim celu tak często?). Czemu więc taka nazwa?

Thread.Sleep() słabo, bardzo słabo. Blokujesz na sekundę cały wątek, który został uruchomiony(co gorsze za pomocą gołego Thread ale o tym zaraz) tylko do pobierania danych. Czemu nie użyłeś await Task.Delay(1000), albo timera, który co sekundę będzie wywoływał tę metodę.

Zresztą, gdybyś to pobieranie danych pogodowych zrobił asynchronicznie, to nie potrzebny byłby żaden nowy wątek, bo GUI nie byłoby blokowane podczas pobierania danych.

No i literówka w nazwie, "threat" ma trochę inne znaczenie :P

new Thread(BackgroundThreat) {IsBackground = true}.Start();

Czemu goły Thread?
Od wprowadzenia .NET Framework 4.0, czyli od 5 lat, zalecanym sposobem tworzenia wielowątkowego i równoległego kodu jest TPL.

for (int i = 0; i < 37; i++) 

Jakieś magiczne liczby, nie wiadomo dlaczego akurat 37.

Czyli mówicie, że taka bardziej surowa lepiej się prezentuje?

No cóż... nadal nie jest to http://4programmers.net/Forum/Off-Topic/Oceny_i_recenzje/258919-moje_portfolio_w_html5
ale już trochę lepiej - z roku 1995 przeszliśmy graficznie do roku 1999 a to już coś
Poucz się trochę może jednak o kompozycji, kontraście i dopasowaniu barw - na początek postaraj się tak dobrać kolor tekstu żeby dało się go w ogóle przeczytać nad tłem

Ok, teraz rozkminiam, zgodnie z radą kolegi wyżej, TPLa, nad frontem mam zamiar zasiąść jak będę miał większą wiedzę o backendzie.
Chciałbym się zorientować, czy z moim poziomem kodowania mógłbym się na jakiegoś juniora starać, a jak nie jakie wiadomości musiałbym przyswoić aby móc, czy wysyłając teraz podanie wprowadziłbym tylko rekruterów w dobry nastrój :P?

ogarnij TPL, kolekcje, parę podstawowych wzorców, logowanie bardziej skomplikowane niż:

 if (model.UserName=="admin" && model.Password=="admin")
                {
                    return View("Index", null);
                }

i myślę że możesz próbować startować na juniora. W najgorszym przypadku dowiesz się czego Ci brakuje. Tylko nie pokazuj im tej strony ;)
No i na przyszłość nie podawaj linka do źródeł z prawdziwym hasłem do panelu admina :D

Lepiej trochę wygląda? Kodu na gh nie aktualizuję, bo jest mocno roboczy.

http://popielarzmatthew.azurewebsites.net/

Jest lepiej :) ale jeżeli w przyszłości chciałbyś się pochwalić pracodawcy, a takie portfolio to zawsze duży plus, to radziłbym jeszcze trochę bardziej dopasować do dzisiejszych trendów. Przejrzyj sobie proste przykłady na tej stronie http://startbootstrap.com/template-categories/popular/ może znajdziesz jakąś inspirację. Ten obrazek u góry wygląda, naprawdę... średnio.
Co do code review:

• ForecastPartialView.cs - na pewno przyczepiłbym się do tego że w niektórych liniach masz aż 7 poziomów wcięcia, może dałoby się coś z tym zrobić, np. zamienić

if(value != null) {
    ...
}
return null;

na

if(value == null)
    return null;
...

Co nieco możesz do przerzucić do funkcji.

• ForecastPartialView.cs:91 - tak w ogóle to po co łapać ten wyjątek, skoro i tak rzucasz go dalej?
• OpenWeatherConnection.cs - takie porównywanie JSONa wygląda kiepsko. Poza tym obsługuje tylko 404 dla nie znalezienia miasta, a co jeżeli wystąpi inny błąd? Może warto tego sparsować tego JSONa i porównać

message.code != 200

-- to jest tylko przykład, nie znam tego api i nie wiem co zwraca, ale mam nadzieję że rozumiesz, o co mi chodzi.

• Do parsowania JSON możesz użyć jakiejś fajnej biblioteki, z tego co widzę dla xmla tak właśnie zrobiłeś. Niestety nie znam żadnego przykładu wartego uwagi, musisz sam poszukać.
• Model chyba nie jest dobrym miejscem do budowania zapytań do strony (OpenWeatherConnection.cs)
  To tak na pierwszy rzut oka