Prośba o code review.

Odpowiedz Nowy wątek
2015-10-16 19:27

Rejestracja: 5 lat temu

Ostatnio: 3 godziny temu

Lokalizacja: Krakow

0

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.
user image
06cc519127.png
http://matthewpopielarz.azurewebsites.net

GitHub:
https://github.com/pixellos/W[...]er/tree/master/WeatherChecker
https://github.com/pixellos/MyWebpage/tree/New

edytowany 2x, ostatnio: Pixello, 2015-10-16 19:38
Odważny jesteś. - BlackTomato 2015-10-16 21:48
Dlaczego? Według mnie żeby być w czymś średnim, później dobrym, to trzeba przejść przez okres bycia do niczego :P - Pixello 2015-10-16 21:49
No ja wiem, ja to wiem ale inni nie wiedzą. Niektórzy byli od razu idealni i pewnie dowiesz się że powinieneś wrócić do gimnazjum albo do żłobka xD - Polskie fora. - BlackTomato 2015-10-16 22:00
Może ktoś się taki znajdzie, ale ja bardziej liczę na to, że ktoś bardziej doświadczony mnie choć trochę nakieruje Przeglądając internet i ucząc się tego co potrzeba nie jestem w stanie dobrać odpowiednich technik, a że jestem w szkole średniej, na dodatek programowanie nie stoi tam na wysokim poziomie (nie mam w programie w ogóle programowania), to muszę szukać pomocy tu :P - Pixello 2015-10-16 22:12

Pozostało 580 znaków

Wielki Kot
2015-10-16 20:42
Wielki Kot
1

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 ;]

Zedytowałem tytuł. Wiem, że grafik ze mnie marny ;) - Pixello 2015-10-16 21:06

Pozostało 580 znaków

2015-10-16 21:20

Rejestracja: 5 lat temu

Ostatnio: 15 minut temu

1

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.

Dzięki, poczytam o TPL, 37 to tak dałem w ferworze walki z kodem i zostało. Zmieniłem na (24 / 3) * 5 - 1, pogoda 3 godzinna na 5 dni. - Pixello 2015-10-16 21:40
(24 / 3) * 5 - 1 tym bardziej nic nie mówi, zrób z tego jakąś stałą o dobrej nazwie, albo coś. - some_ONE 2015-10-16 21:45
CountOfForecastsForEveryOf3HoursPer5Days, tak nazwałem stałą i do niej to równanie wrzuciłem. Tak ogólnie to na kilka miesięcy programowania (wcześniej to były zmienne a,b,c,d,e,f,g itp w c, więc raczej nie liczę), to jak się to prezentuje? - Pixello 2015-10-16 21:47

Pozostało 580 znaków

2015-10-17 14:12

Rejestracja: 5 lat temu

Ostatnio: 3 godziny temu

Lokalizacja: Krakow

0

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

79b68a5e2e.png

zdecydowanie lepiej - Wizzie 2015-10-17 17:32

Pozostało 580 znaków

Wybitny Młot
2015-10-17 16:59
Wybitny Młot
0

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

Pozostało 580 znaków

2015-10-17 17:42

Rejestracja: 5 lat temu

Ostatnio: 3 godziny temu

Lokalizacja: Krakow

0

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?

edytowany 3x, ostatnio: Pixello, 2015-10-17 17:44

Pozostało 580 znaków

Wybitny Młot
2015-10-17 18:00
Wybitny Młot
0

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

Zapomniałem o tym, miałem na autoryzację z katany zmienić, zrobiłem sobie przykładowy programik który jej używał, ale jakoś już na stronie tego nie zastosowałem :) Parę podstawowych wzorców, czyli które są podstawowe? Co do panelu, nad CMS projektów trochę krwi zjadłem ;) Jakby ktoś chciał zobaczyć jak chodzi, to url do logowania to https://matthewpopielarz.azurewebsites.net/Login/Login - Pixello 2015-10-17 18:04

Pozostało 580 znaków

2015-10-25 16:10

Rejestracja: 5 lat temu

Ostatnio: 3 godziny temu

Lokalizacja: Krakow

0

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

http://popielarzmatthew.azurewebsites.net/ 23b3ec7f5b.png

Pozostało 580 znaków

2015-11-16 13:40

Rejestracja: 8 lat temu

Ostatnio: 2 lata temu

Lokalizacja: Wrocław

1

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
edytowany 1x, ostatnio: Zellus, 2015-11-16 13:41

Pozostało 580 znaków

Odpowiedz

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