0

I apologize if I'm posting into the wrong community, I'm quite new here.

I have multiple methods using the same foreach loop, changing only the inner method I call:

    public void CalculationMethod1()
    {
        foreach (Order order in ordersList)
        {
            foreach (Detail obj_detail in order.Details)
            {
                CalculateDiscount(obj_detail);
            }
        }
    }

    public void CalculationMethod2()
    {
        foreach (Order order in ordersList)
        {
            foreach (Detail obj_detail in order.Details)
            {
                CalculateTax(obj_detail);
            }
        }
    }

Each inner method has different logic, database search, math calculations (not important here).

I'd like to call the methods above without repeating the foreach loop everytime, so I throught about the solution below:

    public void CalculateMethod_3()
    {
        foreach (Order obj_order in ordersList)
        {
            foreach (Detail obj_detail in order.Details)
            {
                CalculateDiscount(obj_detail);
                CalculateTax(obj_detail);
            }
        }
    }

But I fall into a rule problem:

 class Program
 {
    static void Main(string[] args)
    {
        Calculation c = new Calculation();
        c.CalculateMethod_3();
        c.AnotherMethod_4(); //It doesn't use objDetail
        c.AnotherMethod_5(); //It doesn't use objDetail
        c.CalculateMethod_6(); //Method 6 needs objDetail but respecting the order of the methods, so It must be after AnotherMethod_4 and AnotherMethod_5
    }
 }

How can I create a method to achieve my objective (I don't want to repeat code) respecting the rule above?

5
  • 1
    I don't understand what the problem with your solution is. It looks fine to me. Commented Dec 11, 2014 at 17:15
  • The problem with this solution is that CalculateMethod_6 needs obj_detail, but i can't call CalculateMethod_3() again, because I don't need to CalculateDiscount or CalculateTax anymore. Actually, I can't call. Commented Dec 11, 2014 at 17:19
  • So CalculateMethod_6 also has the similar foreach style and you just want to "switch out" the contents inside the loops? Commented Dec 11, 2014 at 17:21
  • @moarboilerplate basically I don't want to code the foreach block for every new method I create, is it a bad practice to repeat? Commented Dec 11, 2014 at 17:26
  • Just added an answer--In my opinion, only if you were duplicating logic inside the loops as well. Commented Dec 11, 2014 at 17:27

5 Answers 5

9

You can always pass a delegate to the method and then you can do basically whatever you want.

public void ApplyToDetails(Action<Detail> callback)
{
    foreach (Order order in ordersList)
    {
        foreach (Detail obj_detail in order.Details)
        {
            callback(obj_detail);
        }
    }       
}

Then to use you'd do something like this

ApplyToDetails(detail => CalculateTax(detail));
ApplyToDetails(detail =>
{
    CalculateDiscount(detail);
    CalculateTax(detail);
});
Sign up to request clarification or add additional context in comments.

3 Comments

And of course you can call the parameterless methods as well: ApplyToDetails(detail => AnotherMethod_4());
Thank you guys, It's indeed a way to go!
You can simplify the the call to ApplyToDetails(CalculateTax) (without parameter braces after CalculateTax), since CalculateTax corresponds to a Action<Detail> already.
1

Delegates come in very handy in many cases and definitely in such a case. I know this has already been answered and rightly so, but here is an alternative for comparison. I have provided a link to give you some insight.

public class CalculationMethods
{
    public delegate void CalculationDelegate(Detail method);

    private Dictionary<string, CalculationDelegate> _methods;

    public CalculationMethods
    {
        this._methods = new Dictionary<string, CalculationDelegate>()
        {
            { "Discount", CalculateDiscount },
            { "Tax",      CalculateTax      }
        };
    }

    public void Calculate(string method, Detail obj_detail)
    {
        foreach (Order order in ordersList)
        {
            foreach (Detail obj_detail in order.Details)
            {
                var m = this._methods.FirstOrDefault(item => item.Key == method).Value;
                m(obj_detail);
            }
        }
    }
}

Usage:

//Initialize
var methods = new CalculationMethods();

//Calculate Discount
methods.Calculate("Discount", obj_detail);

//Calculate Tax
methods.Calculate("Tax", obj_detail);

Side Note: I would recommend some exception handling in case the method of calculation isn't found among the list of delegates. Example below: (Replace the calculate method with the following.)

public void Calculate(string method, Detail obj_detail)
{
    foreach (Order order in ordersList)
    {
        foreach (Detail obj_detail in order.Details)
        {
            var m = this._methods.FirstOrDefault(item => item.Key == method).Value;

            //Check if the method was found
            if (m == null)
                throw new ApplicationNullException("CalculationDelegate")

            m(obj_detail);
        }
    }
}

Decent tutorial: Delegates and Events

Comments

0

You can use delegates. (Google it - I don't have a development environment in front of me to run up a sample for you). Basically one method that takes a delegate to call: Here is pseudo code...

public void CalculationMethod(delegate myFunction) // need to look up correct param syntax
{
    foreach (Order order in ordersList)
    {
        foreach (Detail obj_detail in order.Details)
        {
            myFunction(); // Need to lookup correct calling syntax
        }
    }
}

I googled "c# delegate as parameter" and came up with http://msdn.microsoft.com/en-us/library/ms173172.aspx which seems to be a reasonable explanation.

1 Comment

Yes, it matches with the others solutions provided. Thanks!
0

As Darren Kopp says, you can use delegates. However, in the case that you are calling a method with a parameter, you can call it directly (you don't need the lambda expression).

With public void ApplyToDetails(Action<Detail> callback) { ... }:

ApplyToDetails(Method_1);         // Uses objDetail
ApplyToDetails(d => Method_2());  // Doesn't use objDetail
ApplyToDetails(d => Method_3());  // Doesn't use objDetail
ApplyToDetails(Method_4);         // Uses objDetail

Note that you must not place parameter braces after the methods you pass as delegate!

Comments

-1

You could use delegates as the other answers have provided but I believe in your case doing so will lead to overly confusing code. Your code is cleaner and more readable if you redeclare the foreach loops in each method. Only if you were copy-pasting portions of the internals would I say you run the risk of code duplication.

Think about it this way: If you created a method passing in a delegate, what would the name of this method be called? It is a method that does something for each Detail in each Order you pass in and should be named something like DoSomethingForEachDetailInOrders(). What kind of class would this method exist for? You don't know what it is you're actually doing in the delegate, so the purpose of this class would have to be more framework-style code, which your app does not appear to be complex enough to warrant.

Additionally, if you were debugging this code or reading through it, instead of being able to see 2 foreach loops inside the method you are reading, you have to go scroll to the definition of the delegate, read that, and then go back to your method and resume reading.

Edit: I originally answered this question by downplaying the duplication of the foreach loops in the hopes that OP would not add additional complexity to his app attempting to make it follow "best practices." I didn't go deeper because the code requires a more intrusive refactor for maintainability. The foreach loop code smell stems from other problems as detailed in the comments below this answer. I still stand by my opinion that adding the delegate method is less desirable than the duplicated loops because the delegate method option is pretty much textbook boilerplate.

Added a code example to explain how the code should be refactored if maintainability is a concern:

public decimal CalculateDiscount(IEnumerable<Order> ordersList)
{
    return ordersList.SelectMany(order => order.Details).Sum(detail => detail.Discount);
}

public decimal CalculateTax(IEnumerable<Order> ordersList)
{
    return ordersList.SelectMany(order => order.Details).Sum(detail => detail.Total) * taxRate;
}

If you ABSOLUTELY MUST HAVE a custom function for getting all details for orders (could be refactored to an extension method):

public IEnumerable<Detail> GetDetailsForOrders(IEnumerable<Orders> orderList)
{
    foreach(var order in orderList)
    {
        foreach (var detail in order.Details)
        {
            yield return detail;
        }
    }
}

public decimal CalculateDiscount(IEnumerable<Order> ordersList)
{
    return GetDetailsForOrders(ordersList).Sum(detail => detail.Discount);
}

public decimal CalculateTax(IEnumerable<Order> ordersList)
{
    return GetDetailsForOrders(ordersList).Sum(detail => detail.Total) * taxRate;
}

8 Comments

Think of the more complicated case where you have to traverse a binary tree for instance. It makes sense to have an ApplyToAllItems method, since it provides an abstraction. The caller does not need to know the details of the data structure. If in future the data structure changes, this will not affect the caller code, just the Apply... method.
That was exactly my thought...code maintanance, that's because the code repetition concerned me.
The solution I provided above implements a Dictionary of delegates. With well structured syntax and the use of proper documentation, readability shouldn't suffer. Delegates help to provide an avenue to easily extend the application. The more generic and modular your syntax is, the more extendable it becomes.
@OlivierJacot-Descombes I agree with you within the context of your example and when the state of the passed-in object needs to change. Within the context of OP's code, I disagree. The root of OP's issue is that his code is inside-out. He is calling a void aggregate method that modifies his class's global state (which is why he has issues with the order of function calls). The abstraction you mention can be achieved by basically reimplementing SelectMany and applying the aggregate function across the resulting collection, but using delegates as recommended actually exacerbates duplication.
@DaniloRuziska If you're worried about maintenance, duplication of foreach loops should not be your primary concern. Your concern should be that your methods are procedural and not functional, which is why the code smell of duplicate foreach loops is popping up. If you reverse the way your functions are called and so you can call CalculateTax(GetDetailsForOrders(orders)) your code will be 100x more maintainable.
|

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.