C # Encoding Code

Question

The other day I looked at a code similar to this code in the code review.

public void DoSomeTasks() { if (CheckSomeState()==true) return; DoTaskOne(); if (CheckSomeState()==true) return; DoTaskTwo(); if (CheckSomeState()==true) return; DoTaskThree(); if (CheckSomeState()==true) return; DoTaskFour(); } 

As the number of tasks increases, the code ends up with ever higher cyclic complexity, and it just doesn't suit me either.

The solution I came across resolves this.

 private void DoTasksWhile(Func<bool> condition, Action[] tasks) { foreach (var task in tasks) { if (condition.Invoke()==false) break; task.Invoke(); } } 

Used as

 public void DoSomeTasks() { var tasks = new Action[] { {()=DoTaskOne()}, {()=DoTaskTwo()}, {()=DoTaskThree()}, {()=DoTaskFour()} } DoTasksWhile(()=>CheckSomeState(), tasks); } 

Anyone have suggestions to make the code more readable?

Answer 1

I did a little refactoring of your implementation

 private void DoTasksWhile(Func<bool> predicate, IEnumerable<Action> tasks) { foreach (var task in tasks) { if (!predicate()) return; task(); } } 

• You do not need to delegate Invoke . Just follow them.
• do not compare boolean values ​​with true/false (this is useless, and you can assign a boolean by mistake)
• this way you only list tasks IEnumerable is good for parameter

You can also create an extension method.

 public static void DoWhile(this IEnumerable<Action> actions,Func<bool> predicate) { foreach (var action in actions) { if (!predicate()) return; actions(); } } 

Use will be very simple:

 tasks.DoWhile(() => CheckSomeState()); 

Answer 2

If your orchestration code is very complex, consider using a framework such as WF (workflow framework).

Otherwise, your code is fine, but I would not change the original one, since it is more readable. It is also more flexible since in the future you can change these IFs, i.e. Conditions may not remain identical.

I do not see how cyclomatic complexity decreases or can be reduced.

Answer 3

Your solution is quite adequate, because it costs (although it may be too large if only four steps), but whether this is the best approach in this case depends on what the purpose of CheckSomeState is actually in the context of your program

• Is it important that CheckSomeState is called exactly once between each task?
• Perhaps sometimes it also needs to be called in the middle of the task?

So, for example, if CheckSomeState actually checks the cancel flag, then it may need additional verification as part of a long-term task.

Another option available to you is an exception (for example, an OperationCancelledException). This reduces your cyclomatic complexity very low, as you can now simply:

 CheckForCancel(); DoTaskOne(); CheckForCancel(); DoTaskTwo(); 

(The disadvantage is that some believe that this is the use of exceptions for the control flow, which is generally disapproved of).

I should also add that the goal should not be to reduce cyclic complexity, but to have code that is easy to understand and maintain. Cyclomatic complexity is a useful metric, but sometimes it unnecessarily punishes code that is pretty simple.