Resolving connection strings with a static cache

All requests must be validated whether the given site URL exists in a database. If it exists, the connection string will be built based on a given URL and an SQL connection will be opened. I created a static cache for these URLs (key: site URL, value: database name) because I don't want to query the database every time I need to build a connection string for a specific URL. When a connection string is successfully created, an API controller checks if any car is available.

Controller

```
public class CarController : ApiController
{
[HttpPost]
public IHttpActionResult Available(SiteUrl model)
{
if (ModelState.IsValid)
{
var conStr = ResolveClientConnectrionString(model.Url);
if (conStr != "")
{
return Ok(CarHelper.CheckIfAnyAvailable(conStr));
}
}

return StatusCode(System.Net.HttpStatusCode.NoContent);
}

private static string ResolveClientConnectrionString(string url)
{
if (CarHelper.DBs.ContainsKey(url))
{
var clientConnectionString = GetConnectionStringForDatabaseName(ConfigurationManager.ConnectionStrings["MainConnectionString"].ConnectionString, CarHelper.DBs[url]);

return clientConnectionString;
}
else
{
var databaseName = "";
using (SqlConnection con = new SqlConnection(ConfigurationManager.ConnectionStrings["MainConnectionString"].ConnectionString))
{
con.Open();

var query = "Select Top(1) url, dbName from sites where url=@url";

using (SqlCommand cmd = new SqlCommand(query, con))
{
cmd.Parameters.AddWithValue("@url", url);
using (SqlDataReader rdr = cmd.ExecuteReader())
{
if (rdr.Read())
{
databaseName = (string)rdr["dbName"];

In Available() you may want to reduce indentation to make it easier to read. I'd also consider to return an appropriate code when URL isn't found in DB (it might make client debugging easier).

if (!ModelState.IsValid)
    return StatusCode(HttpStatusCode.NoContent);

var connectionString = ResolveClientConnectionString(model.Url);
if (conStr == "")
    return StatusCode(HttpStatusCode.NotFound);

return Ok(CarHelper.CheckIfAnyAvailable(connectionString));

ResolveClientConnectrionString(). "MainConnectionString" is repeated three times in your code. Move it to a private (or internal) const string field.

In this SQL code:

Select Top(1) url, dbName from sites where url=@url


You do not actually need url in returned dataset, just drop it. Also I'd suggest to use consistent casing for SQL commands (and to move it to a const string field, at least until you won't need more logic and you will introduce a separate class for that).

private const string SqlQueryDbNameByUrl
    = "SELECT TOP(1) dbName FROM sites WHERE url=@url";

This snippet is small and simple but in general when using AddWithValue() you should be careful, I'd suggest to get used to...don't use it at all (so you won't need to think when it's safe and when it's not).

You do not need to declare databaseName before its usage:

string databaseName = reader.GetString(0);
if (databaseName == "")
    return "";

CarHelper.DBs.Add(url, databaseName);
return GetConnectionStringForDatabaseName(MainConnectionString, databaseName);

Cache. It's just broken, ASP.NET is multi-threaded and Dictionary is not thread-safe and to read everything in ctor is pretty inefficient (unless you have just few entries, it's not really the purpose of a cache). You may use a thread-safe dictionary, you may lock it, you may use an in-memory database (!!!) or simply...make your life easy and reuse existing cache classes like MemoryCache. One note about existing code (which I'd simply drop): CarHelper should not be public and I see no reason for DBs to have a public setter. In this moment responsibility to manage this cache is shared between its users and CarHelper class. If you want a cache then abstract it:

static MyCacheIShouldNotUse
{
    public static string Get(string name, Func factory)
    {
        lock (_data)
        {
            if (_data.TryGetValue(name, out string value))
                return value;

            var newValue = factory(name);
            _data.Add(name, newValue );

            return newValue;         
        }
    }

    private static Dictionary _data = new Dictionary();
}

Given this proof of concept then factory can be dropped in favor of a static private method (in your cache if it's specialized or in a separate class where you manage DB access).

Now you might want to test the cache and static methods aren't the best for this. Let's make them instance methods and eventually provide a singleton (compromise...) instance like MyCacheIShouldNotUse.Default.

Too much work, right? MemoryCache is already there (and it will nicely also handle expiration policies).

Side note: I don't see your full code then I'm just guessing but usually to store URL in database (are you using them to identify the car?!) is not a great idea. URLs are mutable and parsing URL might be better (you're already using ASP.NET MVC, after all).

StackExchange Code Review Q#162443, answer score: 3