Code Review Asked by Arkady Levin on February 1, 2021
I have created helper class for configuration validation, the main purpose is to find any human "errors"
in the configuration for the app.
the case is that any type of "message" can be configured in more then one place, the application is ok with that and do no not limit this, in some very rare cases this is needed but for most this will cause problems.
with that said i still would like to prompt the warnings.
In the app config there can be unlimited amount of "managers" that each can hold list of "messages"
there are also two types of them but that is not so important.
This is my draft of the class so far, it works but i feel this is very slow and can be done better or at least i am sure there are places to "fix" a little.
public static class ConfigurationValidator
{
public static void ValidateConfiguration(Configuration configuration)
{
if (configuration != null)
{
try
{
// Create Temp dictionary for validation.
Dictionary<string, List<string>> tempDictionary = new Dictionary<string, List<string>>();
Dictionary<string, List<string>> duplicatesList = new Dictionary<string, List<string>>();
// Fill the temp Dictionary for validation.
foreach (var player in configuration.PlayerManager.Players)
{
List<string> lst = new List<string>(); // list of messages.
for (int i = 0; i < player.Messages.Count; i++)
{
lst.Add(player.Messages[i].Type);
}
tempDictionary.Add(player.PlayerName, lst); // list name + list of messages.
}
FindDuplicates(tempDictionary, ref duplicatesList);
// clear for next set of validation.
tempDictionary.Clear();
foreach (var reciver in configuration.ReceiverManager.Receivers)
{
List<string> lst = new List<string>();
for (int i = 0; i < reciver.Messages.Count; i++)
{
lst.Add(reciver.Messages[i].Type);
}
tempDictionary.Add(reciver.ReceiverName, lst);
}
FindDuplicates(tempDictionary, ref duplicatesList);
PrintDuplicates(duplicatesList);
}
catch (Exception Ex)
{
Log.Instance.Error(Ex.Message, Ex);
}
}
else
{
Log.Instance.Warn($"Cannot validate empty configuration.");
}
}
private static void FindDuplicates(Dictionary<string, List<string>> data, ref Dictionary<string, List<string>> duplicatesList)
{
// find duplicates
var dupes = new HashSet<string>(
from list1 in data.Values
from list2 in data.Values
where list1 != list2
from item in list1.Intersect(list2)
select item);
foreach (var list in data.Values)
{
foreach (var message in dupes)
{
if (list.Contains(message))
{
var listName = data.FirstOrDefault(x => x.Value == list).Key;
List<string> value = (duplicatesList.TryGetValue(message, out List<string> index) ? duplicatesList[message] : null);
if (value == null)
{
duplicatesList.Add(message, new List<string>());
duplicatesList[message].Add(listName);
}
else
{
duplicatesList[message].Add(listName);
}
}
}
}
}
private static void PrintDuplicates(Dictionary<string, List<string>> duplicatesList)
{
if (duplicatesList == null || duplicatesList.Count <= 0)
{
Log.Instance.Info("Configuration validation was OK! no duplicates were found.");
return;
}
Log.Instance.Error($"Configuration validation found duplicate messages in configuration!!!");
Log.Instance.Warn($"This could lead to not accurate recording or playing please fix before using the DDS Recorder.");
foreach (var key in duplicatesList)
{
Log.Instance.Warn($"The Message: [{key.Key}] is duplicated in this lists: ");
foreach (var listName in key.Value)
{
Log.Instance.Warn($" Name: [{listName}]");
}
}
}
Some quick remarks:
Invert the if (configuration != null) logic and return; immediately if (configuration == null) (after logging this error). That way your code isn't indented as much and easier to read.
Comments should say why, not what. // Create Temp dictionary for validation. is something I should be able to instantly see in your code, if I need a comment to figure this out you need to rewrite your code, not write a comment.
Do not call things "List" etc. unless you absolutely need to. Case in point is your code: duplicatesList isn't a list, it is a dictionary. Simply call it duplicates, just like you would do in real life with collections.
Do not pointlessly abbreviate. lst isn't clear.
The whole foreach (var player in configuration.PlayerManager.Players) block can be expressed in a simple LINQ query, and most certainly the whole logic to construct lst.
Beware of spelling errors: reciver.
Why does a Receiver have a ReceiverName property? Why not simply Name? (Ditto for Player and PlayerName.)
Why is there a try...catch for Exceptions? I don't see anything in your code that could cause exceptions.
I'm baffled at (duplicatesList.TryGetValue(message, out List<string> index) ? duplicatesList[message] : null);. Please look up TryGetValue and how it works and how it should be used.
Do not "shout" at people, especially not by using multiple exclamation marks ("!!!"). The level of urgency is set by the type of log entry -- Error vs Warn vs Info vs Debug etc. "Configuration validation was OK!" is somehow even worse: this should be a simple statement of fact, not an exclamation of joy.
Answered by BCdotWEB on February 1, 2021
I'm going to make some assumptions about your code that there is either an interface or base class or they are using the same class for players and receivers. If not I would create an interface so you don't have to duplicate so much code between players and receivers. I'd leave the printing alone with exception of changing parameter to IDictionary<string, IEnumerable<string>>.
Instead of having the ValidateConfiguration create a temp dictionary and then passing it down to use intersect we can do that entire set using Linq in the FindDuplicates and reduce the amount of code. If I'm reading the code correctly in the end you just want to show each "PlayerName" that has the same "Type" as another "PlayerName"
public static void ValidateConfiguration(Configuration configuration)
{
// ... omitted code for logging and checking for null config.
var playerDups = FindDuplicates(configuration.PlayerManager.Players);
PrintDuplicates(playerDups);
// this assumes receivers and players share the same class. If not they should share a base class or interface
var receiverDups = FindDuplicates(configuration.PlayerManager.Receiver);
PrintDuplicates(receiverDups);
}
// todo the parameter should be the common class or base class or interface that is shared between Players and Receivers
private static IDictionary<string, IEnumerable<string>> FindDuplicates(IEnumerable<Player> player)
{
// flattern out the messages to be the playername and type
// Then group messages together
// filter out the ones that only have one (old code was using intercept)
// convert to dictionary for easier access (could do ILookup)
return player.SelectMany(p => p.Messages.Select(m => new
{
Type = m.Type,
PlayerName = p.PlayerName
}))
.GroupBy(x => x.Type, x => x.PlayerName)
.Where(x => x.Count() > 1)
.ToDictionary(x => x.Key, x => x.AsEnumerable());
}
Answered by CharlesNRice on February 1, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP