Willkommen auf myCSharp.de! Anmelden | kostenlos registrieren
 | Suche | FAQ

Hauptmenü
myCSharp.de
» Startseite
» Forum
» Suche
» Regeln
» Wie poste ich richtig?

Mitglieder
» Liste / Suche
» Wer ist online?

Ressourcen
» FAQ
» Artikel
» C#-Snippets
» Jobbörse
» Microsoft Docs

Team
» Kontakt
» Cookies
» Spenden
» Datenschutz
» Impressum

  • »
  • Community
  • |
  • Diskussionsforum
Einfaches Repository für MongoDB-Anwendungen
emuuu
myCSharp.de - Member

Avatar #avatar-4078.jpg


Dabei seit:
Beiträge: 286

Themenstarter:

Einfaches Repository für MongoDB-Anwendungen

beantworten | zitieren | melden

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 .
2+2=5 (für extrem große Werte von 2)
private Nachricht | Beiträge des Benutzers
Abt
myCSharp.de - Team

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15618
Herkunft: BW

beantworten | zitieren | melden

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 ;-)

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.
private Nachricht | Beiträge des Benutzers
emuuu
myCSharp.de - Member

Avatar #avatar-4078.jpg


Dabei seit:
Beiträge: 286

Themenstarter:

beantworten | zitieren | melden

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?
2+2=5 (für extrem große Werte von 2)
private Nachricht | Beiträge des Benutzers
Abt
myCSharp.de - Team

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15618
Herkunft: BW

beantworten | zitieren | melden

return await Collection.Find(filter).ToListAsync();
ist das gleiche wie

return await Collection.Find(filter).ToListAsync().ConfigureAwait(true);
richtig für Bibliotheken wäre jedoch

return await Collection.Find(filter).ToListAsync().ConfigureAwait(false);

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

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

schreiben, sondern

		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


		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


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

oder eben


	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
private Nachricht | Beiträge des Benutzers
Palladin007
myCSharp.de - Member

Avatar #avatar-4140.png


Dabei seit:
Beiträge: 1429
Herkunft: Düsseldorf

beantworten | zitieren | melden

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.
private Nachricht | Beiträge des Benutzers
Abt
myCSharp.de - Team

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15618
Herkunft: BW

beantworten | zitieren | melden

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:

var filter = Builders<TEntity>.Filter.In("Id", ids);

Magic Strings vermeiden:

var filter = Builders<TEntity>.Filter.In(nameof(IEntity<TKey>.Id), ids);

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

public virtual async Task<TEntity?> Get(TKey id)
private Nachricht | Beiträge des Benutzers
emuuu
myCSharp.de - Member

Avatar #avatar-4078.jpg


Dabei seit:
Beiträge: 286

Themenstarter:

beantworten | zitieren | melden

Hier mal die Variante mit Filter / Sort via Linq:


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?
2+2=5 (für extrem große Werte von 2)
private Nachricht | Beiträge des Benutzers
Abt
myCSharp.de - Team

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15618
Herkunft: BW

beantworten | zitieren | melden

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.
private Nachricht | Beiträge des Benutzers
emuuu
myCSharp.de - Member

Avatar #avatar-4078.jpg


Dabei seit:
Beiträge: 286

Themenstarter:

beantworten | zitieren | melden

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.
2+2=5 (für extrem große Werte von 2)
private Nachricht | Beiträge des Benutzers