1
\$\begingroup\$

I have a WPF application (using MVVM pattern) that allows users to perform a search for a record/records based on their selection choices and entries into the different UI controls (TextBox, CheckBox, RadioButton, DatePicker, TimerPicker,etc). It does this by retrieving information from an Access database.

I have an Access query method in my AccessWorker class that looks like this (note that this is simplified a bit, because I can't post everything I have):

public DataSet GetRecords(string customQuery = "")
{
    var data = new DataSet();

    try
    {
        using (var con = new OleDbConnection(ConnectionString))
        {
            string queryString = "SELECT * FROM Records " + customQuery;

            using (var da = new OleDbDataAdapter(queryString, con))
            {
                da.Fill(data, "Records");
            }
        }
    }
    catch (Exception e)
    {
        ExceptionLogger.Record(e.Message);
    }

    return data;
}

As you can see, I pass a custom query argument to this method in a form of a string. I construct this query string using a StringBuilder in my ViewModel and then I pass it to GetRecords() method. Here's the GetQueryString() method that does the string building:

private string GetQueryString()
{
    var query = new StringBuilder();

    if (!String.IsNullOrWhiteSpace(RecNumber))
    {
        query.Append("Record_Number = '" + RecNumber + "'");
    }

    if (!String.IsNullOrWhiteSpace(EmployeeId))
    {
        if (!String.IsNullOrWhiteSpace(query.ToString()))
        {
            query.Append(" AND ");
        }

        query.Append("Employee_ID LIKE '%" + EmployeeId + "%'");
    }

    if (!String.IsNullOrWhiteSpace(Location))
    {
        if (!String.IsNullOrWhiteSpace(query.ToString()))
        {
            query.Append(" AND ");
        }

        query.Append("Location LIKE '%" + Location + "%'");
    }

    if (!String.IsNullOrWhiteSpace(FirstName))
    {
        if (!String.IsNullOrWhiteSpace(query.ToString()))
        {
            query.Append(" AND ");
        }

        query.Append("First_Name LIKE '%" + FirstName + "%'");
    }

    if (!String.IsNullOrWhiteSpace(LastName))
    {
        if (!String.IsNullOrWhiteSpace(query.ToString()))
        {
            query.Append(" AND ");
        }

        query.Append("Last_Name LIKE '%" + LastName + "%'");
    }

    //...and so on...
    return String.Format("WHERE {0}", query.ToString());
}

The above method is severely shortened here so that I wouldn't have to post a very long wall of code, but I'm dealing with 25 different controls and so there could be a condition for everyone of them that would have to be part of this query string. The method itself, as you can imagine, is rather too large by anyone's standard. When I took a C++ class in college, my instructor told me that if I can help it, I should keep my methods no longer than what I can see on the screen. I'm bowing my head in shame... heh

I don't believe that this is the most elegant way of accomplishing this task. Unfortunately, I haven't come up with a better approach. I do my best at trying to learn new patterns every day, but I've been programming for under a year and a half, so any guidance would be greatly appreciated.

By the way, based on some of the comments, I have to clear up that I'm using properties that are bound to control, not controls' values....

\$\endgroup\$
  • \$\begingroup\$ Seems like your method/database would be highly susceptible to SQL injection if you just concatenate SQL into it. \$\endgroup\$ – Phrancis May 23 '14 at 14:46
2
\$\begingroup\$

My initial thought is that since view models are only (or should only be) concerned with presentation logic, it shouldn't know how to construct an SQL string. I would move the code which builds up the query into a separate Query Object. Also I would have some helper methods (e.g. extension methods) to try and get rid of the repetitive null/empty/whitespace checks. For example:

public class GetRecordsQuery
{
    // If there's a lot of parameters, maybe refactor this to accept an object
    public void Execute(string recordNumber, string employeeId, string location,
                        string firstName, string lastName)
    {
        var query = new StringBuilder();

        query
            .Eq("Record_Number", recordNumber).And()
            .Like("Employee_ID", employeeId).And()
            .Like("Location", location).And()
            .Like("First_Name", firstName).And()
            .Like("Last_Name", lastName)
            .EndQuery();

        // or however AccessWorker members are invoked
        return AccessWorker.GetRecords(String.Format("WHERE {0}", query));
    }
}

public static class StringBuilderExtensions
{
    private const string AndOp = " AND ";

    public static StringBuilder And(this StringBuilder sb)
    {
        var str = sb.ToString();

        if (String.IsNullOrWhiteSpace(str))
            return sb;

        if (str.EndsWith(AndOp))
            return sb;

        return sb.Append(AndOp);
    }

    public static StringBuilder EndQuery(this StringBuilder sb)
    {
        var str = sb.ToString();

        if (String.IsNullOrWhiteSpace(str))
            return sb;

        if (!str.EndsWith(AndOp))
            return sb;

        var length = AndOp.Length;
        var start = sb.Length - length;

        return sb.Remove(start, length);
    }

    public static StringBuilder Eq(this StringBuilder sb, string column, string value)
    {
        if (String.IsNullOrWhiteSpace(value))
            return sb;

        return sb.AppendFormat("{0} = '{1}'", column, value);
    }

    public static StringBuilder Like(this StringBuilder sb, string column, string value)
    {
        if (String.IsNullOrWhiteSpace(value))
            return sb;

        return sb.AppendFormat("{0} LIKE '%{1}%'", column, value);
    }
}

EDIT 1: Corrected code to prevent AND operators being appended when one already exists.

EDIT 2: Added an EndQuery method to trim off any trailing AND operators.

\$\endgroup\$
1
\$\begingroup\$

Just from the title of your question one could tell there's a problem: UI and data have nothing to do in the same sentence, and seeing the two intertwined in code is a big fishy smell, especially in a WPF/MVVM application.

@George is 100% correct, your ViewModel shouldn't know how to build a SQL string (and +1 for ditching repositories!).

What you have here is basically a search feature. You're not going to search with UI controls, you're going to search with the ViewModel property values that are bound to the values of these UI controls.

Let's see what the search criterion can be:

  • Record number
  • Employee ID
  • Location
  • First name
  • Last name

Whether these values come from a DatePicker, a RadioButton or a TextBox is completely irrelevant; your code needs to abstract away these things, and leave the UI at the View level.

I don't like string-concatenating SQL. Normally I'd probably write a small stored procedure for such a search, but if I remember correctly Access databases don't have that, so I'd probably try to get something like this to work:

SELECT * FROM Records 
WHERE Record_Number = ISNULL(@recordNumber, Record_Number)
  AND Employee_ID LIKE '%' + ISNULL(@employeeId, Employee_ID) + '%'
  AND Location LIKE '%' + ISNULL(@location, Location) + '%'
  AND First_Name LIKE '%' + ISNULL(@firstName, First_Name) + '%'
  AND Last_Name LIKE '%' + ISNULL(@lastName, Last_Name) + '%'

My Access is very rusty though, but I'm sure there's a way to pass it parameterized queries. If not named ones like above, ordered ones like this:

SELECT * FROM Records 
WHERE Record_Number = ISNULL(?, Record_Number)
  AND Employee_ID LIKE '%' + ISNULL(?, Employee_ID) + '%'
  AND Location LIKE '%' + ISNULL(?, Location) + '%'
  AND First_Name LIKE '%' + ISNULL(?, First_Name) + '%'
  AND Last_Name LIKE '%' + ISNULL(?, Last_Name) + '%'

And then passing in the parameter values in that specific order. The idea is to let the database deal with putting in the parameter values, doing that in code is asking for SQL injection.

The idea is to use a good-for-all-searches query, so that all searches execute the very same query on the database side, only with different parameters. Then you can leverage indexes and caching... although I'm not sure Access does that, but it can't hurt.

\$\endgroup\$
  • \$\begingroup\$ MS Access, in lieu of stored procedure, can "save" a query and then execute it. This may offer some insight. Not sure why one would want to use Access when they can get the much more solid MySQL for free, but that's neither here nor there I guess. stackoverflow.com/questions/9929009/… \$\endgroup\$ – Phrancis May 23 '14 at 14:49
  • 1
    \$\begingroup\$ I'd use MS SQL Server Express, free as well ;) \$\endgroup\$ – Mathieu Guindon May 23 '14 at 14:51
  • 1
    \$\begingroup\$ I do use properties by the way and I'm working with an existing database, so I don't have say in whether it's an Access, SQLite, MS SQL Server Express, Oracle, whatever. Got to work with what I have. I do like the stored procedure method though. I'm going to see if I can get it done with Access. EDIT: Yep, Access has stored procedures. I'll be testing it to find out how it works out and get back to you. \$\endgroup\$ – B.K. May 23 '14 at 16:55

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.