SOLID, SRP i Refactoring

Pierwsze dwa to jedne z ulubionych (a może dla niektórych znienawidzonych) skrótów w świecie IT. Trzecie to zagadnienie, które jest trochę jak UFO wszyscy o nim mówią, ale mało kto widział. Dzisiaj krótko, ale istotnie na ten temat w Social Cooking.

Na początek krótkie tło teoretyczne.

SOLID – zbiór zasad zaproponowany przez Roberta C. Martina opisujący zasady programowania obiektowego. Zmora programistów, ulubieniec rekruterów 🙂

Single responsibility principle (zasada jednej odpowiedzialności) – zasada, która wymusza na programistach tworzenie metod w taki sposób, aby każda z nich odpowiadała tylko jednej czynności. Jeżeli tak nie jest należy rozbić metodę na kilka mniejszych

Open/closed principle – tworzone oprogramowanie powinno być otwarte na nowe funkcje, ale zamknięte na modyfikacje

Liskov substitution principle – obiekty w oprogramowaniu powinny pozwalać na podstawienie w ich miejsce instancji klas pochodnych bez zaburzenia prawidłowego działania systemu

Interface segregation principle – klient nie powinien być zależny od metod, których nie używa

Dependency inversion principle – wysokopoziomowe moduły nie powinny być zależne od niskopoziomowych. Zależności powinny wynikać z abstrakcji

Przypominając sobie to wszystko niech pierwszy rzuci kamieniem, który myśli o tym wszystkim za każdym razem kiedy zajmuje się nowym projektem. Część faktycznie po pewnym czasie jest stosowana niemalże nieświadomie, albo rutynowo jak na przykład ostatnia reguła w moim przypadku. Podobnie sprawa się ma z interfejsami. Ale…

Kiedy piszę coś nowego, kiedy nie mogę się doczekać, aż coś zacznie działać, kiedy jestem już tak blisko rozwiązania problemu, to niestety nie myślę już o trzech pierwszych zasadach. Szczerze mówiąc trochę głupio się przyznać, ale z pierwszą mam zazwyczaj najwięcej problemów. Może nie tworzę jeszcze gigantów jakie już zdarzało mi się widywać przy okazji niektórych projektów (uśmiech dla kolegów od kart 🙂 ), ale dzielenie metod zawsze przychodzi mi z trudnością.

Dlatego też zapewne ostatnio została mi zwrócona uwaga dotycząca metody w kontrolerze. Mowa tu dokładnie o tej metodzie:

[HttpPost]
public async Task UploadImage()
{
if (!Request.Content.IsMimeMultipartContent())
{
this.Request.CreateResponse(HttpStatusCode.UnsupportedMediaType);
}
var uploadFolder = "~/Resource/ImageFiles";
var provider = GetMultipartProvider(uploadFolder);
var result = await Request.Content.ReadAsMultipartAsync(provider);

var originalFileName = GetDeserializedFileName(result.FileData.First());

var uploadedFileInfo = new FileInfo(result.FileData.First().LocalFileName);

var username = GetFormData(result);
var destDir = uploadFolder + "/" + username;
var destPath = destDir + "/" + originalFileName;

var destLocalDir = HttpContext.Current.Server.MapPath(destDir);
var destLocalPath = HttpContext.Current.Server.MapPath(destPath);
string host = Request.GetRequestContext().Url.Request.Headers.Host;
var destThumbPath = destDir + "/" + originalFileName.Split('.')[0] + "_thumb." +
originalFileName.Split('.')[1];

var destLocalThumbPath = HttpContext.Current.Server.MapPath(destThumbPath);

Directory.CreateDirectory(destLocalDir);
File.Move(uploadedFileInfo.FullName, destLocalPath);
ResizeSettings settings = new ResizeSettings("width=100&height=100");
ImageBuilder builder = ImageBuilder.Current;

builder.Build(destLocalPath, destLocalThumbPath, settings);
var user = await
_usersRepo.SaveProfileImagePath(username, destPath.Replace("~", "http://"+host), destThumbPath.Replace("~", "http:/ /" + host));
var returnData = "ImageUploaded";
return this.Request.CreateResponse(HttpStatusCode.OK, new { returnData });
}

No faktycznie można się w tym wszystkim pogubić. Już nie wspominając o strasznych hardcodach, które aż rażą.

Zdawałem sobie sprawę z tego jak to wygląda i jak jest tragiczne. Mówiłem sobie jak zwykle, że później do tego wrócę, zrefactoruję, upiększę. Ale cholera jeszcze nigdy tego nie zrobiłem. Ciężko mi wrócić do czegoś co już teoretycznie działa i poprawić czytelność kodu. Czasem się zastanawiam czy to moje lenistwo ma taki wpływ na moje podejście, czy może to w jakich firmach pracuję i gdzie nigdy nie ma czasu na refactoring. Jak coś działa to nie rusz, bo po pierwsze primo możesz zepsuć, a po drugie primo za ładny kod nam nie płacą.

No, ale jak już ktoś publicznie mi wytyka to, że to jest słabe i dla mojej własnej późniejszej wygody mógłbym to poprawić no to zmienia postać rzeczy. W takich sytuacjach trzeba przemyśleć swoje postępowanie, przyznać się do błędu i poprawić co ma być poprawione.

Teraz w związku z tym wygląda to tak:

[HttpPost]
public async Task<HttpResponseMessage> UploadImage()
{
string destPath;
string destLocalPath;
string destThumbPath;
string destLocalThumbPath;

if (!Request.Content.IsMimeMultipartContent())
{
this.Request.CreateResponse(HttpStatusCode.UnsupportedMediaType);
}
var uploadFolder = "~/Resource/ImageFiles";
var provider = GetMultipartProvider(uploadFolder);
var result = await Request.Content.ReadAsMultipartAsync(provider);

var originalFileName = GetDeserializedFileName(result.FileData.First());

var username = GetFormData(result);

PrepareDestinationPaths(out destPath, uploadFolder, username, originalFileName, out destLocalPath, out destThumbPath, out destLocalThumbPath);

MoveFileToProperDest(result, destLocalPath);
await CreateThumb(destLocalPath, destLocalThumbPath, username, destPath, destThumbPath);
var returnData = "ImageUploaded";
return this.Request.CreateResponse(HttpStatusCode.OK, new { returnData });
}

Czy jest już idealnie?

Na pewno nie.

Czy jest lepiej?

Zdecydowanie. Na pewno jest jeszcze pole do popisu, ale wydaje mi się, że metoda jest już zdecydowanie bardziej czytelna i ktokolwiek na nią spojrzy będzie +/- widział o co chodzi.

Co dalej?

Dalej będę się starał pisać wolniej i bardziej dokładnie 🙂 Muszę znaleźć nieco cierpliwości przed komputerem. Trzymajcie za mnie kciuki.

Related posts

  • Pingback: dotnetomaniak.pl()

  • Jeśli mogę coś podpowiedzieć, to np. uploadFolder można pewno do jakiejś konfiguracji dać. Zwracanie wyniku funkcji przez out też nie jest dobrą praktyką. Od zwracania wyniku z funkcji jest return.
    No i tak w ogóle, to całość kodu związanego z obsługą plików warto byłoby wydzielić z kontrolera do innej klasy. Bez tego nie ma mowy o SRP.

    • Kajetan Duszyński

      Faktycznie później sobie o tym wszystkim jeszcze myślałem i doszedłem do podobnych wniosków. Na pewno muszę out’y wywalić (może jakiś model ze ścieżkami zrobię).
      Też zgodzę się z tym, że trzeba wywalić tę logikę z kontrolera. Muszę się jednak przyznać, że nie do końca mam pomysł jak to logicznie rozdzielić. Jakieś helpery? Co Ty na to?

      • No też bym pomyślał o jakimś pomocniczym serwisie. No, i być może jak go zrobisz, to model ze ścieżkami nie będzie potrzeby, po po prostu będą one polami nowej klasy, które zainicjalizujesz w konstruktorze. 🙂