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
Dictionary erstellen - Code optimieren ?
snoop83
myCSharp.de - Member



Dabei seit:
Beiträge: 46

Themenstarter:

Dictionary erstellen - Code optimieren ?

beantworten | zitieren | melden

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

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15.928

beantworten | zitieren | melden

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



Dabei seit:
Beiträge: 46

Themenstarter:

beantworten | zitieren | melden

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

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15.928

beantworten | zitieren | melden

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 :-)
private Nachricht | Beiträge des Benutzers
Th69
myCSharp.de - Experte

Avatar #avatar-2578.jpg


Dabei seit:
Beiträge: 4.389

beantworten | zitieren | melden

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

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15.928

beantworten | zitieren | melden

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 :-)
private Nachricht | Beiträge des Benutzers
chilic
myCSharp.de - Experte



Dabei seit:
Beiträge: 2.106

beantworten | zitieren | melden

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.
Dieser Beitrag wurde 1 mal editiert, zum letzten Mal von chilic am .
private Nachricht | Beiträge des Benutzers
Th69
myCSharp.de - Experte

Avatar #avatar-2578.jpg


Dabei seit:
Beiträge: 4.389

beantworten | zitieren | melden

@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?
Dieser Beitrag wurde 3 mal editiert, zum letzten Mal von Th69 am .
private Nachricht | Beiträge des Benutzers
Abt
myCSharp.de - Team

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15.928

beantworten | zitieren | melden

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

Avatar #avatar-4140.png


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

beantworten | zitieren | melden

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

Avatar #avatar-4119.png


Dabei seit:
Beiträge: 15.928

beantworten | zitieren | melden

https://jimmybogard.com/service-locator-is-not-an-anti-pattern/ :-)
private Nachricht | Beiträge des Benutzers
snoop83
myCSharp.de - Member



Dabei seit:
Beiträge: 46

Themenstarter:

beantworten | zitieren | melden

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.
Dieser Beitrag wurde 4 mal editiert, zum letzten Mal von snoop83 am .
private Nachricht | Beiträge des Benutzers
Alf Ator
myCSharp.de - Member



Dabei seit:
Beiträge: 665

beantworten | zitieren | melden

Kleine Anmerkung am Rande:

Service Locator is an Anti-Pattern

Benutze stattdessen besser einen IoC Container.
private Nachricht | Beiträge des Benutzers
Palladin007
myCSharp.de - Member

Avatar #avatar-4140.png


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

beantworten | zitieren | melden


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
Dieser Beitrag wurde 1 mal editiert, zum letzten Mal von Palladin007 am .
private Nachricht | Beiträge des Benutzers
snoop83
myCSharp.de - Member



Dabei seit:
Beiträge: 46

Themenstarter:

beantworten | zitieren | melden

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

Avatar #avatar-4140.png


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

beantworten | zitieren | melden

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



Dabei seit:
Beiträge: 46

Themenstarter:

beantworten | zitieren | melden

Danke @Palladin007. Die kurze Variante mittels Linq .ToDictionary kannte ich noch nicht.
private Nachricht | Beiträge des Benutzers