0
\$\begingroup\$

I want to perform a filter on a DataGridView using 3 different TextBox. The solution below works, but does not consider very effective and good. Is there a way to improve it?

private void textBoxCod_TextChanged(object sender, EventArgs e)
{
    Filter();
}

private void textBoxArt_TextChanged(object sender, EventArgs e)
{
    Filter();
}

private void textBoxDesc_TextChanged(object sender, EventArgs e)
{
    Filter();
}

private void Filter()
{
    string queryFilter = "1=1"; //With this we avoid to check if there is the AND in the query
    var filterCod = textBoxCod.Text;
    var filterArt = textBoxArt.Text;
    var filterDescr = textBoxDesc.Text();
    if (filterCod != "")
        queryFilter += " AND product_code LIKE '%" + filterCod + "%'";
    if(filterArt != "")
        queryFilter += " AND article LIKE '%" + filterArt + "%'";
    if (filterDescr != "")
        queryFilter += " AND description LIKE '%" + filterDescr + "%'";

    bSource.Filter = queryFilter;
    dataGridView.DataSource = bSource;
}

Before I used the string.Format but I realized that not having optional parameters could not combine several filters.

\$\endgroup\$
3
  • \$\begingroup\$ Your code appears to be susceptible to SQL injection. owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet \$\endgroup\$
    – aydjay
    Apr 19, 2016 at 11:42
  • 1
    \$\begingroup\$ @d347hm4n - The actual code is being protected from SQL injection. Here I removed everything that did not fit in the case \$\endgroup\$ Apr 19, 2016 at 11:48
  • \$\begingroup\$ Great, as long as you are aware :) \$\endgroup\$
    – aydjay
    Apr 19, 2016 at 11:50

3 Answers 3

3
\$\begingroup\$

You can remove the 1=1 hack by utilizing a collection of some sort, such as a List<string>, and String.Join.

Including braces with your if blocks aids readability.

You can avoid setting bindingSource filter if there are not actually any queries to run, and there is no need to reset the grids datasource:

    private void Filter()
    {
        var queries = new List<string>();
        //string queryFilter = "1=1"; //With this we avoid to check if there is the AND in the query

        var filterCod   = textBoxCod.Text;
        var filterArt   = textBoxArt.Text;
        var filterDescr = textBoxDesc.Text();

        if (filterCod != "")
        {
            queries.Add(String.Format("codice_prodotto LIKE '%{0}%'", filterCod));
        }

        if (filterArt != "")
        {
            queries.Add(String.Format("articolo LIKE '%{0}%'", filterArt));
        }

        if (filterDescr != "")
        {
            queries.Add(String.Format("descrizione_pt LIKE '%{0}%'", filterDescr));
        }

        if (queries.Count >= 1)
        {
            var queryFilter = String.Join(" AND ", queries);
            bSource.Filter = queryFilter;
            //dataGridView.DataSource = bSource;
        }

    }
\$\endgroup\$
4
\$\begingroup\$

Adding few improvements to @Steve's answer:

  • It is always good to check for spaces(blanks): if(!string.IsNullOrWhiteSpace(filterCod))

  • Wouldn't be nicer to have queries.Any() instead of queries.Count >= 1 ?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Both good points, as an infrequent .net user, i had forgotten the Any() method \$\endgroup\$
    – Steve
    Apr 19, 2016 at 12:32
1
\$\begingroup\$

In addition to the other suggestions, I'd also advise you to consider moving away from pure SQL and instead use an ORM like Entity Framework or NHibernate, or even Dapper.NET.


And also consider splitting up your project into layers. Right now you're constructing SQL in your UI layer, which is all kinds of wrong and a maintainability nightmare going forward. Even your simplified example takes 30+ lines, I hesitate to guess how big the full .cs file is...

Take a look at these suggestions. I realize this seems like a lot of work, but the pay-off in the long run is well worth it. Dealing with well-structured code in small classes instead of managing a 1000+ lines .aspx.cs file is far more pleasant.

\$\endgroup\$
3
  • \$\begingroup\$ Whilst modular code is certainly a good idea, this code looks to be Windows Forms, with a BindingSource. There may well be no RDBMS msdn.microsoft.com/en-us/library/… \$\endgroup\$
    – Steve
    Apr 19, 2016 at 12:37
  • \$\begingroup\$ @Steve There is a mysql tag and talk of SQL injection. And the code could be ASP.NET. \$\endgroup\$
    – BCdotWEB
    Apr 19, 2016 at 13:03
  • \$\begingroup\$ Fair point! Dont know how i missed the sql tag. I have never used webforms, so i just presumed winforms. All good either way. \$\endgroup\$
    – Steve
    Apr 19, 2016 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.