External Void method to change referencing object parameter bad practice?

[beantownace]

Hello all,

I have more of a best practices question always fun to ask.

I have a class with a worker method that does a decent amount of logic. One part of the method I broke out was a part that calculates and updates two objects which exists within a ForEach loop. I created a Utility class that takes in the two object parameters into a Void method does the calculation and sets some of the object property values but the Void does not have a return as the objects are being passed by reference. I of course don't need to use ref before the param in this case and the external object to that utility method now has the updated objects. Also seems this is just updating the original passed object parameters and not making a copy of it as I have a Sum later and things are working as I expect so performance should be fine I think at least.

Is this bad practice to have a void external in a Helper type class for calculating? The reason I ask is that Void to another develop could cause confusion that it does not return anything unless you saw the referencing class using that void method. Curious thoughts on this as I am trying to break up the code as best I can but this one peaked my interest if it makes sense. Also only the CustomerSetup class will have use for that Calc method in the helper class so may want to just keep it within the setup class itself but it is a good use of a static method.

Example:

public class CustomerWorker
{
   public void CustomerSetup
   {
       //code
       foreach(var invoice in invoices)
       {
           InvoicePO po = //call fills this object
           InvoiceAcct acct = //call fills tis object
       
           //calls an external calc method to change two properties in these
           InvoiceHelper.CalcInvoice(po, acct);
       
           //now objects are changed and does other things
       }
   }
}

public static class InvoiceHelper
{
   public static void CalcInvoice(InvoicePO po, InvoiceAcct acct)
   {
       //does some calculations on po and acct
       // no return of course but external static method call objects are updated
   }
}

 

[Skydiver]

Methods that return void and transform the inputs are common. It's often just a matter of applying better naming on the method names so that it's not surprise to the caller, OR having just built up enough of a team lore or culture that understands method XYZ will modify its inputs.

Yes, I know that is pretty counter to the current trend of "functional programming" where the methods do not modify the inputs and return transformed new objects instead. I guess it really depends in which camp you belong to.

 

[Skydiver]

Another thought that came to mind. In general, in OOP you tell the object to modify itself. You don't go in and modify the properties of the object directly. (Yes, I know the line gets blurry with the Java style getter/setter methods and the corresponding C# properties which are the equivalent of the Java getter/setter.)

Unless the objects are are just POCO's or DTO's, then you should be following the Law of Demeter.

https://www2.ccs.neu.edu/research/demeter/demeter-method/LawOfDemeter/paper-boy/demeter.pdf

 

[Skydiver]

Anyway, your original question sort of boils down to using something like Array.Sort() or String.ToLower() and the confusion that ensues from the inconsistency. From a naive point of view: If you are used to String.ToLower() returning a new string, then why does Array.Sort() not return a new array? (Of course from a non-naive point of view, it comes down to knowing that C# strings are immutable, so there is no choice but to return a new string.)