1
\$\begingroup\$
public bool CheckPlace(string State, string City)
{
    TestDataContext data = new TestDataContext();
    bool IsKochi = new bool();
    if (!string.IsNullOrEmpty(State) && !string.IsNullOrEmpty(City))
    {
        var Place = (from a in data.Places where a.State == State || a.City == City select a.Place).ToList();
        if (Place != null)
        {
            foreach (var placeName in Place)
            {
                if (placeName.ToString().ToLower() == "kochi")
                {
                    IsKochi = true;
                    break;
                }
                else
                {
                    IsKochi = false;
                }
            }
        }
    }

    return IsKochi;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ new bool() is a very weird way to write false. \$\endgroup\$
    – svick
    Oct 29, 2012 at 22:51
  • 1
    \$\begingroup\$ The title of your post should be the function/purpose of your code. \$\endgroup\$
    – SirPython
    Aug 4, 2015 at 23:33

3 Answers 3

1
\$\begingroup\$
public bool CheckPlace(string State, string City)
{
    if (string.IsNullOrEmpty(State) || string.IsNullOrEmpty(City))
    {
        return false;
    }

    using (var data = new TestDataContext())
    {
        var ret = data.Places.Where(a => (a.State == State || a.City == City)).Select(a => a.Place).ToList().Any(x => x.ToString().ToLower() == "kochi");

        return ret;
    }
}

If the Place have a string property what can be use at the "is kochi" check then use that in the Any() and leave out the .ToList().

Instantiating the TestDataContext() in this method is bad (can not test the method) please inject it as method or class constructor parameter (you can inject a lambda for instantiating it also).

(Syntax errors maybe exists in my code [powered by Notepad].)

\$\endgroup\$
1
  • \$\begingroup\$ I like the short-circuiting in your first if statement. Concise and liable to save a lot of cycles depending on how complex the TestDataContext is. \$\endgroup\$ Oct 29, 2012 at 16:19
1
\$\begingroup\$

First of all, coding standards generally suggest that all local variables, including parameters, be camelCase.

Here is a suggestion...

public bool CheckPlace(string state, string city)
{
    TestDataContext data = new TestDataContext();
    bool isKochi = false;
    if (!string.IsNullOrEmpty(state) && !string.IsNullOrEmpty(city))
    {
        isKochi = data.Places.Any(a => (a.State == state || a.City == city) && a.Place.ToLower() == "kochi");
    }

    return isKochi;
}

.Any returns true if there are any values in the collection based on the filter condition passed into the .Any clause

Second, What is the type of Place. Is it a distinct list that can be turned into an enum? That could make it so you are not checking for a specific string.

\$\endgroup\$
0
\$\begingroup\$
  • Make data a parameter. This allows you to test the method without relying on a specific Context instance. You can wrap another method that passes in the default Context, but this way you are not tied to the specific implementation.

  • Create a method that does the query on the data. This way you can have a simple method call that clear states what it is doing, instead of forcing someone to interpret the code. It also makes a long line of code shorter because it is on it's own and not in nested in an if block.

  • bool defaults to false. You don't have to keep setting IsKochi to false every time you don't find a match.

  • The convention is that names of method variables are in lowerCamelCase, not UpperCamelCase.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Just an FYI...This is called camelCase (first character lower case). This is called PascalCase (first character upper case). \$\endgroup\$
    – Gene S
    Oct 29, 2012 at 15:12

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