Witam,
jestem dość początkującym programistą, niedawno zacząłem się uczyć C# i WPFa. Napisałem prostą aplikację do ściągania mangi ze strony, na której można je czytać online przy użyciu HtmlAgilityPack. Lubię poczytać sobie od czasu do czasu mangę, ale na stronach tego typu często wyskakują reklamy, poza tym klikanie pomiędzy stronami zajmuje więcej czasu niż samo czytanie... dlatego też postanowiłem sobie napisać coś takiego dla własnej wygody. Jako że DibbyDum i Shalom zasugerowali mi w moim temacie w Edukacji, żebym zwrócił się w tym dziale o pomoc przy sprawdzaniu kodu, chciałbym żebyście mi wytknęli wszystkie błędy, jakie popełniłem przy pisaniu tej aplikacji. Program nie jest skończony, mam zamiar jeszcze wprowadzić możliwość czytania mangi z poziomu mojej aplikacji, ale ściąganie i podstawowe opcje zostały zaimplementowane.
Nie mam doświadczenia w pisaniu dłuższych aplikacji, więc każde sugestie co do poprawy kodu są mile widziane. A oto link do githuba:
Takie moje przemyślenia na na pierwszy rzut oka. :)
- Brakuje sprawdzania czy katalog do przechowywania mangi istnieje, powinien sam stworzyć jeżeli go nie ma.
- Od strony WPFa:
a. Strasznie miszmasz i rozpierdziel zainteresuj się stylami żeby nie pisać po 10 razy:
<TextBlock FontFamily="Verdana" FontSize="12" Grid.Column="1" VerticalAlignment="Center" HorizontalAlignment="Center" ... />
b. Zamiast upychać wszystko w jedno okno w kolejnych zakładkach widział bym tam bardziej kolejne UserControle ładnie by to rozdzieliło całość.
- Kod csharp:
a. Taki konstrukcje są wdług mnie zbędne:
public List<string> mangaChapters { get; set; }
public List<string> mangaPages { get; set; }
public Manga()
{
mangaChapters = new List<string>();
mangaPages = new List<string>();
}
całoś powyżej można by zastąpić:
public List<string> mangaChapters new List<string>();
public List<string> mangaPages new List<string>();
dwa razy mniej kodu i konstruktor jest też zbędny bo i tak nic nie przekazujesz podczas tworzenia klasy i inicjalizacja w nim zmiennych również.
b. Nazwy zmiennych i funkcji również bywają mylące:
protected override void gettingMangaName() // Ale i tak nic nie zwracasz?
protected string directoryAndMangaPath { get; set; }
protected string mangaPath { get; set; }
// Itd. Oczywiście nie wszystkie ale parę by sie jeszcze znalazło
c. Wielokrota implementacja INotifyPropertyChanged powinieneś raz zaimplementować i używać gdzie Ci potrzeba
d. Zakończenie działania funkcji poptrzez wyjątek?
infoString = "Manga has probably ended(or an error occured).\n" + infoString; bez sensu. :P
e. Jeszcze przebudował bym BaseMangaDownloader czemu ma ona się zajmować np. createDirectory. Ogólnie struktura jeszcze do przemyślenia.
- Widzę że próbujesz rozdzielać UI od pozostałej logiki bardzo dobrze ale zainteresuj się wzorcem MVVM żeby to jakoś uporządkować.
Zaden ze mnie .netowiec więc tylko uwagi ogólne:
-
Nie duplikujemy kodu. Nigdy
protected void createDirectory()
{
if (!Directory.Exists(directoryAndMangaPath))
{
Directory.CreateDirectory(directoryAndMangaPath);
}
}
protected void createDirectory(string concat)
{
if (!Directory.Exists(directoryAndMangaPath + @"\" + concat))
{
pageNumber = 1; //when creating new chapter directory, page number resets itself to zero.
Directory.CreateDirectory(directoryAndMangaPath + @"\" + concat);
}
}
Przemyśl czy nie da się tego jednak zrobić w jednej metodzie, albo w dwóch które korzystają ze wspólnego kodu.
- Więcej niż jednak kropka w linijce = zło. Nie łamiemy enkapsulacji, bo po coś ona jest. Coś takiego jest niepoprawne:
public List<Manga> mangas = new List<Manga>();
public void enumerateMainDirectory()
{
mangas.Clear();
string[] list = Directory.GetDirectories(DefaultOptions.Instance.DirectoryPath);
Console.WriteLine(list.Count());
foreach (string d in list)
{
//Console.WriteLine(d);
Manga manga = new Manga();
manga.name = Path.GetFileName(d);
string[] listOfChapters = Directory.GetDirectories(d + @"\");
foreach (string chapter in listOfChapters)
{
manga.mangaChapters.Add(chapter);
}
mangas.Add(manga);
}
}
Manga powinna mieć kontruktor ktory przyjmuje listę stringów albo powinna mieć metodę inicjalizujacą którą ma taki argument. Klasa Manga (i tylko ona!) powinna wiedzieć jak sobie wewnętrznie przechowuje te chaptery. Nie powinieneś ujawniać na zewnątrz takich własności. Wewnętrzna implementacja klasy to jest jej prywatna sprawa ;)
- Jeden poziom zaglębienia na metodę, maksymalnie dwa ale tylko jeśli to jest sensownie umotywowane. Poza tym metody maja być krótkie i jasno opisywać co robią.
public override void StartDownloading()
{
if (!initializationException)
{
gettingMangaName();
bool continueLoop = true;
while (continueLoop)
{
if (gettingMangaChapterAndPage())
continueLoop = true;
else
{
continueLoop = false;
break;
}
var imgDiv = document.DocumentNode.SelectSingleNode("//img[@id='img']");
string imgLink = imgDiv.Attributes["src"].Value.ToString();
string nextUrl = "http://www.mangareader.net" + imgDiv.ParentNode.Attributes["href"].Value.ToString();
Console.WriteLine(nextUrl);
downloadImage(imgLink, directoryAndMangaPath + @"\" + chapterNumber + @"\" + pageNumber + Path.GetExtension(imgLink));
infoString = "Page " + pageNumber + " of " + chapterNumber + " chapter has been downloaded.\n" + infoString;
mangaPath = nextUrl;
initParser(mangaPath);
pageNumber++;
}
}
}
To jest bardzo zła metoda.
- czemu to nie jest
while (gettingMangaChapterAndPage())tylko jakiś taki dziwny twór z ifami? o_O - czemu te dziwne instrukcje wewnątrz pętli nie są objęte nową metodą? Ja na przykład nie do konca rozumiem co ten kod robi. Nie da sie go objąć metodą z sensowną nazwą?
Tak bardzo na szybko pewne rzeczy, jakie mi się rzuciły:
Model / BaseMangaDownloader.cs
public String InfoString;
public String infoString
{
get { return InfoString; }
set { InfoString = value; this.OnPropertyChanged("infoString"); }
}
Konwencja nazewnicza C# jest taka, że publiczne składowe klasy są nazywane wielką literą, a prywatne nazywane małą literą. W przypadku właściwości, to pole o które jest oparta właściwość często jest zaczynana znakiem podkreślenia. Nie wspominając, że tutaj to w ogóle jakiś galimatias, dlaczego oba te elementy są publiczne? :-)
Podobnie masz np. w DefaultOptions.cs - _DirectoryPath i DirectoryPath są oba publiczne. Niektórzy jeszcze tutaj przyczepili by się do tego, że odwołanie do InfoString również powinno być z this. oraz, że dodatkowo raczej używa się string zamiast String. Np. StyleCop ma takie zasady w standardowym zestawie. Ogólnie, to StyleCop w moim średnio-liberalnym ustawieniu znalazł tutaj 239 "błędów stylowych".
Plus, ja widziałem jeszcze nieco inne rozwiązanie - zamiast _pole = value najpierw jest sprawdzanie, czy value nie jest przypadkiem takie samo jak wartość pola i dopiero jeżeli jest następuje przypisanie i wysłanie komunikatu PropertyChanged.
MangaReaderDownloader.cs
private bool gettingMangaChapterAndPage()
Getting? Dlaczego nie get?
if (gettingMangaChapterAndPage())
continueLoop = true;
else
{
continueLoop = false;
break;
}
Ja osobiście nie jestem przeciwnikiem konstrukcji if bez klamerek, ale w sytuacji, kiedy istnieje else raczej zaleca się wersję z klamrami. Nie wspominając, że mógłbyś to dokładnie zrobić odwrotnie i możesz śmiało zastąpić przez:
if (!gettingMangaChapterAndPage())
{
continueLoop = false;
break;
}
Albo, jak wspomniane wyżej - całe wywołanie getting... wrzucić do warunku pętli while.
ownloadImage(imgLink, directoryAndMangaPath + @"\" + chapterNumber + @"\" + pageNumber + Path.GetExtension(imgLink));
infoString = "Page " + pageNumber + " of " + chapterNumber + " chapter has been downloaded.\n" + infoString;
Polecam String.Format (jest naprawdę super!) oraz Path.Combine.
Dodatkowo:
- w solucji masz jakieś TataMySQL ;-),
- "Mainainability Index", czyli ocena m.in. podziału twoich klas na mniejsze składowe wychodzi w okolicach 80 punktów - to dobrze. Ale właśnie te zauważone przez Shaloma StartDownloading i gettingMangaCośtamCośtam mają już w okolicach 50. Nadal daleko do 0 w sumie ;-)
Jak by dać while (true){..} to całość zadziała tak samo bo i tak wychodzisz z pętli używając break;
while (continueLoop)
{
if (gettingMangaChapterAndPage())
continueLoop = true;
else
{
continueLoop = false;
break;
}
// ...
}
Lepiej tutaj pasuje użycie:
do
{
// ...
} while (gettingMangaChapterAndPage())
Dobra, wziąłem się za poprawę moje kodu według waszych sugestii.
@DibbyDum
ad. 1
Wprowadziłem sprawdzanie, czy katalog istnieje, jeśli go nie ma, to jest tworzony.
ad. 2
Stworzyłem odpowiednie style, żeby kod w xamlu był bardziej czytelny.
Rozdzieliłem zakładki na odpowiednie UserControls.
ad. 3
Poprawiłem wytknięte konstrukcje w kodzie:
public List<string> mangaChapters { get; set; }
public List<string> mangaPages { get; set; }
public Manga()
{
mangaChapters = new List<string>();
mangaPages = new List<string>();
}
na:
public List<string> mangaChapters = new List<string>();
public List<string> mangaPages = new List<string>();
Pozmieniałem nazwy zmiennych i metod, na bardziej opisowe.
protected override void gettingMangaName() // Ale i tak nic nie zwracasz?
protected string directoryAndMangaPath { get; set; }
protected string mangaPath { get; set; }
na:
protected override void extractMangaName()
protected string localMangaPath { get; set; }
protected string mangaURL { get; set; }
c. Wielokrota implementacja INotifyPropertyChanged powinieneś raz zaimplementować i używać gdzie Ci potrzeba
Używam tej implementacji w 2 klasach: Manga i BaseMangaDownloader, w jaki sposób to zaimplementować, żeby obie klasy mogły z tego korzystać bez duplikacji kodu?
d. Zakończenie działania funkcji poptrzez wyjątek?
Poprawiłem to, wcześniej w wypadku wyjątku NullReferenceException metoda kończyła działanie, teraz sprawdzam ifem czy dana zmienna jest nullem, jeśli tak to metoda kończy działanie. Mam nadzieje że to o to chodziło :p
private bool extractMangaChapter()
{
var chapterDiv = document.DocumentNode.SelectSingleNode("//img[@id='img']");
string altAtt = null;
if (chapterDiv == null)
return false;
else
altAtt = chapterDiv.Attributes["alt"].Value.ToString();
string chapterNumberGarbage = altAtt.Remove(0, mangaName.Count());
string[] tempString = chapterNumberGarbage.Split('-');
currentChapterNumber = "Chapter " + tempString[0].Trim();
createDirectory(currentChapterNumber);
return true;
}
e. Jeszcze przebudował bym BaseMangaDownloader czemu ma ona się zajmować np. createDirectory. Ogólnie struktura jeszcze do przemyślenia.
createDirectory znajduje się w BaseMangaDownloader, ponieważ w przyszłości zamierzam, obok MangaReaderDownloader stworzyć inne klasy, pozwalające na ściąganie mangi z innych stron. Te klasy prawdopodobnie też będą korzystały z tej metody, dlatego się znajduje w BaseMangaDownloader. Wydaje mi się, że tak powinno być, jeśli nie to poprawię:P
- Widzę że próbujesz rozdzielać UI od pozostałej logiki bardzo dobrze ale zainteresuj się wzorcem MVVM żeby to jakoś uporządkować.
Tak, czytałem już trochę o tym wzorcu, ale nie udało mi się znaleŹć w miarę aktualnego tutoriala do niego, będę szukał dalej :)
@Shalom
Udało mi się zamknąć createDirectory w jednej metodzie, mam nadzieję, że zrobiłem to w poprawny sposób:
protected void createDirectory(string concat = "")
{
if (!String.IsNullOrEmpty(concat))
concat = @"\" + concat;
if (!Directory.Exists(localMangaPath + concat))
{
currentPageNumber = 1; //when creating new chapter directory, page number resets itself to zero.
Directory.CreateDirectory(localMangaPath + concat);
}
}
- Więcej niż jednak kropka w linijce = zło. Nie łamiemy enkapsulacji, bo po coś ona jest. Coś takiego jest niepoprawne:
public List<Manga> mangas = new List<Manga>();
public void enumerateMainDirectory()
{
mangas.Clear();
string[] list = Directory.GetDirectories(DefaultOptions.Instance.DirectoryPath);
Console.WriteLine(list.Count());
foreach (string d in list)
{
//Console.WriteLine(d);
Manga manga = new Manga();
manga.name = Path.GetFileName(d);
string[] listOfChapters = Directory.GetDirectories(d + @"\");
foreach (string chapter in listOfChapters)
{
manga.mangaChapters.Add(chapter);
}
mangas.Add(manga);
}
}
Manga powinna mieć kontruktor ktory przyjmuje listę stringów albo powinna mieć metodę inicjalizujacą którą ma taki argument. Klasa Manga (i tylko ona!) powinna wiedzieć jak sobie wewnętrznie przechowuje te chaptery. Nie powinieneś ujawniać na zewnątrz takich własności. Wewnętrzna implementacja klasy to jest jej prywatna sprawa ;)
Poprawiłem to na:
public List<Manga> mangas = new List<Manga>();
private string directory = DefaultOptions.Instance.DirectoryPath;
//niestety tutaj muszą być dwie kropki;p
public void findAllMangas()
{
mangas.Clear();
string[] mangaList = Directory.GetDirectories(directory);
foreach (string title in mangaList)
{
Manga manga = new Manga();
manga.name = Path.GetFileName(title);
string[] listOfChapters = Directory.GetDirectories(title + @"\");
manga.setChapterList(listOfChapters);
mangas.Add(manga);
}
}
I metoda w klasie Manga
public void setChapterList(string[] listOfChapters)
{
foreach (string chapter in listOfChapters)
{
mangaChapters.Add(chapter);
}
}
W ten sposób chyba enkapsulacja została zachowana.
- Jeden poziom zaglębienia na metodę, maksymalnie dwa ale tylko jeśli to jest sensownie umotywowane. Poza tym metody maja być krótkie i jasno opisywać co robią.
public override void StartDownloading()
{
if (!initializationException)
{
gettingMangaName();
bool continueLoop = true;
while (continueLoop)
{
if (gettingMangaChapterAndPage())
continueLoop = true;
else
{
continueLoop = false;
break;
}
var imgDiv = document.DocumentNode.SelectSingleNode("//img[@id='img']");
string imgLink = imgDiv.Attributes["src"].Value.ToString();
string nextUrl = "http://www.mangareader.net" + imgDiv.ParentNode.Attributes["href"].Value.ToString();
Console.WriteLine(nextUrl);
downloadImage(imgLink, directoryAndMangaPath + @"\" + chapterNumber + @"\" + pageNumber + Path.GetExtension(imgLink));
infoString = "Page " + pageNumber + " of " + chapterNumber + " chapter has been downloaded.\n" + infoString;
mangaPath = nextUrl;
initParser(mangaPath);
pageNumber++;
}
}
}
To jest bardzo zła metoda.
- czemu to nie jest while (gettingMangaChapterAndPage()) tylko jakiś taki dziwny twór z ifami? o_O
- czemu te dziwne instrukcje wewnątrz pętli nie są objęte nową metodą? Ja na przykład nie do konca rozumiem co ten kod robi. Nie da sie go objąć metodą z sensowną nazwą?
Poprawiłem tą metodę na:
public override void StartDownloading()
{
if (!initializationException)
{
Download();
}
}
private void Download()
{
extractMangaName();
while (extractMangaChapter())
{
//selecting div which holds image and link to next URL
HtmlNode imgDiv = document.DocumentNode.SelectSingleNode("//img[@id='img']");
string imgLink = extractImageLink(imgDiv);
string nextURL = extractNextPageLink(imgDiv);
string filename = String.Format(@"{0}\{1}\{2}{3}",
localMangaPath, currentChapterNumber, currentPageNumber, Path.GetExtension(imgLink));
downloadImage(imgLink, filename);
mangaURL = nextURL;
initParser(mangaURL);
currentPageNumber++;
}
InfoString = "Manga has ended.\n" + InfoString;
}
Metoda dalej wydaje się dość długa, ale większość jej kodu to wywoływanie kolejnych metod. Nie miałem pomysłu, w jaki sposób bardziej to skrócić.
I dodałem odpowiednie metody:
private string extractImageLink(HtmlNode imgDiv)
{
string imgLink = imgDiv.Attributes["src"].Value.ToString();
return imgLink;
}
private string extractNextPageLink(HtmlNode imgDiv)
{
string nextUrl = "http://www.mangareader.net" + imgDiv.ParentNode.Attributes["href"].Value.ToString();
return nextUrl;
}
@Ktos
Poprawiłem właściwości, na prywatne i publiczne, według Twojego zalecenia:p
Poprawiłem nazwę gettingMangaChapterAndPage() na extractMangaChapter()
Zgodnie z Twoją sugestią zainteresowałem się String.Format i rzeczywiście zdecydowanie lepiej to działa w wypadku:
infoString = "Page " + pageNumber + " of " + chapterNumber + " chapter has been downloaded.\n" + infoString;
Zdecydowanie lepiej wygląda:
InfoString = String.Format("Page {0} of {1} chapter has been downloaded.\n{2}", currentPageNumber, currentChapterNumber, InfoString);
Path.Combine też wygląda nieźle, ale będę się nim bawił później.
@DibbyDum jeszcze raz
do
{
// ...
} while (gettingMangaChapterAndPage())
Nie działa w tym wypadku, gettinMangaChapterAndPage() (teraz już się nazywa extractMangaChapter()) musi być wywoływane na początku, jeśli użyję do while, musiałbym wywoływać tą metodę także przed pętlą. while() działa dobrze.
Trochę się rozpisałem, ale wytknęliście mi wiele błędów, dzięki Wam za to. Jeśli macie jeszcze jakieś sugestie, chętni je przeczytam.
zamiana znacznika <code class="csharp"> na <quote> - furious programming
Używam tej implementacji w 2 klasach: Manga i BaseMangaDownloader, w jaki sposób to zaimplementować, żeby obie klasy mogły z tego korzystać bez duplikacji kodu?
Ale to łatwo zobaczyć w swoim projekcie w folderze Model a tak naprawdę to są bardzie ViewModel jak by to odnieść do MVVM. Masz taką strykturę:
Więc widać jak na dłoni że możesz dziedziczyć obie klasy implementujące INotifyPropertyChanged z jednej i wtedy wyjdzie coś takiego:
Implementacje samej klasy znasz ale dla formalności:
public abstract class ViewModelBase : INotifyPropertyChanged
{
public event PropertyChangedEventHandler PropertyChanged;
protected virtual void OnPropertyChanged(string propName)
{
if (PropertyChanged != null)
{
PropertyChanged(this, new PropertyChangedEventArgs(propName));
}
}
}
Edit: A tak w ogóle masz już dwa śmieciowe projekty w solucji TataMYSQL i WpfApplication1 zrób i trzymaj porządek. ;)
Okej, dodałem tą klasę, dzięki.
Te dwa projekty były wywalone z solucji, nie wiem czemu dalej była o nich informacja, ale teraz już chyba powinno być czysto.
Przerobiłem aplikacje przy wykorzystaniu wzorca MVVM i wrzuciłem na BitBucketa (bardziej mi jakoś podszedł niż GitHub).
Byłbym wdzięczny gdybyście zajrzeli i ponownie skrytykowali mój kod:D Głównie chodzi mi o wzorzec MVVM, który dopiero zacząłem ogarniać.
https://bitbucket.org/Dessambe/mangacenter/src
Mam także dodatkowe pytanie - na podstawie mojej aplikacji moglibyście określić czy posiadam wystarczające umiejętności programistyczne, żeby zacząć szukać pracy w tej dziedzinie(w Krakowie, jeśli ma to jakieś znaczenie)? Oczywiście nie chodzi mi o jakąś super płatną robotę, wystarczyłaby mi najniższa pozycja, żeby tylko móc uczyć się programowania w trakcie pracy.
Wspomnę jeszcze, że posiadam licencjat z informatyki.
Wszystko w miarę okej co się tak naprawdę rzuca strasznie w oczy to Commands. Ja jestem zwolennikiem trzymania Commands w ViewModel, ale jeśli chcesz mogą być i osobno. Ale tak czy siak można to napisać prościej:
// W osobnym pliku napisz implementację: ICommand. Raz wystarczy. ;)
public class BaseCommand : ICommand
{
private readonly Action _command;
private readonly Func<bool> _canExecute;
public BaseCommand(Action command, Func<bool> canExecute = null)
{
if (command == null)
throw new ArgumentNullException("command");
_canExecute = canExecute;
_command = command;
}
public void Execute(object parameter)
{
_command();
}
public bool CanExecute(object parameter)
{
if (_canExecute == null)
return true;
return _canExecute();
}
public event EventHandler CanExecuteChanged;
}
// Później możesz swobodnie wykorzystywać gdzie chcesz. ;)
public class FooViewModel
{
private BaseCommand _saveCommand;
public ICommand SaveCommand
{
get { return _saveCommand ?? (_saveCommand = new BaseCommand(Save)); }
}
private void Save()
{
// ...
}
}