All geek questions in one place

Refactoring inner loops with lots of dependencies between levels

I have the following preudo-code

using (some web service/disposable object) { list1 = service.get1(); list2 = service.get2(); for (item2 in list2) { list3 = service.get3(depending on item2); for (item3 in list3) { list4 = service.get4(depending on item3 and list1); for (item4 in list4) { ... } } } }

where all the code has, say, 500 lines with a lot of logic inside the for statements. The problem is that refactoring is readable and supported by the code and as best practice for such situations. Here are the possible solutions that I have found so far.

1: Divide into methods Given that we extract each for into our own method to improve readability, we get method2, method3 and method4. Each method has its own parameters and dependencies, which is good, except for method4. Method4 depends on list1, which means that list1 must also be passed to methods 2 and 3. In my opinion, this becomes unreadable. Any developer looking at method2 will understand that it makes no sense to list1, so he should look down the chain until method4 can actually implement the dependency → inefficiently. Then, what happens if the variable list4 or item4 changes and no longer needs to depend on list1? I need to remove the list1 parameter for method4 (which should be done, of course), but also for methods 2 and 3 (outside the range of changes) -> again inefficient. Another side effect is that in the case of many dependencies and several levels, the number of transmitted parameters will increase rapidly. Think about what happens if list4 also depends on list11, list12, and list13, all created at list1 level.

2: Keep a long single method The advantage is that each list and item can access each parent list and item, which makes further changes one-line. If list4 is no longer dependent on list1, just remove / replace the code without changing anything. Obviously, the problem is that the method is a couple of hundred lines, which we all know is not good.

3. The best of both worlds? The idea is to divide into methods only the internal logical part of each for . Thus, the main method will decrease, and we will get readability and, possibly, maintainability. Leaving for in the main method, we can still access each dependency as a single layer. The result is something like this:

 for (item2 in list2) { compute(); list3 = service.get3(depending on item2); for (item3 in list3) { compute(item2, eventually item1) list4 = service.get4(depending on item3 and list1); for (item4 in list4) { ... } } }

But there is another problem. What do you call these compute methods for reading? Let's say that I have a local variable instanceA or type ClassA , which I populate from item2. Class A contains the LastItem4 property, which must be saved, for example, by the last item4 in the order the date was created or something else. If I use compute to create instanceA , then this calculation will only be partial. Since the LastItem4 property needs to be filled at the item4 level, in another calculation method. So, how can I name these 2 calculation methods in order to suggest what I'm actually doing? The tough answer, in my opinion.

4. #region Leave one long method, but use regions. This is a feature strictly used in some programming languages, and it seems to me that this is a patch, and not as a best practice, but maybe it's just me.

I would like to know how would you do this refactoring? Keep in mind that this can be even more complicated. The fact that this is a service is a little misleading, the idea is that I don't really like using disposable objects as method parameters, because you don't know the intent without reading the actual method code. But I expected this in the comments, as others may feel different.

For example, suppose the service is third-party, without the ability to change the way it works.

+6
source share
4 answers

I ended up using solution 3 described in the question, combined with DTO. This is not necessarily the best choice, but it is better than the others in terms of or readability, flexibility and scaling.

+1
source

You seem to be looking for a general answer, not a specific answer. The key to a common answer is the sharing of responsibilities and the pursuit of the principle of shared responsibility.

Let me start by saying that your method does many things toooooooo. This many things, ideally, a class should not do either. This should be done using a class group.

Robert C. Martin used to say: “Classes tend to hide in methods,” which is what happens here. If your method is large, you can reorganize it into smaller methods, but if a local variable is needed in this method, this method should probably go in its class.

If you need some fields for sharing, they must live together (I mean the same type). Thus, all your List1, List2, List3, List4 should live in a class, and not as method parameters.

Given that you cannot change the Service class (if it is a third-party API). In this case, you can always create your own class that wraps the above service.

So, you are essentially a class that looks like this:

 public class ServiceWrapper { private IList<string> list1; private IList<string> list2; private IList<string> list3; private IList<string> list4; private readonly Service1 service; public ServiceWrapper(Service1 service) { this.service = service; } public SomeClass GetSomething() { list1 = service.Get1(); list2 = service.Get2(); foreach (var item2 in list2) { PopulateMore(item2); } return result;//Return some computed result } private void PopulateMore(string item2) { list3 = service.Get3(item2); foreach (var item3 in list3) { PopulateEvenMore(item3); } } private void PopulateEvenMore(string item3) { list4 = service.Get4(item3); foreach (var item4 in list4) { //And so on } } }

Given that your service looks something like this:

 public class Service1 : IDisposable { public void Dispose() { throw new NotImplementedException(); } public IList<string> Get1() { throw new NotImplementedException(); } public IList<string> Get2() { throw new NotImplementedException(); } public IList<string> Get3(string item2) { throw new NotImplementedException(); } public IList<string> Get4(string item3) { throw new NotImplementedException(); } }

So your very long method becomes very small.

 public void YourVeryLongMethodBecomesVerySmall() { using (Service1 service = new Service1()) { SomeClass result = new ServiceWrapper(service).GetSomething(); } }

As you said in the comments, if you have other responsibilities inside these methods, you need to separate these responsibilities and move them to your class. For example, if you want to parse something, it should go inside the Parser class, and ServiceWrapper should use the parser to complete the task (ideally with some abstraction and free communication).

You should extract the methods and classes as much as possible so that it makes no sense to extract them more.

+1
source

The short answer is, it depends on your problem domain and how much you can divide your method into verbs that make sense, but overall option 3 is the best choice that matches my experience.

Oh #region, do not use it. It is best used to split the generated code into your code. Using a region to "structure" your code is like putting garbage under a carpet, it's still there, and you can feel it.

One other thing to note: you may have encountered a collection problem, and a good library like Guava (google collections) will help you a lot. Additionally, using Java 8 lambda or .Net LINQ functions can help you rewrite code and make it more expressive.

+1
source

Do not use regions. They do not help if you print, if you use CodeMap, etc.

Compose the variables list1, list2, .. private class.

Create a method for each for() loop:

 private void method3() { for (item3 in list3) { list4 = service.get4(depending on item3 and list1); method4(); } }

Make some good comments for each method ...

This has the added benefit of providing nice unit tests for each methodX()

0
source

Source: https://habr.com/ru/post/982378/

More articles:


All Articles