Hallo,

ich habe in einem DomainLayer eines Webservices folgende Methode so ähnlich mehrfach implementiert:

private Dictionary<long, List<TaskTO>> GetOpenTasksByUserIDs()
{
	Dictionary<long, List<TaskTO>> result = new Dictionary<long, List<TaskTO>>();
	var activeUsers = DomainServiceLocator.userService.GetAllActiveUsers();
	foreach (UserTO user in activeUsers)
	{
		var openTasks = DomainServiceLocator.lpaService.GetAssignedOpenTasksForUserID(user.ID);
		foreach (TaskTO openTask in openTasks)
		{
			if (!result.ContainsKey(user.ID))
			{
				result.Add(user.ID, new List<TaskTO>());
			}
			result[user.ID].Add(openTask);
		}
	}
	return result;
}

Ich wüsste gern, ob man den Code vereinfachen kann. Insbesondere das if-Statement im inneren foreach stört mich (inkl. Klammersetzung 4 Zeilen Code um einen Eintrag hinzuzufügen falls er noch nicht existiert).

Ich bin dankbar für eure Ideen.

Was ich so sehe:
- Die Art und Weise, wie der Service Locator Pattern angewendet ist, ist ein Anti Pattern
- Des weiteren allokierst Du sehr viele Ressourcen unnöttig (zB die leere Liste), die Du dann im nächsten Schritt wieder überschreibst
- Offenbar werden synchron mehrere Quellen angesprochen, was blockierender Code darstellt

Das if ist noch das unkritischste ;-)

Hallo,

kannst du diese 3 Punkte noch etwas genauer ausformulieren oder einen Alternativen Code vorschlagen?

Ziel dieser Methode ist es, eine Übersicht zu bekommen, für welchen "User" welche "Task"s offen sind. Dies wird im Nachgang benötigt, da je Task eine Mail verschickt werden soll. D.h. die Task- und User-Instanzen werden anschließend noch benötigt.

Befass Dich mit den Themen :-)
Die Microsoft Docs sind bei solchen Basis-Themen sehr gut. Einfach mal ein Blick rein werfen.

- Dependency Injection statt Service Locator Pattern -> Dependency injection in .NET
- Auf unnötige Allocations verzichten (zB null ist nicht böse, wenn man es richtig verwendet :-) bzw. auf Allocations verzichten, wenn sie ohnehin im nächsten Schritt überschrieben werden (hier if/else)
- Asynchrone Programmierung anwenden -> Asynchronous programming in C#

Werd jetz aber nich den Code für dich refrakturieren - soll ja ein Lerneffekt für Dich sein :-)

@Abt: Die leere Liste wird nicht überschrieben, sondern openTask dieser Liste hinzugefügt.

Statt ContainsKey sollte besser TryGetValue verwendet werden (damit man nicht danach per Index den Eintrag wieder neu suchen muß).

Worauf ich damit hinaus wollte:

Ich nehm an, dass var openTasks eine List<TaskTO> ist (Käse hier var zu verwenden, but nvmd).
Dictionary besitzt eine sehr bequeme Methode, wie man einen Key mit Value in einem Rutsch erzeugen kann, ohne a) eine neue Liste zu erstellen und b) manuell kopieren zu müssen: TryAdd.
Den Zustand einer leeren Liste muss es hier nie geben.
Perfektes Beispiel wie wichtig es ist, die Dinge zu kennen, die man verwendet - oder mal in die Docs zu spicken. Tut nicht weh :-)

Etwas OT ... aber erwähnenswert.

Zitat

Käse hier var zu verwenden

var verwenden ist in vielen Fällen entweder Faulheit oder der Irrglaube man würde damit etwas besser machen, obwohl es nicht der Fall ist ;-)
Lesbarer wirds meist nicht.

@Abt: Ja, du hast recht, die ganze foreach-Schleife ist hier überflüssig - ich habe gestern übersehen, daß hier keine Überprüfung der einzelnen OpenTask vorgenommen wird, sondern nur ob die UserId schon existiert oder nicht.

PS: Bzgl. var - dann solltet ihr nicht meine Codes sehen...
Aber auch hier die erste Zeile sollte m.e. besser oder aber geschrieben werden.

Wobei ich mich hier generell frage, warum hat TaskTO nicht eine Methode zum Abfragen der OpenTasks?

Zitat von Th69

oder aber Dictionary<long, List<TaskTO>> result = new(); geschrieben werden.

Das. Wir haben 2022, nicht mehr 2005 :-)

Zitat

Wobei ich mich hier generell frage, warum hat TaskTO nicht eine Methode zum Abfragen der OpenTasks?

Glaube die Architektur-Leichen liegen noch an anderen, zentraleren Stellen (siehe Service Locator), die hier keinen Einfluss haben.
Die Methode hier kannst halt nur so weit optimieren, wie es die Umgebung zulässt.

Halb Off-Topic:

Zitat von Abt

Die Art und Weise, wie der Service Locator Pattern angewendet ist, ist ein Anti Pattern

Der Satz wirkt, als gäbe es auch einen "richtigen" Weg, das Service Locator Pattern zu verwenden?
Wie sieht denn dieser "richtige" Weg aus?

Für mich ist das generell ein Anti Pattern

https://jimmybogard.com/service-locator-is-not-an-anti-pattern/ :-)

Ok um ehrlich zu sein kann ich noch nicht alle Tipps umsetzen. Der Code stammt aus einem Projekt (WindowsService) basierend auf Dot Net 4.5.2.

Ich habe den Code jetzt umgeschrieben und poste zur besseren Übersicht mal die gesamte Klasse:

internal class PeriodicOpenTasksMailSender
{
	private static Logger logger = LogManager.GetCurrentClassLogger();

	internal void Run()
	{
		if (!HasToRun())
		{
			return;
		}
		
		logger.LogMethodInvoke(this);
		CheckAllUsers();
		SetHasRunToday();
	}

	private bool HasToRun()
	{
		MailsenderPeriod period = DomainServiceLocator.applicationPropertyService.Get_PeriodicOpenTaskAndIssueSender_Period();
		DateTime? timestamp = DomainServiceLocator.applicationPropertyService.Get_PeriodicOpenTaskAndIssueSender_LastRanTimestamp();
		return period.IsOverdue(timestamp);
	}

	private void SetHasRunToday()
	{
		DomainServiceLocator.applicationPropertyService.Set_PeriodicOpenTaskAndIssueSender_LastRanTimestamp();
	}

	private void CheckAllUsers()
	{
		foreach (UserTO user in DomainServiceLocator.userService.GetAllUsers())
		{
			CheckSingleUser(user);
		}
	}

	private void CheckSingleUser(UserTO user)
	{
		List<LPATaskTO> openTasks = DomainServiceLocator.lpaService.GetAssignedOpenTasksForUserID(user.ID);
		if (openTasks.IsNullOrEmpty())
		{
			return;
		}

		SendMailToUser(user, openTasks);
	}

	private void SendMailToUser(UserTO user, IEnumerable<LPATaskTO> openTasks)
	{
		MailSendingOpenTasksInfo mailsending = new MailSendingOpenTasksInfo(user, openTasks);

		DomainServiceLocator.mailSender.SendSingleMail(mailsending);
	}

}

Wobei

		DomainServiceLocator.mailSender.SendSingleMail(mailsending);

die Mailadresse des Benutzers holt und die Informationen über die offenen "Task"s in Mail-Text umwandelt sowie die Mail sendet.

Alle Properties von DomainServiceLocator sind Interfaces. Dort gibt es z.B. IOService, DBService, MailSender, UserService und so weiter. Die einzige Stelle, wo wir in dem Projekt bisher Service-Instanzen zur Laufzeit bestimmen, ist der DBMS-Typ (MySQL / MSSQL). Ansonsten haben alle Interfaces bisher genau 1 Implementierung.

Kleine Anmerkung am Rande:

Service Locator is an Anti-Pattern

Benutze stattdessen besser einen IoC Container.

Jetzt haben wir ein Problem
Im von Abt verlinkten Artikel Service Locator is not an Anti-Pattern ist dein verlinkter Artikel Service Locator is an Anti-Pattern verlinkt

Wer hat jetzt Recht?
Ich persönlich finde, dass im "Nicht-Anti-Pattern"-Artikel kaum Argumente genannt werden, außer am Ende die Performance.
Alles andere klingt nach Faulheit oder "keine Lust, umzudenken".
Sicher gibt es durchaus Situationen, in denen das ServiceLocator-Pattern seine Daseinsberechtigung hat, allerdings denke ich, dass das wirklich große Ausnahmen sind - wie z.B. die Fälle, in denen die Konstruktor-Performance ausschlaggebend ist.

Allerdings würde das Thema den Rahmen hier ziemlich sicher sprengen :D

Hehe, mir gings auch eher um die verschachtelten foreach und das (mich störende) if im Original-Post. Zusätzlich dazu die Tatsache, dass erstmal jede Menge Instanzen allokiert werden, welche dann später erst genutzt werden.

Ich hab's jetzt so umgebaut, dass ich im Rahmen des Projektes damit leben kann.

Meine Frage zielte also eher auf Vereinfachung des ursprünglichen Codes ab bzw. wenn ich jetzt im Nachhinein darüber nachdenke, auch darauf, wie man mit (unbekannten?) Datenmengen umgeht bei einer Art Stapelverarbeitung. Der Code soll nichts anderes machen, als für alle Benutzer im System X eine Mail zu verschicken und sie über offene Aufgaben (Tasks) benachrichtigen. Bzw. war es die Vorarbeit dafür, nämlich das Finden der entsprechenden Benutzer und dazugehörigen offenen Aufgaben.

Wenn es nur darum geht:

Oder komplett mit Linq:

Funktioniert natürlich nur, wenn GetAssignedOpenTasksForUserID auch eine Liste zurück liefert.
Oder Du passt einfach den Typ im Dictionary an.

Danke @Palladin007. Die kurze Variante mittels Linq .ToDictionary kannte ich noch nicht.