7
\$\begingroup\$

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();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Aug 17 '16 at 15:38
  • \$\begingroup\$ Maybe you can use a struct with string fields instead of a string directly and use the C# 6.0 null check operator .? \$\endgroup\$ – null Aug 17 '16 at 16:20
  • \$\begingroup\$ @null even if I wanted to, the code would just make a problem in our production environment since prod .net version is 4.5 which I believe C# 6.0 new features and improvement is in 4.6. Just so you know I already did use that and made me revert it after deploying our changes in production. \$\endgroup\$ – rpmansion Aug 17 '16 at 16:33
  • \$\begingroup\$ The .NET version does not equal the C# version in this case. You can use C#6 features and target older versions of the framework. \$\endgroup\$ – Jesse C. Slicer Aug 17 '16 at 17:34
  • \$\begingroup\$ @JesseC.Slicer I stand corrected, thanks for the correction. =) \$\endgroup\$ – rpmansion Aug 17 '16 at 20:14
3
\$\begingroup\$

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))
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ in reality those two test data is our institution's names, so we do not want that to be included in the list that is why I used two where. also i tried using || operator but this will not work inside where and I was force to do the two where \$\endgroup\$ – rpmansion Aug 17 '16 at 16:42
  • 1
    \$\begingroup\$ If you want that filter then it's a && operator you should have used, not ||: .Where(institution => !institution.Name.Contains("institutiontest1") && !institution.Name.Contains("institutiontest2")). \$\endgroup\$ – Mathieu Guindon Aug 17 '16 at 16:44

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.