Getting statistics in rest api endpoint

Question

We have following endpoint on our api that should grab some events from db, aggregate them and output as a json. But I am having hard time reading this function and hope you could help me out with some ideas how to refactor this to something more reasonable. Or maybe its fine and I just need to read harder?

[HttpGet]
public AmazonDTO GetCRSTimeStatuses(string countryId, string regionId, string storeId, string dateFrom, string dateTo)
{
    List<CRSTimeStatus> res = new List<CRSTimeStatus>();

    try
    {
        DateTime _dateFrom;
        DateTime.TryParse(dateFrom, out _dateFrom);

        _dateFrom = _dateFrom.Date;

        DateTime _dateTo;
        DateTime.TryParse(dateTo, out _dateTo);

        int _countryId;
        int.TryParse(countryId, out _countryId);

        int _regionId;
        int.TryParse(regionId, out _regionId);

        int _storeId;
        int.TryParse(storeId, out _storeId);

        PageDetailType _pageDetailType = ModelHelper.GetActualPageType(_countryId, _regionId, _storeId, tillId: 0);

        var eventValue = _cache.Get<List<Event>>("eventsChache");

        var events = new List<Event>();
        var countries = new List<Country>();

        // Read base relation data from DB.
        countries = _countryRepository.GetAll().ToList();
        _regionRepository.GetAll().ToList();
        _storeRepository.GetAll().ToList();
        _askoTillRepository.GetAll().ToList();

        if (_hostingEnvironment.IsDevelopment())
        {
            EventCache evc = new EventCache(_eventRepository, _cache);
            evc.SetEventsCache(_dateFrom, _dateTo);
        }

        events = _cache.Get<List<Event>>("eventsChache");

        Dictionary<int, List<Event>> eventsForPos = new Dictionary<int, List<Event>>();

        events = events == null ? new List<Event>() : events;

        var forEvets = events.FindAll(x => x.DateFrom >= _dateFrom && (x.DateTo <= _dateTo || !x.DateTo.HasValue));

        foreach (Event e in forEvets)
        {
            if (eventsForPos.ContainsKey(e.PosTillId))
            {
                eventsForPos[e.PosTillId].Add(e);
            }
            else
            {
                eventsForPos[e.PosTillId] = new List<Event>() { e };
            }
        }

        foreach (var item in countries)
        {
            var tills = item.Regions.SelectMany(t => t.Stores.Where(e => e.PosTills != null).SelectMany(r => r.PosTills));

            foreach (var till in tills)
            {
                till.Events = eventsForPos.ContainsKey(till.Id) ? eventsForPos[till.Id] : null;
            }

        }

        switch ((PageDetailType)_pageDetailType)
        {
            case PageDetailType.CountriesDetail:

                break;
            case PageDetailType.CountryDetail:
                res = ModelHelper.GetGraphStatusesCountry(countries, _countryId);
                break;
            case PageDetailType.RegionDetail:
                res = ModelHelper.GetGraphStatusesRegion(countries, _regionId);
                break;
            case PageDetailType.StoreDetail:
                res = ModelHelper.GetGraphStatusesStore(countries, _storeId);
                break;
            case PageDetailType.TillDetail:

                break;
            default:
                break;
        }
    }
    catch (Exception ex)
    {
        _logger.LogError(ex.ToString());
        return AmazonDTO.Error(((int)HttpException.InternetServerError).ToString(), ex.ToString());
    }

    return AmazonDTO.Data(res);
}

My initial suggestion is to remove data parsing at the start and move it elsewhere. The code also looks like it is trying to do three things at once based on incoming parameters, maybe split it into three endpoints or use some form of inheritance would help here.

Any ideas are welcomed!

Answer 1

Not using return values of TryParse

The method receives several parameters as string, and parses them to other types such as DateTime or int. It's surprising to see calls to TryParse, and using the value in the out parameters without checking the returned value.

The code basically assumes that the string values are of the correct format. But if that assumption is always true, that fact should be at least documented, but it would be even better to pass the real types instead of string values. If the assumption is not guaranteed to be true, then the program may crash.

Repetitive code

The parameter parsing code (string values to DateTime or int) are repetitive, and take at least 2 lines per value. It would be better to add some helper functions to simplify this, so that you could write like this:

DateTime _dateFrom = parseDateTime(dateFrom).Date;
DateTime _dateTo = parseDateTime(dateTo);

Pointless initializers

Here, it's pointless to initialize countries with an empty list when it's going to be reassigned right after:

var countries = new List<Country>();

countries = _countryRepository.GetAll().ToList();

The same goes for events.

Single responsibility principle

This method is doing too much, that's why it's so long. It would be better to split it up to smaller logical units.

Statements with side effects?

What are these for?

_regionRepository.GetAll().ToList();
_storeRepository.GetAll().ToList();
_askoTillRepository.GetAll().ToList();

It looks like loading objects, converting them to List, and doing nothing with the values. What's going on here? At the minimum this should be documented (with comments).

Group related statements together

It's hard to read when related statements are not grouped together, for example:

var events = new List<Event>();

// more than 10 lines of code ...

events = _cache.Get<List<Event>>("eventsChache");

// some code ...

events = events == null ? new List<Event>() : events;

It's hard to follow how the events variable is used when there are unrelated statements in between.

Local variable or field?

The variable names use an inconsistent mix of styles, which makes the code harder to read. For example, some local variables have a _ prefix, others do not. Some variables defined outside the method also have _ prefix. Those should probably be properties instead.

Use var more

You used var at some places, but not at others. For example Dictionary<int, List<Event>> eventsForPos = new Dictionary<int, List<Event>>(); is really tedious and a good candidate for using var.

Bug?

This condition looks like a bug:

x.DateTo <= _dateTo || !x.DateTo.HasValue

If x.DateTo doesn't have a value, won't x.DateTo <= _dateTo crash? It should be safer if you flip that condition:

!x.DateTo.HasValue || x.DateTo <= _dateTo

Simpler foreach

This can be written simpler:

foreach (Event e in forEvets)
{
    if (eventsForPos.ContainsKey(e.PosTillId))
    {
        eventsForPos[e.PosTillId].Add(e);
    }
    else
    {
        eventsForPos[e.PosTillId] = new List<Event>() { e };
    }
}

Like this:

foreach (Event e in forEvets)
{
    if (!eventsForPos.ContainsKey(e.PosTillId))
    {
        eventsForPos[e.PosTillId] = new List<Event>();
    }
    eventsForPos[e.PosTillId].Add(e);
}

And btw there's a typo in forEvets.

Group case statements with same body together

Instead of this:

switch ((PageDetailType)_pageDetailType)
{
    case PageDetailType.CountriesDetail:
        break;
    case PageDetailType.CountryDetail:
        res = ModelHelper.GetGraphStatusesCountry(countries, _countryId);
        break;
    case PageDetailType.RegionDetail:
        res = ModelHelper.GetGraphStatusesRegion(countries, _regionId);
        break;
    case PageDetailType.StoreDetail:
        res = ModelHelper.GetGraphStatusesStore(countries, _storeId);
        break;
    case PageDetailType.TillDetail:
        break;
    default:
        break;
}

It would be better like this:

switch ((PageDetailType)_pageDetailType)
{
    case PageDetailType.CountryDetail:
        res = ModelHelper.GetGraphStatusesCountry(countries, _countryId);
        break;
    case PageDetailType.RegionDetail:
        res = ModelHelper.GetGraphStatusesRegion(countries, _regionId);
        break;
    case PageDetailType.StoreDetail:
        res = ModelHelper.GetGraphStatusesStore(countries, _storeId);
        break;
    case PageDetailType.CountriesDetail:
    case PageDetailType.TillDetail:
    default:
        break;
}