Factory jest niezalecana w C#; co zrobić zamiast tego?

Problem jest technicznie łatwy dla mnie do zrobienia tak, żeby działało. Ale już nie wiem, jak to rozwiązać tak, by nie było code smellów.

Czytam z dokumentacji Microsoftu:

Microsoft napisał:

• Avoid using the service locator pattern. For example, don't invoke GetService to obtain a service instance when you can use DI instead.
• Another service locator variation to avoid is injecting a factory that resolves dependencies at run time. Both of these practices mix Inversion of Control strategies.

Ma to sens o tyle, o ile w Factory zazwyczaj występuje new, a o ile pamiętam Microsoft naciskał, by używać ichniego DI zamiast new (i zamiast static ofc).

No ale w takim razie czego użyć zamiast Factory?

Przykładowa sytuacja (od której teraz się odbiłem): Zamawianie kuriera. DHL obsługuje sandbox, który tym się różni od nie-sandboxa, że requesty do sandboxa dają odpowiedzi dokładnie takie same, jak do serwisu "normalnego", jednakże nie powodują skutków w rodzaju przyjazdu kuriera pod drzwi o określonej porze. Sandbox jest do testowania.

Niestety DHL nie był łaskaw wyabstrahować tego. Mam klasy DHL.DHL24WebapiClient oraz SandboxDHL.DHL24WebapiClient, które udostępniają dokładnie takie same metody, parametry, typy danych - niestety takie same, a nie te same. Klasy te nie implementują żadnego wspólnego interfejsu.

A ja mam w runtime, na podstawie danych wejściowych, a nie w compile time zdecydować, czy chcę wysłać request do sandboxa, czy do "normalnego" API DHLa.

public class DhlWrapper
{
    private readonly DHL.DHL24WebapiClient client;
    private readonly SandboxDHL.DHL24Webapi clientSandbox;
    public DhlWrapper(DHL.DHL24WebapiClient client, SandboxDHL.DHL24Webapi clientSandbox) =>
        (this.client, this.clientSandbox) = (client, clientSandbox);

    public async Task<BasicShipmentData> CreateShipments(AuthData auth, ShipmentFullData shipmentFullData, bool sandbox=false) =>
        sandbox ? await clientSandbox.CreateShipments(auth, shipmentFullData) : await client.CreateShipments(auth, shipmentFullData);

    public async Task<DeleteShipmentResult> DeleteShipments(AuthData auth, string[] shipments, bool sandbox=false) =>
        sandbox ? await clientSandbox.DeleteShipments(auth, shipments) : await client.DeleteShipments(auth, shipments);

    public async Task BookCourier(AuthData auth, /* jeszcze kilka parametrów */, bool sandbox = false) =>
        sandbox ? await clientSandbox //.. nie chce mi się dalej pisać, jest jasne co się tu dzieje

    // i jeszcze dobre trochę metod
}

Takie rozwiązanie jak to powyżej wydaje mi się być brzydkie?

Ale może właśnie tak, jak powyżej należy pisać? Bo alternatywa, jaką widzę, to factory pattern, która jest niezalecana przez Microsoft.

Niestety DHL nie był łaskaw wyabstrahować tego. Mam klasy DHL.DHL24WebapiClient oraz SandboxDHL.DHL24WebapiClient, które udostępniają dokładnie takie same metody, parametry, typy danych - niestety takie same, a nie te same. Klasy te nie implementują żadnego wspólnego interfejsu.
W takich sytuacjach pisze się adaptery, które implementują wspólny interfejs.

Co do samego użycia factory - wydaje mi się, że troche nadinterpretujesz to co jest tam napisane. W samym factory nie ma nic złego. W wielu przypadkach jest bardzo wskazane, zwłaszcza gdy tworzymy obiekty IDisposable. Tu chodzi o to, żeby fabryki nie rozwiązywały zależności w runtimie.

czesiek napisał(a):

Tu chodzi o to, żeby fabryki nie rozwiązywały zależności w runtimie.

A czy tu nie ma właśnie rozwiązania zależności w runtime? Używamy sandboxa albo "normalnego" DHLa w zależności od tego, co wiemy dopiero w runtime.

czesiek napisał(a):

W takich sytuacjach pisze się adaptery, które implementują wspólny interfejs.

Chodzi Ci o mój pierwszy snippet, czy raczej o coś takiego jak niżej?

public interface IDHL
{
    Task<BasicShipmentData> CreateShipments(AuthData auth, ShipmentFullData shipmentFullData);
    Task<DeleteShipmentResult> DeleteShipments(AuthData auth, string[] shipments);
    Task BookCourser(AuthData auth, /* i jeszcze kilka parametrów*/);
    // I jeszcze dobre trochę metod
}

public class DHL : IDHL
{
    private readonly DHL.DHL24WebapiClient client;
    public DHL(DHL.DHL24WebapiClient client) =>
        this.client = client;

    public async Task<BasicShipmentData> CreateShipments(AuthData auth, ShipmentFullData shipmentFullData) =>
        await client.CreateShipments(auth, shipmentFullData);

    public async Task<DeleteShipmentResult> DeleteShipments(AuthData auth, string[] shipments) =>
        await client.DeleteShipments(auth, shipments);

    public async Task<BookCourier> (AuthData auth, /* i jeszcze kilka parametrów */) =>
        await client.BookCourier(auth, /* ... */);

    // I jeszcze dobre trochę metod
}

public class SandboxDHL : IDHL
{
    // Copy - paste z ciała public class DHL
}

czesiek napisał(a):

Tu chodzi o to, żeby fabryki nie rozwiązywały zależności w runtimie.

Co wiąże się z tym, że konstruktor rośnie liniowo względem możliwych ścieżek, niech będzie UpsClient, PocztexClient, DhlClient itp, a my wykorzystujemy zawsze tylko jedną z nich, więc n - 1 musimy zdiscardować. Dla uwagi załóżmy, że wszystko jest Transient i jest tworzone za każdym razem. Mi to nie brzmi jak dobry plan.

YetAnohterone napisał(a):

czesiek napisał(a):

Tu chodzi o to, żeby fabryki nie rozwiązywały zależności w runtimie.

A czy tu nie ma właśnie rozwiązania zależności w runtime? Używamy sandboxa albo "normalnego" DHLa w zależności od tego, co wiemy dopiero w runtime.

Mi bardziej chodziło o zależności do tworzonych obiektów. Czyli jeśli DHL.DHL24WebapiClient ma zależności to nie powinny być rozwiązywane w runtimie. Ale mogę się mylić :)
Samo wywołanie konstruktora DHL.DHL24WebapiClient w factory nie wydaje mi się niepoprawne.

Chodzi Ci o mój pierwszy snippet, czy raczej o coś takiego jak niżej?

O coś takiego jak poniżej ;)

Trochę namieszałem, więc napisze jeszcze raz jak to powinno wyglądać z mojej perspektywy.

Guideline MS wyraźnie zaznacza "When using DI is possible". Jeśli wybrana zależność jest rozwiązywana w runtimie, to musimy albo ją stworzyć sami, albo użyć locatora.

Z mojej perspektywy powinieneś utworzyć factory i:

1. Jeśli chcesz tworzyć klientów z przez kontener IoC to wstrzykujesz do fabryki ServiceLocator i tam rozwiązujesz zależności w zależności (:P) od wyboru użytkownika/konfiguracji itp.
2. Jeśli chcesz sam tworzyć obiekty, wywołujesz konstruktor (jeśli tu się pojawiają zależności, to się robi problem i wybrałbym opcje 1).

Chętnie poczytam jeśli ktoś zna inne praktyczne rozwiązanie. Od biedy możnaby wstrzykiwać jeszcze Lazy<> zależności do factory, ale wydaje mi się to krzywe.

U Ciebie w tej klasie widzę następujący problem - przekazujesz czy to sandbox w każdej metodzie. Tak nie powinno być, to powinno być ustalone odgórnie. Czyli np. przychodzi request do serwera i ma w headerze X-MyCompany-Sandbox: True to powinien jakiś middleware/filtr to przechwycić i ustawić w kontenerze globalny stan per request ioc.dawaj(PerRequestConfiguration).useSandbox() i dalej w kontenerze powinien być zaszyty wybór jak jest ta flaga sandbox to wstrzykują się klasy z sandboxem (to już niestety fabryką).

Na moje oko jak ten wybór zrobisz przez factory to nic się nie stanie. Nie bierz wszystkiego co jest w docach od MS na słowo. Zwłaszcza tego typu twierdzenia są podane przez uzasadnienia. Jak masz wątpliwości do napisz do nich email / dodaj komentarz. Tutaj przykład jak to można zrobić w Autofac: https://benedict-chan.github.io/blog/2014/08/13/resolving-implementations-at-runtime-in-autofac/

Ja mam taką zasadę kontener IoC powinien być tak głupi jak się tylko da. Można się też bez niego obejść. Kontener != IoC. Idea IoC jest taka że klasa jawnie mówi czego potrzebuje, że nie ma ukrytych zależności. Wg. mnie nie kłóci się to z ideą fabryki, w dobrym kontenerze to można nawet zdefiniować sobie fabrykę jako źródło komponentu. Domyślam się że kontener IoC od MS jest wybrakowany i tam fabryka ma taki sens że trzeba service locatorem ściągać właściwe komponenty z kontenera. Ale nich słabość ich implementacji nie przesłania Ci dobrych praktyk.

Wpadł mi do głowy pomysł, który przynajmniej jest zgodny z literą zaleceń MS

Zakładam, że mamy:

public interface IDHL
{
    Task<BasicShipmentData> CreateShipments(AuthData auth, ShipmentFullData shipmentFullData);
    Task<DeleteShipmentResult> DeleteShipments(AuthData auth, string[] shipments);
    Task BookCourser(AuthData auth, /* i jeszcze kilka parametrów*/);
    // I jeszcze dobre trochę metod
}

public class DHL : IDHL
{
    private readonly DHL.DHL24WebapiClient client;
    public DHL(DHL.DHL24WebapiClient client) =>
        this.client = client;
    /*
     *  blablabla
    */
}

public class SandboxDHL : IDHL
{
    // Copy - paste z ciała public class DHL
}

To teraz użycie tego:

public interface IZamawiaczPaczki // później pomyślę nad angielską nazwą
{
    Task ZamówPaczkę(blablabla);
}

public class ZamawiaczPaczki<D> : IZamawiaczPaczki where D : IDHL // później pomyślę nad angielską nazwą
{
    private readonly D dhl;
    public ZamawiaczPaczki(D dhl) =>
        this.dhl = dhl;

    public async Task ZamówPaczkę(blablabla)
    {
        blablabla;
        await dhl.CreateShipments(blablabla);
        await dhl.BookCourier(blablabla);
        blablabla;
    }
}

No i wreszcie: (skoro fabryka zabroniona):

public class DHLModel : PageModel
{
    private readonly ZamawiaczPaczki<DHL> dhl;
    private readonly ZamawiaczPaczki<SandboxDHL> sandbox;

    public DHLModel(ZamawiaczPaczki<DHL> dhl, ZamawiaczPaczki<SandboxDHL> sandbox) =>
        (this.dhl, this.sandbox) = (dhl, sandbox);

    public async Task OnPost(/*blabla*/)
    {
        IZamawiaczPaczki zamawiacz = IsSandbox() ? sandbox : dhl;
        await zamawiacz.ZamówPaczkę(blablabla);
    }
}

(Tak naprawdę to nie mam tu strony internetowej, ale for the sake of example)

Wybór implementacji jest tu w najwcześniejszym możliwym momencie, więc chyba OK.

Szczerze - zupełnie nie wiem jaki problem tym sposobem rozwiązujesz. W Twoim wysokopoziomowym Page Modelu masz zależność do niskopoziomowych, konkretnych implementacji (Dhl, SandboxDHL). Generyczny Zamawiacz raczej nic nie wnosi (równie dobrze mógłbaby to być niegeneryczna klasa, która w konstruktorze przyjmuje Twoje IDHL. Pomyśl, co by było gdybyś chciał tu dodać wiecej implementacji interfejsu do shippingu. Albo użyć ich w większej ilośći miejsc. Wszędzie musisz utworzyć instancje każdego klienta i "wyifować", którego użyć.

Cały czas trzymasz się tego, że "fabryka jest zabroniona", co jest z gruntu błędne. Zobacz - na tej samej stronie, z której wyciąłeś to zdanie, masz sugestie implementacji Factory, która w konstruktorze przyjuje IServiceProvider: https://docs.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines#transient-limited-lifetime

Dobra, zatem z fabryką

Zakładam, że mamy:

public interface IDHL
{
    Task<BasicShipmentData> CreateShipments(AuthData auth, ShipmentFullData shipmentFullData);
    Task<DeleteShipmentResult> DeleteShipments(AuthData auth, string[] shipments);
    Task BookCourser(AuthData auth, /* i jeszcze kilka parametrów*/);
    // I jeszcze dobre trochę metod
}

public class DHL : IDHL
{
    private readonly DHL.DHL24WebapiClient client;
    public DHL(DHL.DHL24WebapiClient client) =>
        this.client = client;
    /*
     *  blablabla
    */
}

public class SandboxDHL : IDHL
{
    // Copy - paste z ciała public class DHL
}

Fabryka:

public class DHLFactory
{
    public IDHL GetDHL(bool sandbox) =>
        sandbox ? new SandboxDHL() : new DHL();
}

To teraz użycie tego:

public class ZamawiaczPaczki
{
    private readonly IDHL dhl;
    public ZamawiaczPaczki(IDHL dhl) =>
        this.dhl = dhl;

    public async Task ZamówPaczkę(blablabla)
    {
        blablabla;
        await dhl.CreateShipments(blablabla);
        await dhl.BookCourier(blablabla);
        blablabla;
    }
}

No i wreszcie:

public class DHLModel : PageModel
{
    private readonly DHLFactory dhlFactory;

    public DHLModel(DHLFactory dhlFactory) =>
        this.dhlFactory = dhlFactory;

    public async Task OnPost(/*blabla*/)
    {
        await new ZamawiaczPaczki(dhlFactory.GetDhl(IsSandbox())).ZamówPaczkę(blablabla);
    }
}

Tak należy to zrobić, czy coś tu zrypałem?

(Zakładam, że nie mogę ustalić, czy jest sandbox w jakimś middleware czy w ogóle wcześniej, niż w event handlerze. Tak jak pisałem, to tak naprawdę nie jest mikroserwis ani w ogóle strona internetowa.)

EDIT: Tu jest użycie new, i to dwukrotne, czego nie zaleca MS jako rzekomy tight coupling, ale mogę uznać, że MS bzdury opowiada

Nie, wróć, bzdura, ZamawiaczPaczki też trzeba zfactoryzować, wtedy DHLModel będzie mogła przyjąć factory w konstruktorze przez DI i usuniemy jedno new

public class ZamawiaczPaczkiFactory
{
    public ZamawiaczPaczki GetZamawiaczPaczki(bool sandbox) =>
        new ZamawiaczPaczki(dhlFactory.GetDHL(sandbox));
}

I użycie:

public class DHLModel : PageModel
{
    private readonly ZamawiaczPaczkiFactory zamawiaczFactory

    public DHLModel(ZamawiaczPaczkiFactory zamawiaczFactory) =>
        this.zamawiaczFactory = zamawiaczFactory;

    public async Task OnPost(/*blabla*/)
    {
        await zamawiaczFactory.GetZamawiaczPaczki(IsSandbox()).ZamówPaczkę(blablabla);
    }
}

Świąteczny czas nie sprzyja myśleniu i dyskusjom, ale moja propozycja.

    /// <summary>
    /// Zamiast tworzyć klasę na factory można wstrzykiwać Func<>, a lepiej delgata, który nadaje temu Func<> kontekst
    /// </summary>
    public delegate IClient ClientFactory(HttpContext httpContext);

    public interface IClient
    {
        void DoSth();
    }

    public class SandboxClient : IClient
    {
        public void DoSth()
        {
            throw new NotImplementedException();
        }
    }

    public class ProductionClient : IClient
    {
        public void DoSth()
        {
            throw new NotImplementedException();
        }
    }

    public static class ConditionBasedResolutionControllerExtensions
    {
        public static IServiceCollection RegisterClients(this IServiceCollection services)
        {
            services.AddScoped<SandboxClient>();
            services.AddScoped<ProductionClient>();
            services.AddScoped<ClientFactory>(serviceProvider =>
            {
                /// w moim przykładzie warunkujemy poprzez HttpContext, ale w tym momencie mamy dostęp do całego kontenera
                /// więc równie dobrze można ograć jakieś feature switchery operujące na globalnym stanie
                /// albo nawet wyciągać go z zewnątrz (szaleństwo nie ma końca)
                return httpContext =>
                {
                    return httpContext.Request.Headers["env"].Any(x => x == "sandbox")
                        ? serviceProvider.GetRequiredService<SandboxClient>()
                        : serviceProvider.GetRequiredService<ProductionClient>();
                };
            });

            return services;
        }
    }

    [ApiController]
    [Route("[controller]")]
    public class ExampleController : ControllerBase
    {
        private readonly ClientFactory clientFactory;

        public ExampleController(ClientFactory clientFactory)
        {
            this.clientFactory = clientFactory;
        }

        [HttpGet]
        public IActionResult Get()
        {
            var client = clientFactory(HttpContext);
            client.DoSth();
            return Ok();
        }
    }

YetAnohterone napisał(a):

Wydaje mi się, że jeśli informacja o tym, czy jest sandbox jest nie w nagłówku, tylko w ciele żądania, to tak chyba nie da się tego (łatwo, "czysto") zrobić?

Nie tylko się da, ale robi się to tak samo. Dokładnie tak samo.

Niech

public class SomeBody 
{
    public string Env {get;set;}
}

...
[HttpPost]
public IActionResult Post(SomeBody body)
{
    var client = clientFactory(body.Env);
    client.DoSth();
    return Ok();
}

Wszystko jest pod kontrolą, można zmieniać typ funkcji na cokolwiek co nam pasuje. Może być nawet Func<IClient> i warunkowanie po dniu tygodnia (FridayDiscountClient). Jedyne co jest kwestią dyskusji to czy tak powinno się robić, bo o ile wg mnie wykorzystanie tego później jest OK, o tyle część logiki zamykamy w kontenerze DI.

Jasne, działa, chwilowe zaćmienie umysłu.

Side note: Pytałem o to na C#owym Discordzie. Stwierdzili, że fabryka ma prawo bytu i może jawnie stworzyć obiekt, który ma stworzyć, zaś ten fragment z dokumentacji, który zacytowałem w OP:

Another service locator variation to avoid is injecting a factory that resolves dependencies at run time. Both of these practices mix Inversion of Control strategies.

Tam miało chodzić tylko o to, by fabryka prosiła w konstruktorze o zależności obiektu, który ma stworzyć, zamiast tworzyć te zależności jawnie.

Czyli: To jest OK:

public class ZamawiaczPaczkiFactory
{
    private IDHL sandbox, normal;
    public ZamawiaczPaczkiFactory(DHL normal, SandboxDHL sandbox) =>
        (this.sandbox, this.normal) = sandbox, normal);
    public ZamawiaczPaczki Create(bool isSandbox) =>
        isSandbox ? new ZamawiaczPaczki(this.sandbox) : new ZamawiaczPaczki(this.normal);
}

Nie-OK byłoby, gdyby ZamawiaczPaczkiFactory tworzyła sobie DHL normal lub SandboxDHL sandbox za pomocą new lub GetService i przed tym przestrzega dokumentacja MS.

A więc istotnie źle rozumiałem tę rekomendację

YetAnohterone napisał(a):

Side note: Pytałem o to na C#owym Discordzie.

A to jest jakiś oficjalny Discord C#?

YetAnohterone napisał(a):

Czyli: To jest OK:

public class ZamawiaczPaczkiFactory
{
    private IDHL sandbox, normal;
    public ZamawiaczPaczkiFactory(DHL normal, SandboxDHL sandbox) =>
        (this.sandbox, this.normal) = sandbox, normal);
    public ZamawiaczPaczki Create(bool isSandbox) =>
        isSandbox ? new ZamawiaczPaczki(this.sandbox) : new ZamawiaczPaczki(this.normal);
}

Nie-OK byłoby, gdyby ZamawiaczPaczkiFactory tworzyła sobie DHL normal lub SandboxDHL sandbox za pomocą new lub GetService i przed tym przestrzega dokumentacja MS.

A więc istotnie źle rozumiałem tę rekomendację

No i wniosek z tego, że to co pokazałem u góry jest tym złym rozwiązaniem (bo tak naprawdę to factory, które tworzy rzeczy przez GetService), a to w co wątpiłem (i nadal wątpię) tutaj https://4programmers.net/Forum/1815934 jest OK. Mnie to nie przekonuje, chyba trzeba ankietę zrobić, ostatnio to popularny sposób rozwiązywania konfliktów tutaj.

@Saalin: Zgadzam się. Żadnego z tych obiektów nie powinniśmy moim zdaniem tworzyć z poziomu kontenera DI, bo wszystkie pośrednio zależą od parametru przekazanego w runtimie:

1. Factory zwracające new DHL() albo new SandboxDHL():

public interface IDhlFactory{
    IDhl Create(bool isSandbox);
}

2. Factory zwracające new ZamawiaczPaczki(IDhl).

public interface IZamawiaczPaczkiFactory{
    IZamwiaczPaczki Create(IDhl dhlClient);
}

Polecam lekturę: https://stackoverflow.com/a/2280289

Zróbmy jeszcze factory robiące te factory, kompilator wytrzyma. ;]

Skoro jeden zamawiacz paczki jest uwiązany do DHLowej produkcji, a drugi do sandboxa, to po co tak wydziwiać? Wystarczy jedno factory przyjmujące isSandbox i zwracające odpowiednio skonfigurowanego Zamawiacza.

YetAnohterone napisał(a):

Side note: Pytałem o to na C#owym Discordzie. Stwierdzili, że fabryka ma prawo bytu i może jawnie stworzyć obiekt

"Może"?! Chyba raczej musi. :P
I to nie jest żaden tight coupling, że fabryka produkuje obiekty, które ma produkować.

Saalin napisał(a):

No i wniosek z tego, że to co pokazałem u góry jest tym złym rozwiązaniem (bo tak naprawdę to factory, które tworzy rzeczy przez GetService), a to w co wątpiłem (i nadal wątpię) tutaj https://4programmers.net/Forum/1815934 jest OK. Mnie to nie przekonuje, chyba trzeba ankietę zrobić, ostatnio to popularny sposób rozwiązywania konfliktów tutaj.

No i w takim układzie nie wiadomo, co jest potrzebne do tworzenia obiektów, nie da się napisać normalnych testów jednostkowych, a jednym średnim refactoringiem można wyłożyć aplikację i wpędzić się w wiele godzin poszukiwania jakichś magicznych, niejawnie przekazywanych zależności.

Saalin napisał(a):

No i wniosek z tego, że to co pokazałem u góry jest tym złym rozwiązaniem (bo tak naprawdę to factory, które tworzy rzeczy przez GetService), a to w co wątpiłem (i nadal wątpię) tutaj https://4programmers.net/Forum/1815934 jest OK. Mnie to nie przekonuje, chyba trzeba ankietę zrobić, ostatnio to popularny sposób rozwiązywania konfliktów tutaj.

Tak, w tym co pokazałem, przy odpowiednim podniesieniu poziomu skompilikowania możemy uzyskać złożoność rozwiązywania zależności kwadratową względem ilości klas. (chociaż to już byloby chyba przy mocnym spaghetti dopiero)

Ale może jest rozwiązanie? Wpadło mi do głowy ostatnio. Złożoność rozwiązywania zależności liniowa względem głębokości zagnieżdżenia, a nie ilości możliwych implementacji danego interfejsu. A chyba zgodne z zaleceniami MS w podanej wyżej interpretacji:

public class ZamawiaczPaczkiFactory
{
    private DHLFactory dhlFactory;
    public ZamawiaczPaczkiFactory(DHLFactory dhlFactory) =>
        this.dhlFactory = dhlFactory;
    public ZamawiaczPaczki Create(bool isSandbox) =>
        new ZamawiaczPaczki(dhlFactory(isSandbox));
}

public class DHLFactory
{
    public IDHL Create(bool isSandbox) =>
        isSandbox ? new SandboxDHL() : new DHL(); // czy tam getservice, w tej fabryce wolno
}

Chyba też zgodne z tym, co napisał wyżej @somekind ? (z perspektywy klienta jest jedna fabryka, klient nie musi dawać kolejnej fabryki do Create)

A to jest jakiś oficjalny Discord C#?

https://discord.gg/csharp

Oficjalny może nie jest, sami piszą, że są not affiliated or working with Microsoft, ale chyba jest to de-facto standard

Nie wiem po co dwie fabryki, skoro jest tylko jedna rzecz jest zależna od parametrów runtime.

Po to, żeby nie tworzyć zbędnych obiektów. Jeśli jest tylko ZamawiaczPaczkiFactory, to musi ona albo utworzyć jeden obiekt implementujący IDHL za pomocą new albo GetService (co jest podobno niezalecane), albo zamówić sobie wszystkie możliwe implementacje IDHL w konstruktorze, pomimo że tak naprawdę potrzebuje tylko jednej (co może nie ma znaczenia w tak prostym przykładzie, ale przynajmniej teoretycznie takie postępowanie może prowadzić do asymptotycznie złej złożoności rozwiązywania zależności).

zamówić sobie wszystkie możliwe implementacje IDHL w konstruktorze, pomimo że tak naprawdę potrzebuje tylko jednej

No zaraz, jeśli te implementacje są zbędne, to po co je w ogóle implementować? :P Myślałem, że cały czas dyskutujemy o sytuacji, w której wszystkie implementacje są potrzebne, a w runtime wybierana jest jedna z nich.
W tym układzie ZamawiaczPaczkiFactory na podstawie isSandbox może utworzyć ZamawiaczPaczki z odpowiednim DHL24WebapiClient i tyle. To właśnie oba rodzaje DHL24WebapiClient powinny być zależnościami tej fabryki.

A new jest normalnym i standardowym sposobem tworzenia obiektów.

No zaraz, jeśli te implementacje są zbędne, to po co je w ogóle implementować? :P Myślałem, że cały czas dyskutujemy o sytuacji, w której wszystkie implementacje są potrzebne, a w runtime wybierana jest jedna z nich.

Tak. W runtime wybierana jest jedna z nich. JEDNA. A zatem, w runtime wiadomo, że pozostałe są zbędne.

public class SomeServiceFactory
{
    private readonly IWTF wtf1, wtf2, wtf3, wtf4, wtf5, /*...*/ wtfn;
    public SomeServiceFactory(Wtf1 wtf1, Wtf2 wtf2, Wtf3 wtf3, Wtf4 wtf4, Wtf4 wtf5, /*...*/ WtfN wtfn) =>
        (this.wtf1, this.wtf2, this.wtf3, this.wtf4, this.wtf5, /*...*/ this.wtfn) = (wtf1, wtf2, wtf3, wtf4, wtf5, /*...*/ wtfn);
    public SomeService Create(Blabla blabla) =>
        new SomeService(
            Is1(blabla) ? wtf1 :
            Is2(blabla) ? wtf2 :
            Is3(blabla) ? wtf3 :
            Is4(blabla) ? wtf4 :
            Is5(blabla) ? wtf5 :
            /* .... */
            IsN(blabla) ? wtfn :
            throw new ArgumentException("Brak dostępnej implementacji.")
        );
}

Tworzymy n implementacji IWTF, mimo że teraz potrzebujemy tylko jednej. W ogóle nie możemy obejść się bez pozostałych implementacji, bo późniejsze wywołanie fabryki może zwrócić inną implementację, ale to wywołanie potrzebuje tylko jednej.

A teraz załóżmy, że każda implementacja IWTF zamawia sobie m kolejnych zależności. Widać, że koszt zbudowania fabryki to n×m. Co najmniej.

public class SomeServiceFactory
{
    private IServiceProvider serviceProvider;
    public SomeServiceFactory(IServiceProvider serviceProvider) =>
        this.serviceProvider = serviceProvider.
    public SomeService Create(Blabla blabla) =>
        new SomeService(
            Is1(blabla) ? serviceProvider.GetRequiredService<Wtf1>() :
            Is2(blabla) ? serviceProvider.GetRequiredService<Wtf2>() :
            Is3(blabla) ? serviceProvider.GetRequiredService<Wtf3>() :
            Is4(blabla) ? serviceProvider.GetRequiredService<Wtf4>() :
            Is5(blabla) ? serviceProvider.GetRequiredService<Wtf5>() :
            /* ... */
            IsN(blabla) ? serviceProvider.GetRequiredService<WtfN>() :
            throw new ArgumentException("Brak dostępnej implementacji.")
    );
}

Widać, że tu złożoność nam spada do n+m, czyli asymptotycznie lepiej. Ale, rzekomo takie postępowanie jest niezalecane.

public class SomeServiceFactory
{
    private WTFFactory wtfFactory;
    public SomeServiceFactory(WTFFactory wtfFactory) =>
        this.wtfFactory = wtfFactory;
    public SomeService Create(Blabla blabla) =>
        new SomeService(wtfFactory.Create(blabla));
}

public class WTFFactory
{
    private IServiceProvider serviceProvider;
    public WTFFactory(IServiceProvider serviceProvider) =>
        this.serviceProvider = serviceProvider;
    public IWTF Create(Blabla blabla) =>
        Is1(blabla) ? serviceProvider.GetRequiredService<Wtf1>() : // albo new Wtf1(dep1, dep2, ..., depm)
        Is2(blabla) ? serviceProvider.GetRequiredService<Wtf2>() : // albo new Wtf2(dep1, dep2, ..., depm)
        Is3(blabla) ? serviceProvider.GetRequiredService<Wtf3>() : // albo new Wtf3(dep1, dep2, ..., depm)
        Is4(blabla) ? serviceProvider.GetRequiredService<Wtf4>() : // albo new Wtf4(dep1, dep2, ..., depm)
        Is5(blabla) ? serviceProvider.GetRequiredService<Wtf5>() : // albo new Wtf5(dep1, dep2, ..., depm)
        /* ... */
        IsN(blabla) ? serviceProvider.GetRequiredService<WtfN>() : // albo new WtfN(dep1, dep2, ..., depm)
        throw new ArgumentException("Brak dostępnej implementacji.")
}

Złożoność dalej jest n+m. I, o ile rozumiem te nieszczęsne zalecenia, nie przeczy to zaleceniom, gdyż WTFFactory ma prawo stworzyć sobie w dowolny sposób, nawet za pomocą new albo Service Locatora obiekt implementujący IWTF - natomiast takiego prawa nie ma już żadna inna fabryka, a w szczególności nie SomeServicefactory.

A new jest normalnym i standardowym sposobem tworzenia obiektów.

Tak. Ale zalecenia twierdzą inaczej:

Avoid direct instantiation of dependent classes within services. Direct instantiation couples the code to a particular implementation.

https://docs.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines

Więc trochę na zasadzie cargo cult programming chciałem się do nich dostosować.

YetAnohterone napisał(a):

Tak. W runtime wybierana jest jedna z nich. JEDNA. A zatem, w runtime wiadomo, że pozostałe są zbędne.

Czy jedna instancja działających aplikacji potrzebuje wszystkich instancji IDhl?

• Jeśli tak, to znaczy, że utworzenie wszystkich pozostałych (oprócz tej potrzebnej aktualnie użytkownikowi) nie jest zbędne, bo przydadzą się później.
• Jeśli nie, to znaczy, że całe podejście jest złe, bo skoro uruchomiona instancja jest przywiązana do jednej implementacji IDhl, to znaczy, że zależy to od konfiguracji, a nie od runtime.

A teraz załóżmy, że każda implementacja IWTF zamawia sobie m kolejnych zależności. Widać, że koszt zbudowania fabryki to n×m. Co najmniej.

Dobrze, ale teraz masz dwie zależności - DHL.DHL24WebapiClient i Sandbox.DHL24WebapiClient, prawda?
Nie rozwiązuj problemów, których nie masz, a które raczej nigdy nie wystąpią.

Tak. Ale zalecenia twierdzą inaczej:

Avoid direct instantiation of dependent classes within services. Direct instantiation couples the code to a particular implementation.

Nie, nie twierdzą. Te zalecenia mówią o tym, żeby w OrderController nie robić new OrderService(), w OrderService nie robić new OrderRepository(), a w OrderRepository nie robić new EFContext(). Nie o tym, jak zbudować graf obiektów w fabryce.

Jeśli tak, to znaczy, że utworzenie wszystkich pozostałych (oprócz tej potrzebnej aktualnie użytkownikowi) nie jest zbędne, bo przydadzą się później.

Chyba, że to wszystko jest Transient, wtedy się nie przydadzą.

Nie rozwiązuj problemów, których nie masz, a które raczej nigdy nie wystąpią.

Jest w tym mądrość, przyznaję. KISS, YAGNI? Inaczej zaraz dojdziemy do słynnego FizzBuzzEnterpriseEdition.

Nie, nie twierdzą. Te zalecenia mówią o tym, żeby w OrderController nie robić new OrderService(), w OrderService nie robić new OrderRepository(), a w OrderRepository nie robić new EFContext(). Nie o tym, jak zbudować graf obiektów w fabryce.

No niekoniecznie. Bo dalej mamy:

Another service locator variation to avoid is injecting a factory that resolves dependencies at run time. Both of these practices mix Inversion of Control strategies.

Na początku interpretowałem ten punkt jako zakaz fabryk w ogóle, ale potem (znowu: o ile dobrze zrozumiałem) wytłumaczono mi, że tu chodzi o to, że fabryce nie wolno jawnie tworzyć zależności do obiektu, który ma stworzyć. Czyli, jeśli fabryka ma stworzyć ZamawiaczPaczki, a zależnością ZamawiaczaPaczki jest IDHL, to fabryka ma prawo jawnie stworzyć tylko ZamawiaczaPaczki, ale już nie wolno jej jawnie stworzyć obiektu implementującego IDHL.

Jeśli tak rzeczywiście jest, to mamy ograniczenia, jak zbudować graf obiektów w fabryce.

Czy teraz dobrze interpretuję te zalecenia - nie wiem. Bardzo możliwe, że znowu interpretuję je zupełnie źle i dlatego wychodzą mi potworki. A fabryka może i powinna stworzyć za pomocą new wymagany obiekt wraz ze wszystkimi jego zależnościami.

YetAnohterone napisał(a):

Chyba, że to wszystko jest Transient, wtedy się nie przydadzą.

Te klienty do API są transientowe? Dlaczego?

Na początku interpretowałem ten punkt jako zakaz fabryk w ogóle, ale potem (znowu: o ile dobrze zrozumiałem) wytłumaczono mi, że tu chodzi o to, że fabryce nie wolno jawnie tworzyć zależności do obiektu, który ma stworzyć. Czyli, jeśli fabryka ma stworzyć ZamawiaczPaczki, a zależnością ZamawiaczaPaczki jest IDHL, to fabryka ma prawo jawnie stworzyć tylko ZamawiaczaPaczki, ale już nie wolno jej jawnie stworzyć obiektu implementującego IDHL.

Tak, IDHL to zależność ZamawiaczaPaczki, ale to nie jest Twoja zależność.

Jeśli tak rzeczywiście jest, to mamy ograniczenia, jak zbudować graf obiektów w fabryce.

Jak dla mnie fabryka to jest coś, co na podstawie jakiejś konfiguracji oraz zależności buduje potrzebny obiekt. Fabryka w swoich zależnościach (tak jak każda inna klasa) powinna dostawać tyko zmienne (volatile), a nie stabilne zależności. Kod, który jest zawarty w tym samym repozytorium kodu to na ogół nie jest zmienna zależność.

I zdradzisz, co to za cudny przypadek, że użytkownik wybiera sobie podczas składania zamówienia, czy ma mieć je dostarczone, czy nie?
Bo jak na razie to ogólnie brzmi dla mnie jak jakieś rozwiązywanie problemu x/y.

Te klienty do API są transientowe? Dlaczego?

Nie są. Teoretycznie pisałem. I pewnie przez to przyznałem ci rację, gdy pisałeś, że zajmuję się problemami nieistniejącymi.

Jak dla mnie fabryka to jest coś, co na podstawie jakiejś konfiguracji oraz zależności buduje potrzebny obiekt. Fabryka w swoich zależnościach (tak jak każda inna klasa) powinna dostawać tyko zmienne (volatile), a nie stabilne zależności. Kod, który jest zawarty w tym samym repozytorium kodu to na ogół nie jest zmienna zależność.

Hmm, z tego, co czytałem na SoftwareEngineering Stackexchange wywnioskowałem, że w zasadzie każdy serwis mający jakieś side effecty powinien rygorystycznie przestrzegać DI, natomiast wszystko, co jest side effectów pozbawione (w sensie pure function) może nawet być static. Próba stosowania się do tego była pewnie cargo cult z mojej strony.

I zdradzisz, co to za cudny przypadek, że użytkownik wybiera sobie podczas składania zamówienia, czy ma mieć je dostarczone, czy nie?
Bo jak na razie to ogólnie brzmi dla mnie jak jakieś rozwiązywanie problemu x/y.

Firma podobno może mieć wiele kluczy API do DHLa, w zalezności od oddziału/magazynu/czegoś tam/nie pamiętam. Tak mi powiedziano, nie wnikam. Decyzja, którego klucza użyć przy zamawianiu paczki, jest decyzją runtime'ową, bo nie będzie pięciu kopii aplikacji na firmę, tylko jedna. Ma być obsługa sandboxa dla celów demonstracyjnych. No to zrobić jeden konfiguracyjny switch czy zawsze sandbox czy zawsze normalne DHL? Tak zrobiłem na początku, ale po czasie wydało mi się to dziwne, podatne na gotchas - w konfiguracji jest globalnie zaznaczony sandbox + klucz API do 'normalnego' DHLa i apka nie działa. Nie jest to nic super ważnego, ale wydawało mi się, że bardziej elegancko będzie zmienic sandboxa na ustawienie per klucz API, a nie globalne. Tak właśnie zdawało mi się być logicznie: skoro to, czy łączymy się z sandboxem czy nie zależy od klucza, to powinno być to ustawienie per klucz.