Witam wszystkich serdecznie.
Napisałem taki bardzo prosty projekt do zmniejszania obrazków. Wcześniej poczytałem trochę o MVP i postarałem się go wykorzystać. Czy mógłbym prosić o ocenę czy jest do dobrze/źle (chodzi głownie o wzorzec) co można poprawić, co dodać i takie ogólne spostrzeżenia odnoście kodu. Niżej zamieszczam zdjęcie GUI ( może więcej osób zobaczy kod :) )
Ech kolejny ;] Widok jest ok (chociaż ja osobiście podzieliłbym to na kawałki, np. zrobił osobną klasę z jakiegoś jednego panelu, osobną z innego, żeby było czytelniej), reszta nie. Prezenter służy TYLKO do tego żeby komunikować widok z modelem. Cała logika aplikacji to jest MODEL. Prezenter w idealnym świecie robi tylko tyle:
Załóżmy że piszesz aplikację która pozwala wczytać sobie z dysku plik Excela XLS i wyświetla go w jakiejś śmiesznej tabelce w okienku. Taki Excel dla ubogich. Widok oczywiście zawiera definicje jak wyświetlić tą tabelkę i ma guzik pozwalający załadować dane. Prezenter przechwytuje kliknięcie guzika następnie wywołuje na jakimś serwisie XLSFileLoader metodę loadFile(), odbiera z tej metody jakieś ExcelDataDTO które zawiera np. listę arkuszy, każdy arkusz zawiera listę wierszy, każdy wiersz listę komórek a każda komórka wartość. Ale to wszystko są proste obiekty / struktury przechowujące dane. Prezenter po odebraniu tych danych zleca Widokowi wyświetlenie ich. Koniec.
Cała logika związana z tym jak wczytać ten plik i jak rozkodować dane znajduje się w Modelu do którego zalicza się XLSFileLoader i wszystkie inne klasy z których korzysta żeby wykonać logikę "biznesową" aplikacji.
Dobrze patrzeć na to z perspektywy unit testów.
Generalnie w Widoku powinno nie być praktycznie niczego do testowania jednostkowego, bo nie powinno tam być żadnej logiki. Ot najwyżej jakiś prosty test weryfikujący czy ladujemy dane do odpowiedniej kontrolki.
Analognicznie w prezenterze nie bardzo jest co testować - możesz najwyżej sprawdzić czy prezenter woła odpowiednią/odpowiednie metody modelu/serwisów i czy zleca załadowanie odpowiedniego widoku. Ale znów, to jest raczej banalny test który testuje tylko prosty przepływ.
Faktyczne testy będą właśnie dla Modelu, bo tam siedzi cała logika aplikacji.
Jeśli chciałbyś pisać test dla Widoku bo np. masz tam skomplikowaną konwersję danych (np. z domenowych na obiekty do prezentacji) to znaczy że ta konwersja powinna wylecieć do modelu/osobnego serwisu.
Jeśli chciałbyś pisać test dla Prezentera bo wykonuje jakieś skomplikowane operacje to te operacje powinny wylecieć do modelu/osobnego serwisu.
Dzięki wielkie za odpowiedź :)
Postarałem się poprawić kod. Dużą cześć tego, co miałem w MainViewPresenter wywaliłem do osobnej klasy w modelu.
Czy teraz jest w miarę ok ? Jakbyś @Shalom mógł rzucić okiem to byłoby super :)
Tutaj link:
https://github.com/Astrocyt/ImageConverter
Pozdr.
Astrocyt
Jest trochę lepiej ale 3 uwagi:
ConvertAndSaveImage jeśli już chcesz mieć taką metodę to jej ciało powinno wyglądać tak:convert();
save();
;)
Dzięki ponownie :)
Postarałem się zastosować do Twoich porad a także zmieniłem parę rzeczy. Prezenter mocno mi się skrócił i jest tam o wiele mnie kodu niż na początku - teraz tylko wywołuje metody modelu i widoku ale coś zostało. W prezenterze jest pole z załadowanymi ścieżkami do zdjęć które są przekazywane do parametru metody konwertującej. To może tak zostać czy jest jakiś sposób aby się tego pozbyć ?
Zmieniłem także klasę
ImageConvertProperties
Dodałem tam właściwości pozwalające na pobieranie aktualnie dostępnych metod konwersji a także logika konwertowania stringa do Enum lub ImageFormat została przeniesiona do właściwości tylko do zapisu.
Tak to teraz wygląda :
/* jakies pola klasy */
public static string[] AvailableInterpolationModes
public static string[] AvalilableCompositionQuality
public static string[] AvalilableSmoothingModes
public static string[] AvailableImageFormats
public string InterpolationMode
public string CompositionQuality
public string SmoothingMode
public string ImageFormat
i widok teraz nie robi żadnego Enum.GetNames tylko coś takiego:
//pobieranie
ComboBox.Items.AddRange(ImageConvertProperties.AvalilableCompositionQuality)
//zapisywanie
_convertProperties.CompositionQuality = comboboxZQuality.SelectedItem.ToString();
Kontakt modelu z widokiem przebiega przez strukturę (tak jak wcześniej) tylko trochę to zmieniłem :
public void LoadImages(string path, bool deepSearch)
{
ImagesConverter converter = ImagesConverter.GetInstance();
string[] paths = converter.GetAllImagesPaths(path,deepSearch);
ImageInfo[] info = ImageInfo.CreateImageInfoFromPaths(paths);
this._loadedImages = paths;
this._view.ActualizeLoadedImages(info);
}
ImageConverter pobiera tylko ścieżki obrazków a ImageInfo już sobie to sam zamienia i jest przekazywane do widoku.
Co do 1 punktu to raczej póki co zostawię sobie go jak już ogarnę ten wzorzec lepiej ;)
Ale mam takie pytanie. Jak miała by wyglądać obsługa błędów w programie ? Model powinien wywalać błąd a presenter go obsługiwać ? A jeśli chciałbym wyświetlić jakąś informację gdzie występuje błąd np. błędna ścieżka w TextBox to w interface widoku powinna być jakaś metoda np. SourcePathError() i kiedy model wywala błąd to presenter go obsługuje i wywołuję tę metodę ? Nie wiem właśnie jak coś takiego zaimplementować ponieważ wydaje mi się to naiwne podejście - będzie bardzo dużo metod do obsługi błędów.
Tak więc cały kod jest na github.
Pozdrawiam
Astrocyt
ConvertImage a robi:Dzięki po raz kolejny :)
Zastosowałem się do porad. Dodałem instancję jako pole do presentera. Zmieniłem też parę rzeczy. Do ConvertProperties dodałem tak jak pisałeś właściwość ISizingMode. Teraz wygląda to tak :
public class ImageConvertProperties
{
public ISizingMode SizingMode
{
get
{
return CreateSizingMode();
}
}
ISizingMode CreateSizingMode()
{
ISizingMode sMode;
if(ratioMode)
{
sMode = new RatioMode(ratio);
}
else
{
sMode = new FixedSizeMode(width,height);
}
return sMode;
}
/* reszta kodu ... */
}
Teraz w klasie ImageConverter robię po prostu coś takiego :
private Image ConvertImage(Image sourceImage, ImageConvertProperties properties)
{
Image converted;
Size size = properties.SizingMode.GetNewSize(sourceImage.Width,sourceImage.Height);
converted = new Bitmap(size.Width,size.Height);
using (Graphics g = Graphics.FromImage(converted))
{
g.InterpolationMode = properties.interpolationMode;
g.SmoothingMode = properties.smoothingMode;
g.CompositingQuality = properties.compositionQuality;
g.DrawImage(sourceImage, 0, 0, size.Width, size.Height);
}
return converted;
}
Poza tym wywaliłem całkowicie metodę z 'And' i dodałem wszystko do taska czyli :
Task.Factory.StartNew(()=>{
for (int i = 0; i < imagesPaths.Length && !cancelTask.IsCancellationRequested; i++)
{
Image loadedImage = LoadImage(imagesPaths[i]);
Image convertedImage = ConvertImage(loadedImage, convertProperties);
string savePath = CreateSavePath(imagesPaths[i],
convertProperties.destonationPath,
convertProperties.imageFormat);
SaveImage(convertedImage, savePath);
if(ConvertingProgressChanged != null)
ConvertingProgressChanged(i/(double)imagesPaths.Length);
}
if(ConvertingComplete != null)
ConvertingComplete();
});
Ogólnie kod porozbijał się na mniejsze części i nawet łatwiej się w tym orientować :)
Do kodu chciałbym jeszcze dodać pytanie o to, co zrobić gdy plik o podanej nazwie istnieje. Planuję zrobić coś takiego :
Do ImageConverter dodać delegata który i event w stylu NameConflict(string name) i enum (CopyAndReplace,Replace,Ignore). Enum byłby wartością zwracaną zdarzenia. Event zarejestrować w presenter i odpalać jakiegoś Form'a . Z From pobierać enuma i zwracać go z metody presentera która jest zarejestrowana w zdarzeniu. Czy tak będzie dobrze ?
Pozdr.
Astrocyt
progressStep = calculateProgressStep(imagesPaths.Length);
for (int i = 0; i < imagesPaths.Length && !cancelTask.IsCancellationRequested; i++)
{
processImage(imagesPaths[i]);
updateProgress(progressStep);
}
finalizeConversion();