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;
}
3 Answers
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].)
-
\$\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
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.
Make
data
a parameter. This allows you to test the method without relying on a specificContext
instance. You can wrap another method that passes in the defaultContext
, 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 settingIsKochi
to false every time you don't find a match.The convention is that names of method variables are in lowerCamelCase, not UpperCamelCase.
-
1\$\begingroup\$ Just an FYI...This is called
camelCase
(first character lower case). This is calledPascalCase
(first character upper case). \$\endgroup\$– Gene SOct 29, 2012 at 15:12
new bool()
is a very weird way to writefalse
. \$\endgroup\$