0
\$\begingroup\$

This is my first question so please let me know if this is too vague/broad and I will do more research on question asking.

I have implemented a handful of data loaders in C#, and have come to the point where I have what I consider in my limited experience a decent structure, but I'm not confident in implementing the methods that do the actual loading. It smells bad since each method has so many parameters, but I can't see any way around it- there are too many genuine variables for querying an endpoint.

I have structured it so that there is an API Client Manager that scheduled runnable tasks will query for various endpoint data loads. Each endpoint is parsed to an enumerator which is then routed by version/name to the appropriate LoadEndpointNameVersion method, which calls the endpoint and merges/upserts the data into a target database.

Each endpoint follows this general method:

public void LoadEndpointName(string requestName, dataloader_APICredential credential, dataloader_APIEndpoint endpoint, dataloader_Organisation organisation)
    {
        try
        {
            _supervisor.Report($"{_taskConfig.Name} - {requestName} - Begin");

            var securityToken = GetSecurityToken(credential);

            var clientEntityList = new List<ClientEntity>();
            var endpointLastRunTime = _repository.GetOrganisationEndpointLastRunTime(organisation.OrganisationId, endpoint.APIEndpointId);

            var groupIds = new List<string>(); //this will either be a list of modified Ids if incremental or all Ids if first run

            if (endpointLastRunTime == null || organisation.LastRunTime == null)
            {
                groupIds = _repository.GetAllClientGroupIds(organisation.ConnectionString);
            }
            else
            {
                groupIds = GetModifiedClientGroupIds(credential, organisation.OrganisationName, (DateTime)organisation.LastRunTime);
            }

            foreach (var groupId in groupIds)
            {
                var requestUrl = $"{credential.BaseUrl}/{endpoint.Version}/{endpoint.EndpointName}/clientGroupId/{groupId}";
                var response = APIClientHelper.SendGetRequest(_client, requestUrl, securityToken);

                if (response.StatusCode == System.Net.HttpStatusCode.OK)
                {
                    var result = (List<ClientEntity>)JSONHelper.Deserialize(response, typeof(List<ClientEntity>));

                    if (result.Count > 0)
                    {
                        clientEntityList.AddRange(result);
                    }
                }
                else
                {
                    if (response.StatusCode != System.Net.HttpStatusCode.NoContent)
                    {
                        _supervisor.Report(APIClientHelper.GetAPIResponseException(response, response.StatusCode, credential.Description, requestUrl), "APIClient.LoadClientEntity");
                    }
                }
            }

            var clientEntities = APIToDbObjectConverter.ConvertClientEntity(clientEntityList);
            _supervisor.Report($"{_taskConfig.Name} - {requestName} - {StringHelper.ReportSync(clientEntities)}");

            if (clientEntities.Count > 0)
            {
                DataSyncRepository.Sync_ClientEntity(organisation.ConnectionString, clientEntities, "APIName.API");
            }

            var addresses = APIToDbObjectConverter.ConvertEntityAddress(clientEntityList);
            _supervisor.Report($"{_taskConfig.Name} - {requestName} - {StringHelper.ReportSync(addresses)}");

            if (addresses.Count > 0)
            {
                DataSyncRepository.Sync_Address(organisation.ConnectionString, addresses, "APIName.API");
            }

            var electronicAddresses = APIToDbObjectConverter.ConvertEntityElectronicAddress(clientEntityList);
            _supervisor.Report($"{_taskConfig.Name} - {requestName} - {StringHelper.ReportSync(electronicAddresses)}");

            if (electronicAddresses.Count > 0)
            {
                DataSyncRepository.Sync_ElectronicAddress(organisation.ConnectionString, electronicAddresses, "APIName.API");
            }

            var dueDates = APIToDbObjectConverter.ConvertEntityDueDate(clientEntityList);
            _supervisor.Report($"{_taskConfig.Name} - {requestName} - {StringHelper.ReportSync(dueDates)}");

            if (dueDates.Count > 0)
            {
                DataSyncRepository.Sync_DueDate(organisation.ConnectionString, dueDates, "APIName.API");
            }


            _repository.UpdateOrganisationEndpointLastRunTime(organisation.OrganisationId, endpoint.APIEndpointId, DateTime.Now);
            _supervisor.Report($"{_taskConfig.Name} - {requestName} - Finished");
        }
        catch (Exception ex)
        {
            _supervisor.Report(ex, "APIClient.LoadClientEntity");
        }
    }

I have removed most of the identifying features (Name of the API and EndpointName etc). This is more of a broad design question, but I was looking for general pointers on any patterns that should be employed or how this should be parted out to adhere closer to the single responsibility principle.

I'm torn, as I want clean, readable and straight forward code, but all I've found so far when looking at code from colleagues that are OOP/Uncle bob zealots, is that you end up with 100 parameterless method calls between 50 classes, and it makes the code very difficult to follow. With the structure I've listed here, there are 5 straight forward calls- there is a database configuration with the load schedule, a database configured endpoint, it is converted to an enum for routing through a switch, and then the LoadEndpointName method does exactly that- calls the endpoint, and loads the response data.

I feel like if I hand this over to a colleague, it's very easy to follow and extend, when I look at OOP-ish examples, I feel like I'm constantly going to different class definitions, and it's much harder to keep a big picture in my head.

TLDR: Is there an OOP-Approved pattern/structure for the above method that avoids abstraction/classes to the Nth degree? Or is the issue me and the sooner I come around to the uncle bob style the better.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Have you ever used the Unified Modeling Language (UML) to draw diagrams of objects? These diagrams can actually help you keep track of the all the different objects. The Uncle Bob object oriented design methodology is based on a lot of research by multiple computer scientists in the 1980s and 1990s. In addition to Robert Martin, there is Grady Booch, Ivar Jacobson and Jim Rumbaugh, these 3 created the Unified Modeling Language. \$\endgroup\$
    – pacmaninbw
    Commented Jun 13, 2022 at 12:08

1 Answer 1

2
\$\begingroup\$

Some quick remarks:

  • Do not use underscores in any name, e.g. dataloader_Organisation or Sync_ClientEntity (except the underscore at the start of a private property that is global to the class, like _supervisor).

  • Abbreviations of more than two letters should be PascalCase in class names etc., e.g. API should become Api.

  • Do not call things "xxxxList" etc., e.g. clientEntityList. If you decide to change it to an array of a HashSet etc., your name becomes invalid.

  • I'm presuming organisation.LastRunTime is a nullable DateTime. Then don't cast this to a DateTime if you're sure it isn't null, but use .Value.

  • Why name a property "ClassName" inside its own class? I find things like organisation.OrganisationName to be annoying because there is duplicate information.

  • Looking at (List<ClientEntity>)JSONHelper.Deserialize(response, typeof(List<ClientEntity>)); I cannot help but think that JSONHelper.Deserialize is far too generic. You need to pass the type and then cast the result back to that type? This is just asking for unintended errors.

  • I have no idea what the ReportSync method of StringHelper does. This method name does not convey what it does.

  • APIToDbObjectConverter sounds like a class with very specific code, and I wouldn't be surprised if something like AutoMapper would be a much better solution.

  • I'm not a big fan of .Count > 0, I prefer .Any().


I am a bit baffled that you have a Repository where you need to provide the connection string as a parameter in the method. That seems inelegant and even somewhat dangerous.


As far as "too many parameters": four is well within acceptable limits, IMHO.

I'm less in love with a method that's less than 100 lines but does a lot of things in those 100 lines. I'd certainly split it up into at least three smaller methods, e.g. one to retrieve the groupIds, one to fill the clientEntityList, and one to handle all the conversions.

Since this appears to be a single method in a presumably larger class, I'd even consider converting this single method into its own class and then splitting it up in those three "sub-methods".

\$\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.