Cześć. :)
Rzadko pokazuje komuś swój kod, przez to sam nie wiem, jakiej on jest jakości.
Przypisany jestem do dwóch projektów uczelnianych, meteo nie jest skończone.
O nazwach commitów nie musicie mi mówić, czasem robimy niezły meksyk. :)
Co rzuca się wam w oczy? Nad czym popracować? Za co w firmie mnie złoją? :)
Nie wiem, za co w firmie cię złoją, ale ja bym te pliki co są wszystkie wrzucone do czytnik jakoś posegregował, bo tak nawet nie wiadomo od czego zacząć czytanie :)
dopoki porzadku w repo nie bedzie to mi sie nie chce na to patrzec
(pierwsza zauważona rzecz) pliki binarne w repozytorium i to wrzucone w losowe miejsce.
namespace czytnik - najważniejsza nazwa w kodzie, a źle nazwana - namespace powinno być z wielkiej litery. I angielskie.
public static List<string> PobierzAdresyKanalow() List<string> adresyKanalow = new List<string>(); - polskim nazwom zmiennych/w kodzie mówimy "nie".
Regex linkParser = new Regex(@"((http|ftp|https):\/\/[\w\-_]+(\.[\w\-_]+)+([\w\-\.,@?^=%&:/~\+#]*[\w\-\@?^=%&/~\+#])?)", RegexOptions.Compiled | RegexOptions.IgnoreCase); - ten regex to hardcore, i do tego jeszcze niepoprawnie skopiowany więc nie działa (&?).
foreach (Match m in tableRegEx.Matches(html))
table = table + " " + m.Value;
Jeśli matchów może być dużo to zły pomysł klejenie tego w taki sposób (bo wydajność) - ale to pewnie nie problem.
foreach (Match m in linkParser.Matches(table))
{
if (!adresyKanalow.Contains(m.Value))
adresyKanalow.Add(m.Value);
}
jesli adresy mają byc unikalne to może lepiej użyć Set<...> zamiast List<...>? (Też, mało wydajne i niesemantyczne)
public static List<Kanal> UtworzKanalzArtykulami(List<string> adresyKanalow) - jak wyżej
if (item != "http://ekstraklasa.tv/ekstraklasa-tv,83,m.xml" && item != "http://tvn24bis.pl/tvn24-bis,1,m.xml") - co to za hacki? :P Jeśli już to do jakichś stałych tłumaczących dlaczego akurat te dwa adresy pomijamy.
public static string Wyciagarka(string strZrodlo, string strStart, string strKoniec)
{
int Start, End;
if (strZrodlo.Contains(strStart) && strZrodlo.Contains(strKoniec) != strZrodlo.Contains("http"))
{
Start = strZrodlo.IndexOf(strStart, 0) + strStart.Length; // btw to powinno być z małej litery koniecznie.
End = strZrodlo.IndexOf(strKoniec, Start);
return strZrodlo.Substring(Start, End - Start);
}
else
{
return "";
}
}
Wygląda na jakąś pomocniczą funkcję. To jeszcze nic złego, ale powinna być w takim razie prywatna. Bo robi jakąś czarną magię (łącznie ze sprawdzaniem czy źródło zawiera "http" i porównywaniem tego z tym czy źródło zawiera koniec), nie udostępniałbym tego w publicznym API.
To uwagi do pliku https://github.com/TransportKrausPoloTeam/Czytnik-RSS/blob/master/czytnik/czytnik/DataLoader.cs (jakby co).
TL;DR główny problem to niekonsekwentne nazwy zmiennych/funkcji, polskie nazwy zmiennych/funkcji, wielkie litery w nazwach zmiennych/funkcji gdy powinny być małe oraz małe gdy powinny być wielkie.
To /naprawdę/ jest ważne, reszta punktów to luźniejsze uwagi.
Ok, może inaczej, bo przyznaje się, repozytorium, to jest mambo jambo, a pisalismy we 4 osoby (jeśli o czytnik chodzi) :) Dzięki za uwagi.
Pozwolę sobie wrzucić trochę kodu, którym sam się zajmowałem, razem z planem i opisem (powinienem tak do od początku zrobić, z repo dla kogoś zewnątrz to masakra faktycznie). Plan w załączniku.
DataContainer- http://wklej.org/id/1913524/
DataRepository- http://wklej.org/id/1913873/
Matrix - http://wklej.org/id/1913529/ (Służy do operacji na macierzach- nie skończona, choć działająca już dość sprawnie)
DataProcessor - http://wklej.org/id/1913530/
I 2 klasy testujące:
DataContainerTest - http://wklej.org/id/1913533/
DataProcessorTest - http://wklej.org/id/1913541/
To część kodu, którym zajmowałem się samodzielnie, do niektórych plików się nikomu nie wcinałem. Ja zajmowałem się głównie planowaniem, obsługą bazy danych no i tym co jest tutaj.
Nie stosuje się prefixa _ w nazwach metod.
Zamiast dawać komentarze "sekcja mnożenie" lepiej użyć po prostu #region cośtam (a najlepiej dzielić na klasy).
Czemu w klasie Matrix metody przyjmują argumenty z modyfikatorem ref?
Zależności, takie jak np. MeteoStationDBContext dla klas DataContainer czy DataRepository powinno się wstrzykiwać z zewnątrz, ogólnie polecam poczytać o IoC.
To DataRepository to jakiś taki dziwny worek na wszystko, na 99% każdy rodzaj danych powinien mieć swoje repozytorium. Nie rozumiem też, po co rozróżnienie na DataContainer i DataRepository, obie te klasy służą chyba do pośredniczenia między aplikacją a bazą danych.
Przy rzucaniu wyjątków nie powinno się dawać informacji dla użytkownika typu "popraw dane".
To tak na pierwszy rzut oka.
Cześć. mnie by to bardzo nawet i zainteresowało... jakiś bardziej obszerny projekt do analizy... Jednakże patrząc na multum rzeczowych uwag aż ręce opadają... zatem ja przynajmniej poczekam na jakiś uporządkowany materiał... pozdrowka DeNiss