1
\$\begingroup\$

I inherited a very long and messy C# function with a task to make it more readable and Clean Architecture Pattern friendly.

The primary goal of a function is to send an API request to an endpoint and stream a collection of sport events which took place on a specific date. Response need to be saved to local File Storage. For this approach I abstracted two services within Core (Application) Layer and implemented them in an Infrastructue Layer:

  • webScraperService (for api calls)
  • fileStorageService (for saving scraped data)

I think that this should be good enough.

Regarding the function itself, I have identified key regions (I marked them and put a description) and for now the code itself is working, but it is still far away from being readable and clean (note that I am junior and this is my first "official" task). I excluded validations and exceptions to make the pasted code shorter.

My first thoughts are that this is a typical ETL process so I am thinking in a direction similar to SQL Server Integration Services (SSIS) with Control and Data flow (pipeline pattern) implementation, but it would take to much time and it might be an overkill for this scenario.

From the point of view of a more experienced programmer, do you have some suggestions to share about a good approach to better organize this code?

Thank you in advance.

        public async Task ExecuteAsync()
    {
        #region #1

        // Generating a List of Dates for which we need to send an API Request.

        var dates = GetDates();

        #endregion #1


        #region #2

        // Loop through a previously generated List of Dates for which we need to send an API Request.

        foreach (var date in dates)
        {
            Console.WriteLine("Sending API Request for date: " + date);


            #region #3

            // Sending an API Request to initial Endpoint which will return which contains a specific JSON string which further contains data
            // about sports which, additionally, contains a collection of events which took place on defined date.
            // HTML string will serve as an input for next step.

            var html = string.Empty;

            var initialUri = "matches/soccer/" + date.ToString("yyyyMMdd");

            await webScraperService.GetAsync(initialUri, async stream =>
            {
                using (GZipStream gZipStream = new(stream, CompressionMode.Decompress))
                {
                    using (StreamReader streamReader = new(gZipStream))
                    {
                        html = await streamReader.ReadToEndAsync();
                    }
                }
            });

            #endregion #3


            #region #4

            // From HTML string from previous step, we need to extract a specific JSON string which contains data
            // about sports which contains a collection of events which took place on defined date.
            // Defined JSON string will serve as an input in next step.

            var pattern = ":data-next=\"" + "({.*?})" + "\"";
            var match = Regex.Match(html, pattern);

            var json = match.Groups[1].Value.Replace(""", "\"");

            #endregion #4


            #region #5

            // FROM JSON string We need to extract data based on which we will generate (and loop) list of sport urls
            // to which we need to send an API request in order to get Events which took place on defined date.

            using (JsonDocument jsonDocument = JsonDocument.Parse(json))
            {
                var dateSegment = jsonDocument.RootElement
                    .GetProperty("dataSegment")
                    .GetString();

                var dateCodeSegment = jsonDocument.RootElement
                    .GetProperty("encodedXhash")
                    .GetString();

                var sports = jsonDocument.RootElement
                    .GetProperty("tabs")
                    .GetProperty("sports")
                    .EnumerateArray();

                foreach (var sport in sports)
                {
                    var id = sport
                        .GetProperty("sid")
                        .GetInt32();

                    var slug = sport
                        .GetProperty("sportUrl")
                        .GetString();

                    if (id > 0) // value 0 is not relevant in this context (sportId is always greated than 0)
                    {
                        var sportUri = "ajax-nextgames/" + id + "/1/2/" + dateSegment + "/" + dateCodeSegment + ".dat";


                        #region #6

                        // We are sending new API request for each sport URL and saving the responce to a File Storage.

                        Console.WriteLine("Sending API Request for sport: " + slug);

                        await webScraperService.GetAsync(sportUri, async stream =>
                        {
                            var path = "Events\\" + date.ToString("yyyy-MM-dd") + "-" + slug + ".gz";

                            await fileStorageService.CreateAsync(path, stream);
                        });

                        #endregion #6
                    }
                }
            }

            #endregion #5
        }

        #endregion #1
    }
\$\endgroup\$
6
  • 3
    \$\begingroup\$ oooohhhh Evil regions. Regions is the code desperately crying out. "Help me, Help me!" I need structure! Methods! Cogent Comments! By Grapthar's Hammer! Fix me or kill me! \$\endgroup\$
    – radarbob
    Commented Mar 4, 2023 at 0:47
  • \$\begingroup\$ I know. This is why we are here in first place. What would be your way to organize those methods? Can you add some value to your feedback please? \$\endgroup\$ Commented Mar 4, 2023 at 5:29
  • \$\begingroup\$ touche´. OK I'll look at it. By the way, I've maintained code that used regions to good effect, but not so "narrowly scoped" as your example. Often it was for differing processing, line after line of simply setting values on the MANY properties in complex objects. \$\endgroup\$
    – radarbob
    Commented Mar 5, 2023 at 19:36
  • \$\begingroup\$ Explain GetDates() and its return value. You say they are "Dates" - Capital "D" so it's a C# Date object? A 'List' of said dates? But it is used directly in string concatination. \$\endgroup\$
    – radarbob
    Commented Mar 5, 2023 at 20:10
  • \$\begingroup\$ That is a very good point here. The GetDates() Method returns a list of DateOnly structs but you can't tell that from code. It is basically List<DateOnly> dates = GetDates(). \$\endgroup\$ Commented Mar 6, 2023 at 6:42

4 Answers 4

2
\$\begingroup\$

Quick remarks:

  • Don't use regions. Especially not ones with meaningless names, and even more so ones that are named incorrectly (check "region #1").

  • If you want to split up your code, use methods. The "inside" of the loop foreach (var date in dates) could be a method called ProcessForDate, for instance. Retrieving the HTML from the API: that's a method. Same for the inside of foreach (var sport in sports) etc.

  • Your code should explain itself. If you need a comment like // Generating a List of Dates for which we need to send an API Request., you're doing it wrong. Comments should only be used to explain why something was implemented in that way (i.e. in the case of code that looks odd etc.).

  • Why are you using jsonDocument.RootElement.GetProperty("tabs") etc.? Why not deserialize the returned JSON into a custom class using JSON.NET or another library?

\$\endgroup\$
1
  • \$\begingroup\$ I fully agree for the "regions" part. And the reason I'm not deserializing JSON into a custom class is that the JSON is written so badly. I would end up with a lot of non-meaningful and not-needed classes. As I only needed this one property, it was somehow convenient to do. OK, so I am breaking this large function into smaller methods. Would you prefer OOP or functional programming for this scenario? \$\endgroup\$ Commented Mar 6, 2023 at 7:09
2
\$\begingroup\$

For starters I'm thinking refactor each section to a method. This begins to make it "readable" but not so much "clean." Making methods means you cannot just pass the existing half done unusable stringy-thingies.

Both readable and clean will happen when all that string assembly gets separated from its use. Do all that "API call argument customization" at the beginning, get it out of the many API call iterations. Keep it all in a single object. Maybe a JSON object. Thus ugly string building syntax becomes a descriptive object property name.

Parameter passing will be clean because this object is an effective data structure. It contains the data in a regular-structured and useable form and the resulting using-code becomes surprisingly simple. At that point the code readers will easily see how this data structure is transformed into a calendar of sporting events.


My first thoughts are that this is a typical ETL process so I am thinking in a direction similar to SQL Server Integration Services (SSIS) with Control and Data flow (pipeline pattern) implementation, but it would take to much time and it might be an overkill for this scenario.

What in the Wide World of Sports ...? You suspect this is a bad idea but why? "too much time" and "overkill" is not addressing the technical issues. Instead: First, complexity does not simplify other complexity. Ask yourself: "Is the code not refactor-able in its current form?" "Is it necessary to meet the code goals and user requirements?" "Is this necessary to create desirable separation of concerns, decoupling, abstraction, readability, understandability?"

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the effort. It would be great to some a portion of example code related to the second paragraph of yours. \$\endgroup\$ Commented Mar 6, 2023 at 7:26
  • 1
    \$\begingroup\$ My vision is refactoring to methods showing the top level processing steps. Pulling out string building would be only the first step to encapsulate low level details. I.E. encapsulating detail in code, methods, variables, named so to describe the higher level "what's going on." Thus the process of the methods - regions - begin to stand out. I didn't because of my uncertainty about data types, class structures, and lack of deep understanding of process/steps. \$\endgroup\$
    – radarbob
    Commented Mar 6, 2023 at 21:44
1
\$\begingroup\$

This is a very minor addition to the other answers. Lift your regex pattern up to the class level along with a regex object representing it:

private const string pattern = ":data-next=\"" + "({.*?})" + "\"";
private static readonly Regex patternRegex = new(pattern, RegexOptions.Compiled);

and then in your code, simplify to:

var match = patternRegex.Match(html);

That will remove the boilerplate from the method itself, and possibly give some better performance of the matching.

\$\endgroup\$
1
  • \$\begingroup\$ A couple of minor improvements is the right way to a major one :) Right now I am like in a direction of pure Functional Programming regarding the topic and I will repost refactored code as soon as I complete it. \$\endgroup\$ Commented Mar 8, 2023 at 6:20
0
\$\begingroup\$

After refactoring the initial code (based on your valuable feedback and my further research) I ended up with the below solution. I moved the logic to 3 additional classes:

  • FileDownloader (responsible for downloading content from WebApi to local FileStorage)
  • DateCollection (responsible for generating a list of dates based on specified date range)
  • SportCollection (responsible for generating a list of sports)

It could be improved even more, but I am quite happy at the moment with how it looks now (in comparison to the original).

    public async Task ExecuteAsync()
    {
        DateCollection dates = new('2022-03-01', '2022-03-02');

        foreach (var date in dates)
        {
            string url = "matches/soccer/" + date.ToString("yyyyMMdd");
            string input = await webScraperService.GetStringAsync(url);

            SportCollection sports = new(input);

            foreach (var sport in sports)
            {
                await FileDownloader.DownloadAsync(sport.RequestUri, sport.Path);
            }
        }
    }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.