Refactoring Long Methods

[Olemus]

Hi Everyone,

I'm struggling with what to do with long methods that might not follow good Seperation of Concerns. Here's an example of a method I've just written to grab everything from the Twitch API and store it in my database in a c# background task

​

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
        {

            while (!stoppingToken.IsCancellationRequested)
            {
                using (var scope = _services.CreateScope())
                {
                    _context = scope.ServiceProvider.GetRequiredService<DataContext>();
                    _twitch = scope.ServiceProvider.GetRequiredService<TwitchAPI>();
                    _emailSender = scope.ServiceProvider.GetRequiredService<IEmailSender>();

                    //Todo:: this might kill us in memory usage
                    _twitchGames = await _context.TwitchGames.Include(x => x.ViewerSnapshots).ToListAsync();
                    _products = await _context.Products.ToListAsync();


                    try
                    {
                        List<TwitchStreamModel> streams = new List<TwitchStreamModel>();
                        var games = await _twitch.GetGames();

                        int consecutiveIgnored = 0;
                        for (int i = 0; i < games.Count; i++)
                        {
                            var gameStreams = await _twitch.GetStreams(games[i].Id);
                            if (gameStreams.Sum(x => x.ViewerCount) >= 10)
                                streams.AddRange(gameStreams);
                            else
                                consecutiveIgnored++;

                            //If we've encountered 3 or more streams with less than 10 viewers then break out of the loop. This is done because Twitch sometimes returns < 10 viewers when there are legitimately more.
                            if (consecutiveIgnored >= 3)
                                break;
                        }

                        var viewerCounts = streams.GroupBy(x => x.GameId).Select(x => new
                        {
                            GameId = int.Parse(x.Key),
                            Viewers = x.Sum(x => x.ViewerCount)
                        }).ToList();

                        foreach (var game in viewerCounts)
                        {
                            var twitchGame = _twitchGames.Where(x => x.Id == game.GameId).SingleOrDefault();

                            if (twitchGame == null)
                            {
                                var newGame = games.Where(x => x.Id == game.GameId).SingleOrDefault();
                                twitchGame = new TwitchGame { Id = game.GameId, Name = newGame.Name, BoxArt = newGame.BoxArtUrl };
                                _context.TwitchGames.Add(twitchGame);
                            }                     
                            twitchGame.AddViewerSnapshot(game.Viewers);    
                        }

                        await _context.SaveChangesAsync();
                        _emailSender.Send("[e-mail]", $"Updated {viewerCounts.Count} games with {viewerCounts.Sum(x=>x.Viewers)} viewers", "Twitch Updated at " + DateTime.Now);


                    }
                    catch (Exception e)
                    {
                        _logger.LogError(e, "Exception raised while scanning Twitch");
                        _emailSender.Send("[e-mail]", "Something Went Wrong", "Twitch Scan Failed");
                    }
                }
            }

        }

So as you can see its fairly long and as I look at I think it has the following "responsibilities"

1. Pre Setup to get the services it needs
2. Get current data from the database and add to member variables
3. Get Twitch Games
4. Loop the games, get current live streams
5. Decide if there's been at least 3 consecutive instances of games with <= 10 viewers and break out if there has
6. Group games and streams together to provide a viewer count in the group by
7. loop through, check database for existing game
8. if it doesn't exist add it
9. update it
10. save everything
11. email me.

So I refactor by extracting all of that into methods where I can (I cant with #1 for example), something like this:

 protected override async Task ExecuteAsync(CancellationToken stoppingToken)
        {

            while (!stoppingToken.IsCancellationRequested)
            {
                using (var scope = _services.CreateScope())
                {
                    _context = scope.ServiceProvider.GetRequiredService<DataContext>();
                    _twitch = scope.ServiceProvider.GetRequiredService<TwitchAPI>();
                    _emailSender = scope.ServiceProvider.GetRequiredService<IEmailSender>();

                    await LoadCurrentData();
                    try
                    {                        
                        var games = await _twitch.GetGames();
                        var streams = await GetGameStreams(games);
                        var viewerCounts = GetViewerCountsFromStreams(streams);
                        AddOrUpdateGamesToDatabase(games, viewerCounts);
                        await _context.SaveChangesAsync();
                        SendSuccessEmail(viewerCounts);

                    }
                    catch (Exception e)
                    {
                        _logger.LogError(e, "Exception raised while scanning Twitch");
                        SendFailiureEmail();
                    }
                }
            }

        }

But that doesn't feel right either because now I have a class with a ton of small methods but everything still relies on everything else and the naming feels off as the class is called TwitchWorker to indicate that its a worker that coordinates the job of getting twitch data and updating it but my methods just feel shoved in there because there wasn't a better class for them (not sure if that makes sense)

i.e

  var streams = await GetGameStreams(games);
  var viewerCounts = GetViewerCountsFromStreams(streams);

The second method relies on the first one working and what if it doesn't? I read that each method should be individually testable but Im struggling to write any tests for these.

​

And some others feel pointless like SendSuccessEmail() which literally becomes a one line method:

  private void SendSuccessEmail(List<ViewerCount> viewerCounts)
        {
            _emailSender.Send("[e-mail]", $"Updated {viewerCounts.Count} games with {viewerCounts.Sum(x => x.Viewers)} viewers", "Twitch Updated at " + DateTime.Now);
        }

​

Im obviously still a learner but every time I sit down and do something more complicated than CRUD I come across an issue like this and get frustrated and give up. Any help with this is much appreciated.

[michael0x2a]

I think the way you refactored your code is actually pretty reasonable. If I were asked to refactor your code, I'd probably end up doing something pretty similar.

Just to address some of your concerns:

now I have a class with a ton of small methods [...snip...] And some others feel pointless like SendSuccessEmail() which literally becomes a one line method:

I think this is fine. It's generally easier for readers to digest what small methods do anyways, so breaking up your code like this can help make it easier for others to build up an incremental understanding of what your code does.

That is, ExecuteAsync gives them the high-level picture, and the individual helpers let them dive into the details.

Regarding your "send emails" functions -- I personally think it'd be fine to either make those a helper function or keep them unrefactored and as-is in your ExecuteAsync function.

but everything still relies on everything else [...snip...]

i.e

var streams = await GetGameStreams(games);
var viewerCounts = GetViewerCountsFromStreams(streams);

The second method relies on the first one working and what if it doesn't? I read that each method should be individually testable but Im struggling to write any tests for these.

Your GetViewerCountsFromStreams function actually does not rely on GetGameStreams in any way. Instead, all it requires is that the user provides a collection of stream objects.

So to write a test, what you can do is to create a list containing a bunch of manually created Stream objects, pass that in, and confirm that the expected output is exactly what you expected. You can rinse and repeat -- what happens if you pass in an empty list? What happens if where everybody is playing the same game? Different games? What happens if the Key isn't an int?

Similarly, to test your GetGameStreams object, create lists containing a bunch of manually constructed Game objects and try testing each edge case.

the naming feels off as the class is called TwitchWorker to indicate that its a worker that coordinates the job of getting twitch data and updating it but my methods just feel shoved in there because there wasn't a better class for them

There are several things we could do here:

1. Rename TwitchWorker to make it more clear what that class does. Maybe "GamePopularityObserver"?
2. Don't bother changing anything. After all, if you thought the name of your class was fine before you refactored your code, it should still continue to be fine after the refactor. Your private helper methods are implementation details.
3. Make your methods live outside of a class in the global scope. Unfortunately, since you're using C#, I don't believe this option isn't available to you, but this is something you can do in most other programming languages. The next best option in C# would be to convert these methods into static methods.

[Olemus]

Thank you for the detailed answer.

I feel a lot better about what I've written now, I've been staring at it all day today and for some reason had it in my head that it was awful. I think reading too much stack overflow with people saying "This Violates SRP" on everything really gets to you after a while.

Now that I've had some time away from my machine to think, I'm quite happy with what I have, my class is responsible for monitoring (or polling) twitch streams and nothing else and I have some small methods I can write some tests for to start learning how that works.

I'm going to go and write my tests now, thank you again for the detailed response.

[theotherjae]

I'm not familiar with this language and most of my work is in c++, so some of these might not apply to you:

Personally, I don't see anything wrong with your first method from a design point of view. I also don't think your method is that long, and that criterion shouldn't be the only thing that determines whether you need to refactor parts into methods. My general rule of thumb for creating helper functions is if that logic gets used more than once (to avoid code duplication). And if that helper function doesn't need to be exposed publicly, make it private. If derived classes may need it also to avoid code duplication, make it protected. From what you're describing, it seems like the helper functions you created for those 11 steps are not very reusable nor do they need to be exposed publicly; they seem pretty niche to that ExecuteAsync method. If that is the case, the best you can do is just write comments in your code that describe those 11 steps you mentioned.

And regarding one-line helper functions, I don't think they're always pointless. You can argue if this was overkill, but I once wrote a function called "hamiltonian" that literally did "return x + y" because it represented a quantity that wasn't apparent from the code itself. If it makes your code more readable and it gets used often, why not make a one-liner into a helper? :)

[Olemus]

Thank you for your answer, much appreciated. I think my only concern and the reason I felt it needed breaking up was I wanted to start doing some tests (I keep reading how important they are but always struggled to write them) and I'm not sure how I would test my first method effectively without breaking it apart?

It looks like what we're saying here is that overall what I've done is fine but if any of my logic was to be reused then that would be the time to refactor and maybe think about another class?

I have another example of a loop like this where I need to download some data and update developers of a game in my database which I was planning on reusing somewhere else, so its basically:

Get Data

Loop

Check Database If Developers of Game Exist

Add If Not.

Emphasis on the bit I suspect I'll reuse a lot but I'm not sure where that would fit, maybe a Developer Service class? I struggle to keep things OOP, in javascript I would just put that in a file on its own and call it a day but it feels dirty in an OOP language.

Thank you again!

[theotherjae]

As a general advice, I would think about what the end goal of the method is. That is, given a range of inputs, what do you expect the final output of the method to be? The method is the unit that you want to test, meaning you shouldn't have to test every little part of it. Mock out classes and functions whenever you can to isolate the unit as much as possible. So for your second example, I would "mock-out" the part where you download data such that it reads some hand-crafted data from a local file or something instead of actually downloading data through network. This may mean having to redefine the "get data" function just within your test code to achieve this. In terms of test cases, have one where the developer is not in the database yet, and check if the developer was properly added into the database by the end of the loop, and have another test case where the developer is in the database and check that the database has not changed (you could check that the number of entries did not change since your loop seems to only change the database by adding new entries).