Laden...

Dictionary erstellen - Code optimieren ?

Erstellt von snoop83 vor einem Jahr Letzter Beitrag vor einem Jahr 1.200 Views
S
snoop83 Themenstarter:in
46 Beiträge seit 2006
vor einem Jahr
Dictionary erstellen - Code optimieren ?

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.

16.807 Beiträge seit 2008
vor einem Jahr

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 😉

S
snoop83 Themenstarter:in
46 Beiträge seit 2006
vor einem Jahr

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.

16.807 Beiträge seit 2008
vor einem Jahr

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 🙂

4.931 Beiträge seit 2008
vor einem Jahr

@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ß).

16.807 Beiträge seit 2008
vor einem Jahr

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.


if(!result.TryAdd(user.ID, openTasks))
{
    result[user.ID].AddRange(openTasks);
}

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 🙂

C
2.121 Beiträge seit 2010
vor einem Jahr

Etwas OT ... aber erwähnenswert.

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.

4.931 Beiträge seit 2008
vor einem Jahr

@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


var result = new Dictionary<long, List<TaskTO>>();

oder aber


Dictionary<long, List<TaskTO>> result = new();

geschrieben werden.

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

16.807 Beiträge seit 2008
vor einem Jahr

oder aber

  
Dictionary<long, List<TaskTO>> result = new();  
   

geschrieben werden.

Das. Wir haben 2022, nicht mehr 2005 🙂

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.

2.078 Beiträge seit 2012
vor einem Jahr

Halb Off-Topic:

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 😠

S
snoop83 Themenstarter:in
46 Beiträge seit 2006
vor einem Jahr

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.

A
764 Beiträge seit 2007
vor einem Jahr

Kleine Anmerkung am Rande:

Service Locator is an Anti-Pattern

Benutze stattdessen besser einen IoC Container.

2.078 Beiträge seit 2012
vor einem Jahr

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 😁

S
snoop83 Themenstarter:in
46 Beiträge seit 2006
vor einem Jahr

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.

2.078 Beiträge seit 2012
vor einem Jahr

Wenn es nur darum geht:


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

		// Oder ohne .ToList(), abhängig davon, was openTasks für einen Typ hat
		// Oder Du änderst den Typ im Dictionary - ich mag's sowieso nicht, veränderbare Listen umher zu reichen
		result.Add(user.ID, openTasks.ToList());
	}
	return result;
}

Oder komplett mit Linq:


private Dictionary<long, List<TaskTO>> GetOpenTasksByUserIDs()
{
	return DomainServiceLocator.userService.GetAllActiveUsers()
		.ToDictionary(
			x => x.ID,
			x => DomainServiceLocator.lpaService.GetAssignedOpenTasksForUserID(user.ID));
}

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

S
snoop83 Themenstarter:in
46 Beiträge seit 2006
vor einem Jahr

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