Derzeit versuche ich, den besten Weg zu finden, um eine Klasse umzugestalten, die wie folgt aussieht:

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;
    }

Da sich die Suche basierend auf dem Wert in model.Attribute ändert, wird eine switch-Anweisung verwendet. Der Großteil des Codes ist jedoch nicht vom Attribut abhängig. Gibt es eine Möglichkeit, dies umzugestalten, um die switch-Anweisungen zu entfernen oder auf nur eine zu reduzieren?

c#
0
CodePull 19 Jän. 2019 im 02:06

4 Antworten

Beste Antwort

Ich stimme TKK zu

Das heißt und um Ihre Frage basierend auf dem bereitgestellten Code zu beantworten, sehe ich nur eine Möglichkeit, eine der switch-Anweisungen in dieser Methode zu entfernen.

Die erste switch-Anweisung setzt sqlcommand und die Parameter und die zweite bleibt in der while-Schleife unverändert.

Deklarieren Sie diese oben in Ihrer Methode:

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

Ändern Sie die erste switch-Anweisung, um die Parameter zum Befehl sql hinzuzufügen:

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;
}

Entfernen Sie diesen Codeblock:

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;                                             
}

Und fügen Sie nach Ihrem Fang endlich ein hinzu, um die Verbindungs- und Befehlsobjekte zu bereinigen:

 finally
 {
    conn.Dispose();
    cmd.Dispose();
 }
1
Andrew 18 Jän. 2019 im 23:48

Schalter sind ein Codegeruch, der besagt, dass Ihr Code nicht OO ist. In diesem Fall können verschiedene Arten von SearchModel mit einer search -Methode für jeden Typ unterschiedlich implementiert werden.

1
StackOverthrow 18 Jän. 2019 im 23:20

Ich vervielfache es ein wenig, ich habe nur einen Schalter entfernt, aber es ist höchst unmöglich, zwei zu entfernen, vielleicht verwenden Sie diesen:

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);
            }
        }
    }
}
1
Wojciech Rak 19 Jän. 2019 im 00:22

Die anderen Antworten sind voller Anti-Muster. Die Notwendigkeit der switch-Anweisungen (insbesondere so oft wiederholt) scheint eine Gelegenheit zu sein, sich in eine OOP-Richtung zu bewegen.

Ich habe das Ganze nicht überarbeitet, sondern nur, um Ihnen eine Idee zu geben.

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);
        }
    }
}

Bei diesem Ansatz ist der Wartungsaufwand geringer, da Sie nicht viele verschiedene switch-Anweisungen ändern müssen, wenn Sie einen anderen Modelltyp anschließen müssen, und es ist leichter zu erkennen, wo die Logik für einen bestimmten Suchtyp liegt. Trotzdem bin ich kein Fan der Mutation des Eingabeparameters und denke, dass dies weiter überarbeitet werden könnte.

Das heißt, Sie können sehen, wie subjektiv dies ist und für SO unangemessen sein kann. Hier scheint es ein Problem mit dem grundlegenden Design zu geben.

0
Switch386 19 Jän. 2019 im 11:14