3
\$\begingroup\$

I have a .csv file in which each row represents an association between a software application and a web server. My goal is to fetch a List<Server> by application, so I'm mapping the .csv to a Dictionary<string, List<Server>>. I'm using CsvHelper to read the .csv file and map it to objects.

.csv format:

Family | Environment | Name   | Application
01     | Dev         | WEBD01 | application1
01     | Production  | WEBP01 | application1
02     | Dev         | WEBD02 | application2

Server class:

public class Server
{
    public string Name { get; set; }
    public string Family { get; set; }
    public string Environment { get; set; }
}

Mapping:

protected override void RefreshData()
{        
    // _serverDictionary is a class-level Dictionary<string, List<Server>>
    _serverDictionary.Clear();

    using (TextReader textReader = File.OpenText(_csvFile.FullName))
    using (CsvReader reader = new CsvReader(textReader))
    {    
        while (reader.Read())
        {
            string applicationName = reader.GetField<string>("Application");
            Server server = reader.GetRecord<Server>();

            if (_serverDictionary.ContainsKey(applicationName))
                _serverDictionary[applicationName].Add(server);
            else
                _serverDictionary.Add(applicationName, new List<Server> { server });
        }
    }
}

Fetching:

public IEnumerable<Server> GetByApplication(string applicationName)
{
    List<Server> servers = new List<Server>();

    _appServers.Where(pair => pair.Key.EqualsIgnoreCase(applicationName))
        .ForEach(pair => servers.AddRange(pair.Value));

    return servers;
}

Concerns:

  1. Is a Dictionary the best data structure to use in the first place?
  2. I feel that both the mapping code and the fetch code could be more efficient by better utilizing LINQ projection.
\$\endgroup\$
  • \$\begingroup\$ serverDictionary is local to the RefreshData method so it actually does nothing. Is this really what you have? \$\endgroup\$ – t3chb0t Apr 28 '17 at 19:34
  • \$\begingroup\$ You're right. I mistakenly tried to adjust my code for the question. I've corrected it to reflect what it's actually doing which is updating a class-level Dictionary. \$\endgroup\$ – Tyler Daniels Apr 28 '17 at 19:55
  • \$\begingroup\$ If you are going to use Linq like that then you don't even need Dictionary. I am not getting the AddRange? This would be so much less code with just add Application to Server and use a List with Linq. Server does not know Application and I bet that would be useful. \$\endgroup\$ – paparazzo Apr 28 '17 at 20:04
  • \$\begingroup\$ At one time, I was trying to use IEnumerable<KeyValuePair<T1,T2>> to accommodate a generic interface. Once I moved to a proper dictionary, my mind must have been stuck on using LINQ with the IEnumerable. Sad moment for my brain. \$\endgroup\$ – Tyler Daniels Apr 28 '17 at 20:17
4
\$\begingroup\$

I feel that both the mapping code and the fetch code could be more efficient by better utilizing LINQ projection.

more efficient yes but this time without linq.


if (serverDictionary.ContainsKey(applicationName))
    serverDictionary[applicationName].Add(server);
else
    serverDictionary.Add(applicationName, new List<Server> { server });

This is a very inconvenient way of adding new items to your dictionary because you are accessing it twice for each new item. With TryGetValue you can do the same with only one access to it. This means that if you could retrieve the value then you already have and it's ready to use, there's no need to use the indexer anymore. If however it's not there, then you just add it.

if (serverDictionary.TryGetValue(applicationName, out List<Server> servers))
{
    servers.Add(server);
}
else 
{
    serverDictionary[applicationName] = new List<Server> { server };
}

_appServers.Where(pair => pair.Key.EqualsIgnoreCase(applicationName))
    .ForEach(pair => servers.AddRange(pair.Value));

This might be super inefficient. If you use a dictionary then don't use linq if you don't have to. For fetching you can use the TryGetValue method agian:

return
    serverDictionary.TryGetValue(applicationName, out List<Server> servers) 
        ? servers
        : Enumerable.Empty<Server>();

Retrieving an item with TryGetValue means it's an O(1) operation whereas the same done with the original query means that the entire dictionary needs to be searched which means O(n). If you need to return a copy of the server list the you might use linq for it and call .ToList() on the servers.

There is one more thing about the linq query. You search for the key by ignoring the case. If you have multiple names but with different case then the resulting list will contain items of all those keys. Is this really what you want? The Where won't stop at the first match.

I don't know how you create the dictionary but if you want to ignore the case of the key you should initialize the dictionary appropriately by specifying the key comparer:

serverDictionary = new Dictionary<string, List<Server>>(StringComparer.OrdinalIgnoreCase);

Now the keys is case-insensitive also during adding.

\$\endgroup\$
  • \$\begingroup\$ ContainsKey is O(1) not O(n) \$\endgroup\$ – paparazzo Apr 28 '17 at 21:42
  • \$\begingroup\$ @Paparazzi I'm refering to the second example where OP uses _appServers.Where and not the ContainsKey. \$\endgroup\$ – t3chb0t Apr 29 '17 at 7:27
0
\$\begingroup\$

If you are going to use Linq like that then you don't even need Dictionary. I am not getting the AddRange?

This would be so much less code with just add Application to Server and use a List with Linq. Server class does not know Application and I bet that would be useful.

With List LINQ

public class Server
{
    public string Name { get; set; }
    public string Family { get; set; }
    public string Environment { get; set; }
    public string Application { get; set; }
}
public class ServersSCV
{
    string fileName;
    List<Server> servers = new List<Server>();
    public IEnumerable<Server> Servers { get { return servers.OrderBy(x => x.Application).ThenBy(x => x.Family).ThenBy(x => x.Name); } }
    public IEnumerable<Server> GetServerByApplicastion(string Application)
    {
        return servers.Where(x => string.Compare(x.Application, Application, true) == 0).OrderBy(x => x.Family).ThenBy(x => x.Name);
    }
    public void RefreshData()
    {
        servers.Clear();

        using (TextReader textReader = File.OpenText(fileName))
        using (CsvReader reader = new CsvReader(textReader))
        {
            while (reader.Read())
            {
                Server server = reader.GetRecord<Server>();
                servers.Add(server);
            }
        }
    }
    public ServersSCV (string FileName)
    {
        fileName = FileName;
        RefreshData();
    }
}
\$\endgroup\$
  • \$\begingroup\$ I see what you mean (and it would be easier), but the problem I have with that approach is the application name isn't a property of the server. The .csv just uses it as a key for the association between them. \$\endgroup\$ – Tyler Daniels Apr 28 '17 at 21:27
  • \$\begingroup\$ @SeeSharpCode On the surface "software application" sure sounds like a property of the server. Just the fact you search on it I would call it a property of the server. Adding it as property breaks nothing. At any rate this lets you search on it with a simpler structure. \$\endgroup\$ – paparazzo Apr 28 '17 at 21:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.