0

I need to retrieve the table and check if the variable 'verb' exists in either one of the three columns and then pass that specific row's values to a Verb Object, as stated below.

In this code however, the if condition that reads the columns skips through the condition as if the field doesn't exist in the table even though it actually does. Are there any better ways to do this?

public static Verb GetVerbs(string verb)
{
    List<string> Verbs = new List<string>();
    Verb v = new Verb();

    SqlConnection conn = new SqlConnection("Data Source=.;Initial Catalog=QABase;Integrated Security=True");

    try
    {
        conn.Open();
    }
    catch (Exception e)
    {
        System.Console.WriteLine(e.ToString());
    }

    try
    {
        SqlDataReader myReader = null;

        SqlCommand myCommand = new SqlCommand("select * from EnglishVerbs", conn);

        myReader = myCommand.ExecuteReader();

        while (myReader.Read())
        {
            if ((myReader["BaseForm"].ToString().ToLower().Contains(verb.ToLower())) 
                 || (myReader["PastForm"].ToString().ToLower().Contains(verb.ToLower())) 
                 || (myReader["PastPartForm"].ToString().ToLower().Contains(verb.ToLower())))
            {
                v.BaseTense = Convert.ToString(myReader["BaseForm"]);
                v.PastTense = Convert.ToString(myReader["PastForm"]);
                v.PastParticiple = Convert.ToString(myReader["PastPartForm"]);
            }
            else
            {
                //row doesnt exist
                v.BaseTense = null;
                v.PastTense = null;
                v.PastParticiple = null;
            }
        }
    }
    catch (Exception e)
    {
        System.Console.WriteLine(e.ToString());
    }

    try
    {
        conn.Close();
    }
    catch (Exception e)
    {
        System.Console.WriteLine(e.ToString());
    }

    return v;
}
2
  • Try this: if ((myReader[0].ToString().ToLower().Contains(verb.ToLower())) || (myReader[1].ToString().ToLower().Contains(verb.ToLower())) || (myReader[2].ToString().ToLower().Contains(verb.ToLower()))) Commented Oct 14, 2014 at 7:20
  • 2
    why so many try catch???? Commented Oct 14, 2014 at 7:23

3 Answers 3

3

Why not just do the check directly in SQL? Your SQL would be something like:

SELECT  BaseForm, PastForm, PastPartForm
FROM    EnglishVerbs
WHERE   @Verb IN (BaseForm, PastForm, PastPartForm);

Or if you only want partial matches it would be:

SELECT  BaseForm, PastForm, PastPartForm
FROM    EnglishVerbs
WHERE   BaseForm LIKE '%' + @Verb + '%'
OR      PastForm LIKE '%' + @Verb + '%'
OR      PastPartForm LIKE '%' + @Verb + '%';

SQL is better equiped for handling set based data like this, and it avoids the network traffic of retrieving the entire table then the overhead of looping through every row. Given the number of English verbs there are, I would imagine you don't want to return all of them to the application for every search!

Then you can call this using something like:

public static Verb GetVerbs(string verb)
{
    Verb v = new Verb();

    string sql = @"SELECT   BaseForm, PastForm, PastPartForm
                    FROM    EnglishVerbs
                    WHERE   @Verb IN (BaseForm, PastForm, PastPartForm);";

    string connString = "Data Source=.;Initial Catalog=QABase;Integrated Security=True";
    using (var connection = new SqlConnection(connString))
    using (var command = new SqlCommand(sql, connection))
    {
        command.Parameters.Add("@Verb", SqlDbType.VarChar, 50).Value = verb;
        connection.Open();

        using (var reader = command.ExecuteReader())
        if (reader.Read())
        {
            v.BaseTense = Convert.ToString(reader["BaseForm"]);
            v.PastTense = Convert.ToString(reader["PastForm"]);
            v.PastParticiple = Convert.ToString(reader["PastPartForm"]);
        }
    }
    return v;
}

N.B. I have removed all your try/catch blocks because they are not well used! If you must use try/catch, just use one, and catch specific exceptions before catching general exceptions. It is also a pretty good idea to do something meaningful with your exception like logging it if you are going to bother catching it, not just write it to the console.

It is definitely a good idea to exit the method after catching it, if your initial call of conn.Open(); throws an exception, your method happily carries on to still use the connection. This will mean that more exceptions are thrown.

A rough outline might be something like:

try
{
    using (var connection = new SqlConnection(connString))
    using (var command = new SqlCommand(sql, connection))
    {
        command.Parameters.Add("@Verb", SqlDbType.VarChar, 50).Value = verb;
        connection.Open();

        using (var reader = command.ExecuteReader())
        if (reader.Read())
        {
            v.BaseTense = Convert.ToString(reader["BaseForm"]);
            v.PastTense = Convert.ToString(reader["PastForm"]);
            v.PastParticiple = Convert.ToString(reader["PastPartForm"]);
        }
    }
}
catch (SqlException e)
{
    //Log the exception or do something else meaningful
    throw;
}
catch (Exception e)
{
    //Log the exception or do something else meaningful
    throw;
}
Sign up to request clarification or add additional context in comments.

Comments

1

You select all the EnglishVerbs and iterate through them. If you find the verb contained you do the assignment, else you assign nulls. However, you don't break the loop when you find the verb. If the last element in the list doesn't contain your verb, as you can guess, it's assigned null. Change your loop to something like:

while (myReader.Read())
{
    if ( myReader["BaseForm"].ToString().ToLower().Contains(verb.ToLower()) || 
         myReader["PastForm"].ToString().ToLower().Contains(verb.ToLower()) || 
         myReader["PastPartForm"].ToString().ToLower().Contains(verb.ToLower()) )
    {
        v.BaseTense      = Convert.ToString(myReader["BaseForm"]);
        v.PastTense      = Convert.ToString(myReader["PastForm"]);
        v.PastParticiple = Convert.ToString(myReader["PastPartForm"]);

        break;
    }
    else
    {
        v.BaseTense = null;
        v.PastTense = null;
        v.PastParticiple = null;
    }
}        

Also, you don't have to assign null in the else block every time. Just assign it before the loop once. Like:

v.BaseTense = null;
v.PastTense = null;
v.PastParticiple = null;

while (myReader.Read())
{
    if ( myReader["BaseForm"].ToString().ToLower().Contains(verb.ToLower()) || 
         myReader["PastForm"].ToString().ToLower().Contains(verb.ToLower()) || 
         myReader["PastPartForm"].ToString().ToLower().Contains(verb.ToLower()) )
    {
        v.BaseTense      = Convert.ToString(myReader["BaseForm"]);
        v.PastTense      = Convert.ToString(myReader["PastForm"]);
        v.PastParticiple = Convert.ToString(myReader["PastPartForm"]);

        break;
    }
} 

BUT this isn't the best thing to do. I answered according to your programming style. However, it's best to make the filtering in database side by changing your query as in GarethD's answer. By that, you get less elements from database, this is a huge gain in performance terms. Passing data between project levels (client-application-database levels) should be your biggest concern.

1 Comment

Very informative, Thank you so much and it worked! :)
1

There are several improvements that you can make to the posted code.

First of all, it would make sense to let the database do the work of finding the row(s):

SqlCommand myCommand = new SqlCommand("select * from EnglishVerbs where BaseForm = @VerbToFind or PastForm = @VerbToFind or PastPartForm = @VerbToFind", conn);
myCommand.Parameters.AddWithValue("VerbToFind", verb.ToLower());

With this you are telling the database to find rows where at least one of the columns contains the verb you are looing for. Depending on the collation of the columns you might need to handle case sensitivity. Assuming that you have the verbs stored as lower case in the database, the above should work. Note that more than one row may be found.

You should also perhaps change you error handling, and also you should wrap you IDisposable object in using clauses. This will mean that once you are done with the objects they will be automatically cleaned up even if an exception occurred:

try 
{
    using(SqlConnection conn = new SqlConnection("Data Source=.;Initial Catalog=QABase;Integrated Security=True")
    {
        conn.open();
        using(SqlCommand myCommand = new SqlCommand("select * from EnglishVerbs where BaseForm = @VerbToFind or PastForm = @VerbToFind or PastPartForm = @VerbToFind", conn))
        {
            myCommand.Parameters.AddWithValue("VerbToFind", verb.ToLower());
            using(myReader = myCommand.ExecuteReader())
            {
                 while (myReader.Read())
                 {
                     v = new Verb();
                     v.BaseTense = Convert.ToString(myReader["BaseForm"]);
                     v.PastTense = Convert.ToString(myReader["PastForm"]);
                     v.PastParticiple = Convert.ToString(myReader["PastPartForm"]);
                     verbsToReturn.Add(v);
                 }
            }
        }
    }
}
catch(Exception e)
{
    System.Console.WriteLine(e.ToString());
    throw; //Rethrow exception to let caller know that something went wrong
}

The whole thing might then look something like this:

public static IEnumerable<Verb> GetVerbs(string verb)
{
    if (verb == null)
        throw new ArgumentException("verb cannot be null");
    List<Verb> verbsToReturn = new List<Verb>();
    Verb v = null;

    try 
    {
        using(SqlConnection conn = new SqlConnection("Data Source=.;Initial Catalog=QABase;Integrated Security=True")
        {
            conn.open();
            using(SqlCommand myCommand = new SqlCommand("select * from EnglishVerbs where BaseForm = @VerbToFind or PastForm = @VerbToFind or PastPartForm = @VerbToFind", conn))
            {
                myCommand.Parameters.AddWithValue("VerbToFind", verb.ToLower());
                using(myReader = myCommand.ExecuteReader())
                {
                     while (myReader.Read())
                     {
                         v = new Verb();
                         v.BaseTense = Convert.ToString(myReader["BaseForm"]);
                         v.PastTense = Convert.ToString(myReader["PastForm"]);
                         v.PastParticiple = Convert.ToString(myReader["PastPartForm"]);
                         verbsToReturn.Add(v);
                     }
                }
            }
        }
    }
    catch(Exception e)
    {
        System.Console.WriteLine(e.ToString());
        throw; //Rethrow exception to let caller know that something went wrong
    }

    return verbsToReturn;
}

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.