Load from dictionary or database if not found

Today I have coded a simple function that will get a room model, if its already been loaded in to the dictionary then it will just return it, if its not in the dictionary if will try its best to load it from the database. I just wondered is this the best way I can do ths? or is there a better way…

public bool TryGetModel(string modelName, out RoomModel model)
{
    if (_roomModels.ContainsKey(modelName))
    {
        _roomModels.TryGetValue(modelName, out model);
        return true;
    }

    using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
    {
        mysqlConnection.SetQuery("SELECT id,door_x,door_y,door_z,door_dir,heightmap,public_items,club_only,poolmap,`wall_height` FROM `room_models` WHERE `custom`");
        var modelRow = mysqlConnection.GetRow();

        if (modelRow == null)
        {
            model = null;
            return false;
        }

        model = new RoomModel(Convert.ToInt32(modelRow["door_x"]), Convert.ToInt32(modelRow["door_y"]),
            Convert.ToDouble(modelRow["door_z"]), Convert.ToInt32(modelRow["door_dir"]),
            Convert.ToString(modelRow["heightmap"]), Convert.ToString(modelRow["public_items"]),
            Convert.ToInt32(modelRow["club_only"]).ToString() == "1", Convert.ToString(modelRow["poolmap"]),
            Convert.ToInt32(modelRow["wall_height"]));

        _roomModels.Add(Convert.ToString(modelRow["id"]), model); // save it for next time!!
        return true;
    }
}

Using ContainsKey() together with TryGetValue() seems a little bit too much. Just change it like so

if (_roomModels.TryGetValue(modelName, out model))
{
    return true;
}  

which is much easier to read and faster for items which are in the dictionary.

If the columns in the database allow null values you should check first with Convert.IsDBNull() so the method doesn’t blow in the users face at any of the Convert.ToXX() methods.

The Where clause of the sql query looks like it is not working.
You should at least let the columns have some space to breathe which makes the query easier to read.

Is there a reason not to use an ORM? You need 20 lines just to retrieve one RoomModel from the db, whereas with for instance Entity Framework this could be reduced to at most a handful.

How often do you call this method? Because as one comment suggested, you might be better off loading all models in a dictionary in one go instead of making hundreds of db calls.

I get the TryGetModel() logic and how you’re mimicking the logic of TryGetValue(), but isn’t it a bit much to return a bool and the model? Wouldn’t it be easier to simply return a RoomModel and test if you get a null or an actual value?

The way that you explain your code is different from what your code actually does…

if its already been loaded in to the dictionary then it will just return it

it doesn’t return the RoomModel directly, it returns a boolean value while indirectly returning the RoomModel in the out variable.

I am not seeing the need to return a boolean value, I think I would rather return the RoomModel itself than try to force a method to return a RoomModel and a boolean value.

Your method is violating the SRP(Single Responsibility Principle) in that you are merging 2 actions into one, checking the list for a specific RoomModel and retrieving a RoomModel from the Database.