Retrieving all institutions in a specified country

Question

I'm checking each variable to prevent null exception, but still wondering can this be improve in much simpler and cleaner if you will be writing the same code? Please see the below snippet:

[AjaxOnly]
public ActionResult GetInstitutions(string parent)
{
    if (string.IsNullOrEmpty(parent))
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

    var country = _countryRespository.GetByCountryCode(parent);

    if (country == null)
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

    if (country.Institutions == null || country.Institutions.Count <= 0)
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

    var institutions = country.Institutions
        .OrderBy(i => i.Name)
        .Where(i => !i.Name.Contains("institutiontest1"))
        .Where(i => !i.Name.Contains("institutiontest2"))
        .Select(i => new SelectListItem { Text = i.Name, Value = i.Id.ToString() });

    return Json(institutions, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

CountryRepository.GetByCountryCode method implementation

public Country GetByCountryCode(string countryCode)
{
    return Session.Query<Country>()
                    .Where(c => c.Iso == countryCode)
                    .FetchMany(c => c.Institutions)
                    .SingleOrDefault();
}

Answer 1

Country.Institutions should never return a null collection, and if it ever does, I'd prefer having a NullReferenceException thrown to tell me something is broken with my API. The null-check is therefore redundant here:

if (country.Institutions == null || country.Institutions.Count <= 0)
    return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

As for country.Institutions.Count <= 0, it makes me raise an eyebrow. How would .Count ever return -12? Clearly the intent here is to say "if there aren't any institutions in that country", which is better conveyed like this:

if (!country.Institutions.Any())
    return Json(string.Empty, ...);

Now, you have two separate conditions that both return the same "empty value" - combine them - and because a scope is always better when it's explicit, add those curly braces, too:

if (country == null || !country.Institutions.Any())
{
    return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

This leaves another "empty value" returned when parent is null... which is annoying.

Why can't GetByCountryCode handle null input? (Iso can't legally be empty, right?)

public Country GetByCountryCode(string countryCode)
{
    if (string.IsNullOrEmpty(countryCode)) { return null; }

    return Session.Query<Country>()
                  .Where(country => country.Iso == countryCode)
                  .FetchMany(country => country.Institutions)
                  .SingleOrDefault();
}

That way country will be null given a null input, and you don't need to repeat yourself anymore:

var country = _countryRespository.GetByCountryCode(parent);
if (country == null || !country.Institutions.Any())
{
    return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

And then select your data:

var institutions = country.Institutions
    .OrderBy(i => i.Name)
    .Where(i => !i.Name.Contains("institutiontest1"))
    .Where(i => !i.Name.Contains("institutiontest2"))
    .Select(i => new SelectListItem { Text = i.Name, Value = i.Id.ToString() });

hmm.. what's with these .Where statements? Filtering out test data? So if you go and add some institutiontest3 you'll need to add yet another .Where call?

Don't do that. Make yourself a separate, dev/test database, and have another database for your production data - and use proper configuration to determine which connection string to use to initialize your Session.

---

TL;DR:

[AjaxOnly]
public ActionResult GetInstitutions(string parent)
{
    var country = _countryRespository.GetByCountryCode(parent);
    if (country == null || !country.Institutions.Any())
    {
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
    }

    var institutions = country.Institutions
                              .OrderBy(institution => institution.Name)
                              .Select(institution => new SelectListItem
                              { 
                                  Text = institution.Name,
                                  Value = institution.Id.ToString()
                              });
    return Json(institutions, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

---

If the filtered-out values are meant to be specific values, consider making them explicit:

var ourInstitutions = new[]
{
    "foo",
    "bar"
};

And then you should be able to do this:

.Where(institution => !ourInstitutions.Contains(institution.Name))

Which makes the intent much clearer. If your ORM complains (looks like you're using NHibernate, which I'm not very familiar with), then make that filtering handled by LINQ-to-Objects instead:

.ToList()
.Where(institution => !ourInstitutions.Contains(institution.Name))

If it's just a few values to iterate, there shouldn't be much of a penalty there, and the readability+maintainability gain is considerable, assuming you're looking for specific values. Otherwise, this would be equivalent:

.Where(institution => !ourInstitutions.Any(ours => institution.Name.Contains(ours.Name))