3
\$\begingroup\$

I'm building an in house query tool. The main form has a drop down which allows the user to select how they want to find scripts, and a drop down for what server they want to run the query on. There is also a text box to allow the user to populate a specific value which can be left blank. I think I did pretty well here. This is my first desktop WinForms app.

What Happens When The Query Is Run

private void QueryButton_Click(object sender, EventArgs e)
{
    SqlConnection connection = GetSqlConnection();
    string value = this.ValueTextBox.Text;
    TableAdapters.FindScript dataSource = new TableAdapters.FindScript();

    switch (this.FindByComboBox.SelectedItem.ToString())
    {
        case "Find Scripts By Target VDN":
            this.FindScriptsDataGrid.DataSource = dataSource.FillByTargetVDN(connection, value).Tables[0];
            break;
        case "Find Scripts By Skill Group":
            this.FindScriptsDataGrid.DataSource = dataSource.FillBySkillGroup(connection, value).Tables[0];
            break;
        case "Find Scripts By Translation Route Pool":
            this.FindScriptsDataGrid.DataSource = dataSource.FillByTranslationRoutePool(connection, value).Tables[0];
            break;
        case "Find Scripts By Name":
            this.FindScriptsDataGrid.DataSource = dataSource.FillByName(connection, value).Tables[0];
            break;
        case "Find Scripts By Label":
            this.FindScriptsDataGrid.DataSource = dataSource.FillByLabel(connection, value).Tables[0];
            break;
    }

    foreach (DataGridViewRow row in this.FindScriptsDataGrid.Rows)
    {
        row.HeaderCell.Value = (row.Index + 1).ToString();
    }
}

// SERVER NAMES CENSORED IN THIS POST FOR SECURITY
private SqlConnection GetSqlConnection()
{
    SqlConnection connection = new SqlConnection();
    switch (this.ServerComboBox.SelectedItem.ToString())
    {
        case "SERVER1":
            connection.ConnectionString = ConfigurationManager.ConnectionStrings["csS01"].ConnectionString;
            break;
        case "SERVER2":
            connection.ConnectionString = ConfigurationManager.ConnectionStrings["csS02"].ConnectionString;
            break;
    }
    return connection;
}

The Queries Being Called

class TableAdapters
{
    public class FindScript
    {
        string conditional = string.Empty;
        string query = string.Empty;

        DataSet Fill(SqlConnection connection, string query)
        {
            SqlDataAdapter adapter = new SqlDataAdapter(query, connection);
            DataSet dataSet = new DataSet();
            dataSet.Tables.Add(new DataTable());
            adapter.Fill(dataSet.Tables[0]);
            return dataSet;
        }

        public DataSet FillByTargetVDN(SqlConnection connection, string parameter)
        {
            if (parameter.Length > 0)
            {
                conditional = "AND svce.PeripheralNumber= " + parameter + " ";
            }
            query = "SELECT svce.PeripheralNumber AS Service, "
                + "ms.EnterpriseName AS [Script Name], "
                + "p.EnterpriseName AS Peripheral, " 
                + "sg.PeripheralNumber AS [Skill Mapping], " 
                + "s.Version AS [Latest Version], "
                + "s.DateTime AS Created, " 
                + "s.Author " 
                + "FROM Script_Cross_Reference scr "
                + "LEFT OUTER JOIN Script s ON s.ScriptID = scr.ScriptID "
                + "LEFT OUTER JOIN Master_Script ms ON s.MasterScriptID = ms.MasterScriptID "
                + "LEFT OUTER JOIN Service svce ON svce.SkillTargetID = scr.ForeignKey "
                + "LEFT OUTER JOIN Peripheral p ON svce.PeripheralID = p.PeripheralID "
                + "LEFT OUTER JOIN Service_Member sm ON svce.SkillTargetID = sm.ServiceSkillTargetID " 
                + "LEFT OUTER JOIN Skill_Group sg ON sg.SkillTargetID = sm.SkillGroupSkillTargetID " 
                + "WHERE s.Version = ms.CurrentVersion AND scr.TargetType = 1 "
                + conditional
                + "ORDER BY svce.PeripheralNumber, ms.EnterpriseName, p.EnterpriseName";
            return Fill(connection, query);
        }

        public DataSet FillBySkillGroup(SqlConnection connection, string parameter)
        {
            if (parameter.Length > 0)
            {
                conditional = "AND sg.PeripheralNumber = " + parameter + " ";
            }
            query = "SELECT DISTINCT(sg.PeripheralNumber) AS Skill, " 
                + "ms.EnterpriseName AS [Script Name], " 
                + "s.Version AS [Latest Version], " 
                + "s.DateTime AS Created, " 
                + "s.Author " 
                + "FROM Script_Cross_Reference scr "
                + "LEFT OUTER JOIN Script s ON s.ScriptID = scr.ScriptID "
                + "LEFT OUTER JOIN Master_Script ms ON s.MasterScriptID = ms.MasterScriptID "
                + "LEFT OUTER JOIN Skill_Group sg ON sg.SkillTargetID = scr.ForeignKey " 
                + "WHERE s.Version = ms.CurrentVersion AND scr.TargetType = 2 "
                + conditional 
                + "ORDER BY sg.PeripheralNumber, ms.EnterpriseName";
            return Fill(connection, query);
        }

        public DataSet FillByTranslationRoutePool(SqlConnection connection, string parameter)
        {
            if (parameter.Length > 0)
            {
                conditional = "AND (tr.EnterpriseName like ('" + parameter + "') "
                    + "OR tr.EnterpriseName like ('" + parameter.ToUpper() + "') "
                    + "OR tr.EnterpriseName like ('" + parameter.ToLower() + "')) ";
            }
            query = "SELECT DISTINCT(tr.EnterpriseName) AS Translation_Route, " 
                + "ms.EnterpriseName AS [Script Name], " 
                + "s.Version AS [Latest Version], " 
                + "s.DateTime AS Created, " 
                + "s.Author " 
                + "FROM Script_Cross_Reference scr " 
                + "LEFT OUTER JOIN Script s ON s.ScriptID = scr.ScriptID " 
                + "LEFT OUTER JOIN Master_Script ms ON s.MasterScriptID = ms.MasterScriptID " 
                + "LEFT OUTER JOIN Translation_Route tr ON scr.ForeignKey = tr.SkillTargetID " 
                + "WHERE s.Version = ms.CurrentVersion AND scr.TargetType = 4 "
                + conditional 
                + "ORDER BY tr.EnterpriseName, ms.EnterpriseName";
            return Fill(connection, query);
        }

        public DataSet FillByName(SqlConnection connection, string parameter)
        {
            if (parameter.Length > 0)
            {
                conditional = "AND (ms.EnterpriseName LIKE ('" + parameter + "') "
                    + "OR ms.EnterpriseName LIKE ('" + parameter.ToUpper() + "') "
                    + "OR ms.EnterpriseName LIKE ('" + parameter.ToLower() + "')) ";
            }
            query = "SELECT ms.EnterpriseName AS [Script Name], "
                + "s.Author, "
                + "s.Version, "
                + "s.Valid, "
                + "s.DateTime AS Created "
                + "FROM Script s "
                + "LEFT OUTER JOIN Master_Script ms ON s.MasterScriptID = ms.MasterScriptID "
                + "WHERE s.Version = ms.CurrentVersion "
                + conditional
                + "ORDER BY ms.EnterpriseName";
            return Fill(connection, query);
        }

        public DataSet FillByLabel(SqlConnection connection, string parameter)
        {
            if (parameter.Length > 0)
            {
                conditional = "AND l.Label='" + parameter + "' ";
            }
            query = "SELECT DISTINCT l.Label AS Label, "
                + "ms.EnterpriseName AS [Script Name], "
                + "s.Version AS [Latest Version], "
                + "s.DateTime AS Created, "
                + "s.Author "
                + "FROM Script_Cross_Reference scr "
                + "LEFT OUTER JOIN Script s ON s.ScriptID = scr.ScriptID "
                + "LEFT OUTER JOIN Master_Script ms ON s.MasterScriptID = ms.MasterScriptID "
                + "LEFT OUTER JOIN Label l ON l.LabelID = scr.ForeignKey "
                + "WHERE s.Version = ms.CurrentVersion AND scr.TargetType = 19 "
                + conditional
                + "ORDER BY l.Label, ms.EnterpriseName";
            return Fill(connection, query);
        }
    }
}

Now I already have some concerns about the whole conditional thing in the TableAdapters class which I address here. Other than that are there other areas of improvement that you could point out to me?

\$\endgroup\$
  • 1
    \$\begingroup\$ I always cringe a little bit when I see raw SQL being concatenated. Your code becomes ripe for SQL injection attacks. \$\endgroup\$ – Greg Burghardt Jul 2 '14 at 20:02
  • \$\begingroup\$ @GregBurghardt when I'm working on web based applications I utilize MVC and usually write in LINQ, however this is an intranet winform desktop app so I'm not to concerned with SQL injection attacks. I actually wrote this app as an MVC web app at one point but we had to pull it because of server loads. I could never translate these queries into LINQ for some reason they never returned the correct result sets. \$\endgroup\$ – aaronmallen Jul 2 '14 at 20:08
  • \$\begingroup\$ Still, it feels like there should be a safe way to generate SQL queries based on strings and variable substitutions in C#. Maybe I just haven't seen it yet. \$\endgroup\$ – Greg Burghardt Jul 2 '14 at 20:10
  • \$\begingroup\$ @GregBurghardt I'm curious if the loss in readability would be worth the extra security in this case. \$\endgroup\$ – aaronmallen Jul 2 '14 at 20:13
3
\$\begingroup\$
  • I assume it's your style to refer to class members using "this.". That's not the usual way to do things but it is acceptable.
  • It also seems to be your style to capitalize the names of the UI elements. Again, not the usual style but not horrible (it does confuse the syntax coloration here though).
  • You really need to separate the UI and the business logic. The way things stand now, it would be impossible to drive the program via an API or a batch process. Especially notable since that's usually the best way to test it.
  • I don't believe it's ideal for conditional to be a class member of FindScript. There's no reason it needs to persist.
  • And I'm pretty sure all of the FindScript class could get replaced by a Dictionary<String,String>, maybe with some "{0}" in there so you can use String.Format().
  • That big switch statement in QueryButton_Click() could be refactored into a dictionary.

    For instance:

    private static TableAdapters.FindScript dataSource = new TableAdapters.FindScript();
    
    private static readonly Dictionary<String, Func<SqlConnection,String,DataSet>> switchReplacement = new Dictionary()
    {
        {"Find Scripts By Target VDN", dataSource.FillByTargetVDN},
        {"Find Scripts By Skill Group", dataSource.FillBySkillGroup},
        {"Find Scripts By Translation Route Pool", dataSource.FillByTranslationRoutePool},
        {"Find Scripts By Name", dataSource.FillByName},
        {"Find Scripts By Label", dataSource.FillByLabel}
    };
    

    And then in the function, you can avoid repeating yourself:

    SqlConnection connection = GetSqlConnection();
    string value = this.ValueTextBox.Text;
    this.FindScriptsDataGrid.DataSource = switchReplacement[this.FindByComboBox.SelectedItem.ToString()](connection, value).Tables[0];
    

    Similarly, the switch in GetSqlConnection() could be a Dictionary<String,String>, to avoid duplicating code.

\$\endgroup\$
  • \$\begingroup\$ Big help here. I renamed my UI variables wasn't sure if I should count those as local or not. Also got rid of this that was actually auto generated by Visual Studios for the most part, never use this when working with MVC web apps. As the post states I'm already aware of the issue with conditional and it's been remedied. I was completely unaware of the Dictionary type and its awesome. Thanks! \$\endgroup\$ – aaronmallen Jul 3 '14 at 14:26
  • \$\begingroup\$ quick question though... I had added if (!isNumber) { MessageBox.Show("Error Message"); break; } logic to two of the case statements in my first big switch statement because in two of the options the value needed to be type int. How can I now add back that functionality? If you could Answer here \$\endgroup\$ – aaronmallen Jul 3 '14 at 14:34

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.