Refactor Method To Reduce Number Of Switch Statements - C#

Question

Currently, Im trying to find the best way to refactor a class that looks similar to the following:

public static SearchModel GetSearchResults(SearchModel model)
    {
        List<ResultModel> results = new List<ResultModel>();

        try
        {
            string sqlCommand = string.Empty;

            switch (model.Attribute)
            {
                case "Users":
                    sqlCommand = "GeneralUserSearch";
                    break;
                case "Favorites":
                    sqlCommand = "UserFavorites";
                    break;
                case "Email":
                    sqlCommand = "EmailSearch";
                    break;                                 
            }

            using (SqlConnection conn = new SqlConnection("connection string"))
            {
                conn.Open();

                using (SqlCommand cmd = AdoBase.GetSqlCommand(sqlCommand, conn))
                {                        
                    switch (model.Attribute)
                    {
                        case "Users":
                            if(!string.IsNullOrWhiteSpace(model.Name)) {
                                cmd.Parameters.AddWithValue("Name", model.Name);
                            }
                            if(!string.IsNullOrWhiteSpace(model.Username)) {
                                cmd.Parameters.AddWithValue("Username", model.Username);
                            }                                                           
                            break;
                        case "Favorites":
                            cmd.Parameters.AddWithValue("Favorites", model.Favorites);
                            break;
                        case "Email":
                            cmd.Parameters.AddWithValue("Email", model.Email);
                            break;                                             
                    }

                    using (SqlDataReader reader = cmd.ExecuteReader())
                    {
                        if (reader.HasRows)
                        {
                            while (reader.Read())
                            {
                                ResultModel result = new ResultModel();

                                switch (model.Attribute)
                                {
                                    case "Users":
                                        result.Users.Add(reader["User"]);                                       
                                        break;
                                    case "Favorites":
                                        result.User = reader["User"];
                                        result.Favorites = reader["Favorites"];
                                        break;
                                    case "Email":
                                        result.User = reader["User"];
                                        result.Email = reader["Email"];
                                        break;                                             
                                }

                                results.Add(result);
                            }
                        }
                    }

                }
            }
        }
        catch (Exception ex)
        {
            return ex;
        }

        return model;
    }

Since the search changes based on the value in model.Attribute, a switch statement is used. However, the majority of the code is not dependent upon Attribute. Is there a way to refactor this to eliminate the switch statements or reduce it down to only one?

Answer 1

The other answers are full of anti-patterns. The need for the switch statements (especially repeated so often) seem like an opportunity to move in a more OOP direction.

I haven't refactored the entire thing, but just to give you an idea.

public class SearchHelper
{
    //why does this need to return the model at all? the model isn't altered 
    //and would already be in scope for whatever is calling this method
    public static void GetSearchResults(SearchModel model)
    {
        List<ResultModel> results = new List<ResultModel>();

        try
        {
            using (SqlConnection conn = new SqlConnection("connection string"))
            {
                conn.Open();

                using (SqlCommand cmd = AdoBase.GetSqlCommand(model.SqlCommandName, conn))
                {
                    //this will mutate the object, so you don't need a return type. I'd suggest refactoring this further.
                    model.BuildSqlCommand(cmd);

                    using (SqlDataReader reader = cmd.ExecuteReader())
                    {
                        //your code sample wasn't returning this, but maybe you intended to?
                        BuildResultSet(reader);
                    }

                }
            }
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    private static IEnumerable<ResultModel> BuildResultSet(SqlDataReader reader)
    {
        var results = new List<ResultModel>();

        if (!reader.HasRows) { return results; }

        while (reader.Read())
        {
            ResultModel result = new ResultModel();

            //  ...the result composition would need to be refactored in a similar way as well
            results.Add(result);
        }
        return results;
    }
}

public abstract class SearchModel
{
    public string SqlCommandName { get; private set; }

    private SearchModel() { }

    protected SearchModel(string sqlCommandName)
    {
        SqlCommandName = sqlCommandName;
    }

    public abstract void BuildSqlCommand(SqlCommand command);
}

public class UserSearchModel : SearchModel
{
    public string Name { get; set; }

    public string Username { get; set; }

    public UserSearchModel() : base("GeneralUserSearch")
    {
    }

    //warning...this mutates the input parameter
    public override void BuildSqlCommand(SqlCommand command)
    {
        if (!string.IsNullOrWhiteSpace(Name))
        {
            command.Parameters.AddWithValue(nameof(Name), Name);
        }
        if (!string.IsNullOrWhiteSpace(Username))
        {
            command.Parameters.AddWithValue(nameof(Username), Username);
        }
    }
}

Taking this approach, it's less maintenance because you won't have to modify many different switch statements if you need to plug in another type of model, and it's more easily identifiable where the logic for a given type of search lives. That said, I'm not a fan of the mutation of the input parameter, and think that could be refactored further.

That said, you can see how this is subjective and may be inappropriate for SO. There seems to be an issue with the fundamental design here.

Answer 2

I siplify it a little bit, I removed only one switch, but it's highly impossible to remove two, maybe you use this:

public static SearchModel GetSearchResults(SearchModel model)
{
    List<ResultModel> results = new List<ResultModel>();
    SqlCommand cmd = new SqlCommand(); // only for compile for finnaly dispose
    try
    {
        using (SqlConnection conn = new SqlConnection("connection string"))
        {
            conn.Open();
            string sqlCommand = string.Empty;

            switch (model.Attribute)
            {
                case "Users":
                    cmd = AdoBase.GetSqlCommand("GeneralUserSearch", conn);
                    if (!string.IsNullOrWhiteSpace(model.Name)) {
                        cmd.Parameters.AddWithValue("Name", model.Name);
                    }
                    if (!string.IsNullOrWhiteSpace(model.Name)) { // redundant if or mistake, and there should be model.Username
                        cmd.Parameters.AddWithValue("Username", model.Username);
                    }
                    break;
                case "Favorites":
                    cmd = AdoBase.GetSqlCommand("UserFavorites", conn);
                    cmd.Parameters.AddWithValue("Favorites", model.Favorites);
                    break;
                case "Email":
                    cmd = AdoBase.GetSqlCommand("EmailSearch", conn);
                    cmd.Parameters.AddWithValue("Email", model.Email);
                    break;
            }
            SimpleMethod(results, cmd, model);
        }
    }
    catch (Exception ex)
    {
        return ex;
    }
    finally
    {
        cmd.Dispose();
    }
    return model;
}

SimpleMethod:

public static void SimpleMethod(List<ResultModel> results, SqlCommand cmd, SearchModel model)
{
    using (SqlDataReader reader = cmd.ExecuteReader())
    {
        if (reader.HasRows)
        {
            while (reader.Read())
            {
                ResultModel result = new ResultModel();

                switch (model.Attribute)
                {
                    case "Users":
                        result.Users.Add(reader["User"]);
                        break;
                    case "Favorites":
                        result.User = reader["User"];
                        result.Favorites = reader["Favorites"];
                        break;
                    case "Email":
                        result.User = reader["User"];
                        result.Email = reader["Email"];
                        break;
                }
                results.Add(result);
            }
        }
    }
}

Answer 3

I agree with TKK

That said and to answer your question based on the code provided, I only see a way to eliminate one of the switch statements in this method.

The first switch statement will set sqlcommand and the parameters and the second would remain unchanged in the while loop.

Declare these at the top of your method:

`SqlConnection conn = new SqlConnection("connection string");
 SqlCommand cmd = AdoBase.GetSqlCommand("", conn);`

Change the first switch statement to add the parameters to the sql command:

switch (model.Attribute)
{
    case "Users":
        sqlCommand = "GeneralUserSearch";
        if (!string.IsNullOrWhiteSpace(model.Name))
        {
            cmd.Parameters.AddWithValue("Name", model.Name);
        }

        if (!string.IsNullOrWhiteSpace(model.Name))
        {
            cmd.Parameters.AddWithValue("Username", model.Username);
        }

        break;
    case "Favorites":
        sqlCommand = "UserFavorites";
        cmd.Parameters.AddWithValue("Favorites", model.Favorites);
        break;
    case "Email":
        sqlCommand = "EmailSearch";
        cmd.Parameters.AddWithValue("Email", model.Email);
        break;
}

Remove this block of code:

switch (model.Attribute)
{
    case "Users":
        if(!string.IsNullOrWhiteSpace(model.Name)) {
            cmd.Parameters.AddWithValue("Name", model.Name);
        }
        if(!string.IsNullOrWhiteSpace(model.Name)) {
            cmd.Parameters.AddWithValue("Username", model.Username);
        }                                                           
        break;
    case "Favorites":
        cmd.Parameters.AddWithValue("Favorites", model.Favorites);
        break;
    case "Email":
        cmd.Parameters.AddWithValue("Email", model.Email);
        break;                                             
}

And add a finally after your catch to clean up the connection and command objects:

 finally
 {
    conn.Dispose();
    cmd.Dispose();
 }

Answer 4

Switches are a code smell that says your code isn't OO. In this case you could have different types of SearchModel with a search method implemented differently for each type.