myCSharp.de - DIE C# und .NET Community
Willkommen auf myCSharp.de! Anmelden | kostenlos registrieren
 
 | Suche | FAQ

» Hauptmenü
myCSharp.de
» Startseite
» Forum
» FAQ
» Artikel
» C#-Snippets
» Jobbörse
» Suche
» Regeln
» Wie poste ich richtig?
» Forum-FAQ

Mitglieder
» Liste / Suche
» Wer ist wo online?

Ressourcen
» openbook: Visual C#
» openbook: OO
» Microsoft Docs

Team
» Kontakt
» Übersicht
» Wir über uns

» myCSharp.de Diskussionsforum
Du befindest Dich hier: Community-Index » Diskussionsforum » Entwicklung » Code-Reviews » Einfaches Repository für MongoDB-Anwendungen
Letzter Beitrag | Erster ungelesener Beitrag Druckvorschau | Thema zu Favoriten hinzufügen

Antwort erstellen
Zum Ende der Seite springen  

Einfaches Repository für MongoDB-Anwendungen

 
Autor
Beitrag « Vorheriges Thema | Nächstes Thema »
emuuu
myCSharp.de-Mitglied

avatar-4078.jpg


Dabei seit: 04.02.2011
Beiträge: 280


emuuu ist offline

Einfaches Repository für MongoDB-Anwendungen

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Guten Morgen zusammen,

ich habe für meine Anwendung ein einfaches (MongoDB-)Repository erstellt, dass CRUD liefert und beliebig erweitert werden kann.
Für mich ist Ziel des Ganzen, dass ich in einer Microservice-Umgebung möglichst unkompliziert einen neuen Service erstellen kann, der eine einzige Enity verwaltet erstellen kann.

 MongoRepository
Kritik, Anregungen und vor allem Verbesserungsvorschläge sind willkommen.

Beste Grüße
emuuu

Dieser Beitrag wurde 1 mal editiert, zum letzten Mal von emuuu am 03.09.2020 11:22.

03.09.2020 11:22 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Abt
myCSharp.de-Team

avatar-4119.png


Dabei seit: 20.07.2008
Beiträge: 14.280
Herkunft: Stuttgart/Stockholm


Abt ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Feedback:

- Samples sollten nicht in /src liegen sondern in /samples. Grund: ist so üblich und bei der Verwendung von Tools wie Code Coverage interessiert Dich nur der Lib-Code in src, nicht der Sample-Code. Macht das Tooling einfacher und die Leute sehen auch direkt die Samples auf GitHub
- Mach Doch Deine GIt Messages in Englisch. Gibt dazu auch einen Guide:  How to Write a Git Commit Message
- Deine Methoden sind alle Fire-and-Forget aufgebaut. Der Verwender hat keine Chance die Results der MongoDB Operationen zu erhalten.
- Bei Bibliotheken und async solltest Du immer ConfigureAwait(false) setzen:  ConfigureAwait FAQ
- #region ist Code Smell ;-)

C#-Code:
services.AddTransient<IWeatherForecastRepository, WeatherForecastRepository>();

Warum Transient? Üblich wäre Scoped; da MongoDB Verbindungen jedoch Thread-Safe sind wäre theoretisch bei Thread-Safe-Implementierung sogar ein Singleton möglich.

Implementierung nennt man eigentlich nach der konkreten Implementierung; d.h. dass IWeatherForecastRepository der absolut korrekte Name für ein Interface wäre, jedoch die Implementierung sich ja hier auf MongoDB bezieht: WeatherForecastMongoDbRepository.
Es könnte je auch sein, dass es ein WeatherForecastMssqlRepository gibt.
03.09.2020 11:52 Beiträge des Benutzers | zu Buddylist hinzufügen
emuuu
myCSharp.de-Mitglied

avatar-4078.jpg


Dabei seit: 04.02.2011
Beiträge: 280

Themenstarter Thema begonnen von emuuu

emuuu ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Super, danke schon mal für das Feedback, gerade in Bezug auf Konventionen, da ich da noch nicht so firm bin.

Die eigenen commit messages, mache ich immer auf Englisch, allerdings habe ich den initial commit über das GitHub-plugin von VS erstellt, vermute dass das auf DE localed ist. Da muss ich mir nochmal die Einstellungen anschauen.

Zitat von Abt:
Feedback:
- Deine Methoden sind alle Fire-and-Forget aufgebaut. Der Verwender hat keine Chance die Results der MongoDB Operationen zu erhalten.

Meinst du damit, dass ich await Collection.... direkt zurückgebe?
03.09.2020 12:08 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Abt
myCSharp.de-Team

avatar-4119.png


Dabei seit: 20.07.2008
Beiträge: 14.280
Herkunft: Stuttgart/Stockholm


Abt ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

C#-Code:
return await Collection.Find(filter).ToListAsync();

ist das gleiche wie

C#-Code:
return await Collection.Find(filter).ToListAsync().ConfigureAwait(true);

richtig für Bibliotheken wäre jedoch

C#-Code:
return await Collection.Find(filter).ToListAsync().ConfigureAwait(false);

Bei Methoden, die nur die Rückgabe haben, solltest Du nicht

C#-Code:
        public virtual async Task<IEnumerable<TEntity>> GetAll()
        {
            return await Collection.Find(new BsonDocument()).ToListAsync().ConfigureAwait(false);
        }

schreiben, sondern

C#-Code:
        public virtual Task<IEnumerable<TEntity>> GetAll()
        {
            return Collection.Find(new BsonDocument()).ToListAsync();
        }

denn das verhindert die unnötige State Machine für diese Methode.

Diese Methode hat aber ein anderes Problem: Du materialisierst die Liste, gibst aber nen IEnumerable zurück; besser wäre daher

C#-Code:
        public virtual Task<IList<TEntity>> GetAll()
        {
            return Collection.Find(new BsonDocument()).ToListAsync();
        }

Da aber die implizite Rückgabe von IList hier nicht geht brauchst Du

C#-Code:
    public virtual async Task<IList<TEntity>> GetAll()
        {
            IList<TEntity> list = await Collection.Find(new BsonDocument()).ToListAsync().ConfigureAwait(false);
            return return list;
        }

oder eben

C#-Code:
    public virtual Task<IList<TEntity>> GetAll()
        {
            return Collection.Find(new BsonDocument()).ToListAsync<IList<TEntity>>();
        }

PS: das Find kannst Dir sparen wenn Du alle Elemente willst.
Deinem Repository fehlt aber Paging, also Skip Take oder Expression-Parameter für Filter etc.

Im DAL gibts auch so ne Art Konventionen:
- Query-Methoden (aka QueryWeather(..)) geben IQueryable zurück; also ein nicht-materialisierter Zustand
- Get-Methoden geben den materialisierten Zustand zurück (i.d.R. IList und vergleichbares)

Für IEnumerable gibt es IIRC keinen einzigen Zweck im DAL; wenn dann IAsyncEnumerable
03.09.2020 12:19 Beiträge des Benutzers | zu Buddylist hinzufügen
Palladin007 Palladin007 ist männlich
myCSharp.de-Mitglied

avatar-4140.png


Dabei seit: 03.02.2012
Beiträge: 1.362
Entwicklungsumgebung: Visual Studio 2019
Herkunft: NRW


Palladin007 ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

IEnumerable hat aber den Vorteil, dass man später leichter die Implementierung tauschen kann.
Man kann ja immer noch eine Liste zurück geben und vermeidet so potentielle mehrfache Queries.

IAsyncEnumerable wäre natürlich besser, aber man hat nicht immer die Möglichkeit, es zu nutzen.
In so einem Fall und und wenn man eine recht komplexe Implementierung hat, kann es klüger sein, mit IEnumerable zu arbeiten, anstatt mit IList - außer der Aufrufer braucht IList-Funktionalitäten.

IEnumerable hat natürlich auch den Nachteil, dass jemand, der es nicht besser weiß, damit schnell Probleme haben kann.

Wie man das jetzt in diesem Fall einordnet, ist natürlich ein anderes Thema, aber ich würde IEnumerable im DAL nicht pauschal verneinen.
03.09.2020 13:27 Beiträge des Benutzers | zu Buddylist hinzufügen
Abt
myCSharp.de-Team

avatar-4119.png


Dabei seit: 20.07.2008
Beiträge: 14.280
Herkunft: Stuttgart/Stockholm


Abt ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat von Palladin007:
IEnumerable hat aber den Vorteil, dass man später leichter die Implementierung tauschen kann.

Welche Implementierung? Du bist ja ohnehin im materialisierten Zustand.
IEnumerable birgt gerade im Sinne der Materialisierung mehr Gefahren und Unklarheiten als Vorteile.

Zitat von Palladin007:
kann es klüger sein, mit IEnumerable zu arbeiten

Warum? Hast nen Beispiel, wo es wirklich Vorteile geben würde?


Noch zum Feedback:

C#-Code:
var filter = Builders<TEntity>.Filter.In("Id", ids);

Magic Strings vermeiden:

C#-Code:
var filter = Builders<TEntity>.Filter.In(nameof(IEntity<TKey>.Id), ids);

Und die Rückgabe kannst Du direkt schon Nullable markieren:

C#-Code:
public virtual async Task<TEntity?> Get(TKey id)
03.09.2020 13:30 Beiträge des Benutzers | zu Buddylist hinzufügen
emuuu
myCSharp.de-Mitglied

avatar-4078.jpg


Dabei seit: 04.02.2011
Beiträge: 280

Themenstarter Thema begonnen von emuuu

emuuu ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Hier mal die Variante mit Filter / Sort via Linq:

C#-Code:
public virtual async Task<IList<TEntity>> GetAll<TProperty>(Expression<Func<TEntity, bool>> filter, Expression<Func<TEntity, TProperty>> sorting, int? page = null, int? pageSize = null)
        {
            IList<TEntity> result = await Collection
                .AsQueryable()
                .Where(filter)
                .OrderBy(sorting)
                .ToListAsync().ConfigureAwait(false);

            if(page.HasValue && pageSize.HasValue)
            {
                if (page < 1)
                {
                    page = 1;
                }
                if (pageSize < 1)
                {
                    pageSize = 1;
                }

                result = result
                .Skip((page.Value - 1) * pageSize.Value)
                .Take(pageSize.Value)
                .ToList();
            }
            return result;
        }

TProperty wird benötigt, da TKey auf eine contravariant wäre (da bereits verwendet).

Spricht was gegen den Ansatz? (ThenBy würde ich in einem separaten overload implementieren)

Wie sieht es eigentlich in Richtung best practices aus, was die parameter angeht? Sollte ich lieber für jede Variante nen eigenen overload erstellen oder so wie bisher alles gleichzeitig aufnehmen und nen null-Value einfach handlen?
15.09.2020 17:18 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Abt
myCSharp.de-Team

avatar-4119.png


Dabei seit: 20.07.2008
Beiträge: 14.280
Herkunft: Stuttgart/Stockholm


Abt ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat von emuuu:
Wie sieht es eigentlich in Richtung best practices aus, was die parameter angeht?

- Kein GetAll anbieten, sondern wirklich immer nur ein Paging.
- Kein endlos großen PageSize anbieten

Auch wenn ausgehende Nachrichten kein Limit haben macht ein GetAll eigentlich niemals sinn.
Man würde dem Client auch nicht bestimmen lassen, wie groß die PageSize maximal sein darf, sondern vom Server limitieren.
Auch wenn der Client zB 10.000 Einträge will, würde man nur 1000 zurück geben.
Macht man allgemein beim API Design so (auch bei REST und Co..).

Gib der Methode die Möglichkeit einen CancellationToken mit zu geben, sodass man die DB-Anfrage abbrechen kann.
ToListAsync unterstützt die Signatur.
15.09.2020 17:25 Beiträge des Benutzers | zu Buddylist hinzufügen
emuuu
myCSharp.de-Mitglied

avatar-4078.jpg


Dabei seit: 04.02.2011
Beiträge: 280

Themenstarter Thema begonnen von emuuu

emuuu ist offline

Beitrag: beantworten | zitieren | editieren | melden/löschen       | Top

Zitat von Abt:
Auch wenn der Client zB 10.000 Einträge will, würde man nur 1000 zurück geben.
Macht man allgemein beim API Design so (auch bei REST und Co..).

Im Allgemeinen ist mir das klar, aber hierbei geht es ja um eine Klassenbibliothek, die man als Teil eines Projektes nutzen soll.
Sollte ich da nicht eher dem Anwender überlassen wie er die Pagination in seinem Service limitiert? Es ist ja eher ein Tool, das noch konfiguriert / benutzt werden soll als eine fertige Lösung.
15.09.2020 18:01 E-Mail | Beiträge des Benutzers | zu Buddylist hinzufügen
Baumstruktur | Brettstruktur       | Top 
myCSharp.de | Forum Der Startbeitrag ist älter als 2 Monate.
Der letzte Beitrag ist älter als 2 Monate.
Antwort erstellen


© Copyright 2003-2020 myCSharp.de-Team | Impressum | Datenschutz | Alle Rechte vorbehalten. | Dieses Portal verwendet zum korrekten Betrieb Cookies. 23.11.2020 18:13