Finding exceptions and optimisations in weather forecast service

Question

When creating a new Blazor project, there is a page called FetchData which gives an example of a razor page using a service to pull data to a page.

I set myself a challenge to improve on the service, so that instead of a random label being assigned to a temperature for a forecast, it separates the label categories evenly across the range of temperatures.

Here is the original code for the service:

namespace MyBlazorApp.Data
{
    public class WeatherForecastService
    {
        private static readonly string[] Summaries = new[]
        {
            "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
        };

        public Task<WeatherForecast[]> GetForecastAsync(DateTime startDate)
        {
            return Task.FromResult(Enumerable.Range(1, 5).Select(index => new WeatherForecast
            {
                Date = startDate.AddDays(index),
                TemperatureC = Random.Shared.Next(-20, 55),
                Summary = Summaries[Random.Shared.Next(Summaries.Length)]
            }).ToArray());
        }
    }
}

And here is my new code:

namespace MyBlazorApp.Data
{
    public class WeatherForecastService
    {
        private static readonly string[] Summaries = new[]
        {
            "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
        };

        public Task<WeatherForecast[]> GetForecastAsync(DateTime startDate)
        {
            var minTemp = -10;
            var maxTemp = 40;

            return Task.FromResult(Enumerable.Range(0, 14).Select(index => getWeatherForecast(startDate, minTemp, maxTemp, index)).ToArray());
        }

        public WeatherForecast getWeatherForecast(DateTime startDate, int minTemp, int maxTemp, int index)
        {
            maxTemp += (maxTemp - minTemp) % Summaries.Length;

            var rng = Random.Shared.Next(minTemp, maxTemp);
            var categorySize = (maxTemp - minTemp) / (Summaries.Length);
            var pointer = (rng + (0 - minTemp)) / categorySize;

            if (pointer >= Summaries.Length)
            {
                pointer = Summaries.Length - 1;
            }


            return new WeatherForecast
            {
                Date = startDate.AddDays(index),
                TemperatureC = rng,
                Summary = Summaries[pointer]
            };

        }
    }
}

The code seems to work fine, but I feel that the error handling as well as the code optimization, readability and simplicity can be improved. Any tips on how I can look to find these improvements myself would also be extremely helpful. I changed some of the numbers to feel more appropriate to the context, but ideally the solution should work no matter what variables are set.

In summary, I am trying to find a solution to the problem in the absolute best way possible, as a way of giving an example of what perfect code looks like.

Peter Csala · Accepted Answer · 2022-09-09 15:13:12Z

The `GetForecastAsync` method

Since you always call it with DateTime.Now from FetchData.razor you should consider to remove startDate as a parameter
Nothing indicates that this method should be async, so you could convert it to back to sync

You could replace the OnInitializedAsync override with the following code:

protected override void OnInitialized()
  => forecasts = ForecastService.GetForecastForTwoWeeks();

The GetForecastAsync could be replaced with this:

public WeatherForecast[] GetForecastForTwoWeeks()
    => Enumerable.Range(0, 14)
        .Select(daysSinceStart => GetWeatherForecast(DateTime.Now.AddDays(daysSinceStart)))
        .ToArray();

Since 14 is hard-coded inside the method I would suggest to add this constraint/restriction to the method name (GetForecastForTwoWeeks)
Since minTemp and maxTemp are constants and used only inside the GetWeatherForecast I've moved them from here
The startDate and index are always used together inside the GetWeatherForecast to calculate the new Date
- So, rather than passing them separately you could pass the calculated date

The `getWeatherForecast` method

I would suggest to use C# standard naming guideline and name it with Pascal Case (GetWeatherForecast)
I would also suggest to make it private if it is used only by the GetForecastForTwoWeeks method

You could rewrite the getWeatherForecast like this

const int MinTemperature = -10, MaxTemperature = 40;
static readonly int AdjustedMaxTemperature = MaxTemperature + (MaxTemperature - MinTemperature) % Summaries.Length;
static readonly int CategorySize = (AdjustedMaxTemperature - MinTemperature) / Summaries.Length;
private static WeatherForecast GetWeatherForecast(DateTime date)
{
    var temperature = Random.Shared.Next(MinTemperature, AdjustedMaxTemperature);
    var summaryIndex = (temperature - MinTemperature) / CategorySize;

    return new ()
    {
        Date = date,
        TemperatureC = temperature,
        Summary = Summaries[summaryIndex]
    };
}

Temp can abbreviate multiple terms (like temporarily, temperature, etc.) so, it might make sense to avoid abbreviating here
I would advise you to avoid overwriting method's parameter value (maxTemp += ...)
The adjusted maximum temperature and the category size are always the same
- So, they could be calculated only once
BTW the AdjustedMaxTemperature is the same as the MaxTemperature since (MaxTemperature - MinTemperature) % Summaries.Length is 0
+ (0 - minTemp) can be simplified to -minTemp
This pointer >= Summaries.Length condition check is not needed, it will never be true
rng does not mean anything, whereas temperature is more meaningful/expressive IMHO

The most concise version I can think of now (which is still readable):

private const int MinTemperature = -10, MaxTemperature = 40;
private static readonly int AdjustedMaxTemperature = MaxTemperature + (MaxTemperature - MinTemperature) % Summaries.Length;
private static readonly int CategorySize = (AdjustedMaxTemperature - MinTemperature) / Summaries.Length;

public WeatherForecast[] GetForecastForTwoWeeks()
    => (from daysSinceStart in Enumerable.Range(0, 14)
        let temperature = Random.Shared.Next(MinTemperature, AdjustedMaxTemperature)
        let summaryIndex = (temperature - MinTemperature) / CategorySize
        select new WeatherForecast
        {
            Date = DateTime.Now.AddDays(daysSinceStart),
            TemperatureC = temperature,
            Summary = Summaries[summaryIndex]
        })
        .ToArray();

Brilliant, thanks for the detailed answer. I will try to apply this knowledge in my future work! — AT123
– AT123, Commented Sep 10, 2022 at 17:07
For the removal of pointer >= Summaries.Length - If the temperature values were adjusted so that they weren't fully divisible by the Summaries list length, a remainder would be created from the calculations which would create an index out of range exception without the code above. But for the initial values it would be fine. I should have specified this in the original question. — AT123
– AT123, Commented Sep 12, 2022 at 15:49
@AT123 I've provided suggestions based on your provided code. Any requirement can invalidate my suggest so you should evaluate them whenever you receive a new. — Peter Csala
– Peter Csala, Commented Sep 12, 2022 at 16:31

Stack Exchange Network

Finding exceptions and optimisations in weather forecast service

1 Answer 1

The `GetForecastAsync` method

The `getWeatherForecast` method

You must log in to answer this question.

Hot Network Questions

Finding exceptions and optimisations in weather forecast service

1 Answer 1

The GetForecastAsync method

The getWeatherForecast method

You must log in to answer this question.

Related

Hot Network Questions

The `GetForecastAsync` method

The `getWeatherForecast` method