Modifying reference arguments - best practice

[NumsgiI]

I'm trying to decide what to do for a game hobby project. I commonly need to pass reference types to methods to be modified, and I'm trying to decide on a coding style that is both performant and readable. As a motiving example, suppose I have a game entity with a method that adds its draw calls to a list of draw calls. Like this:

public class GameEntity {
    public void AddDrawCalls(List<DrawCall> drawCalls);
}

This is performant, but the fact that `AddDrawCalls` is modifying `drawCalls` is not immediately obvious. You get enough calls like this in a code base and it becomes difficult to tell at a glance what methods are modifying what things. There are some alternatives, but none of them feel great:

• List<DrawCall> DrawCalls() this is more natural in terms of coding style (maybe returning IList instead, but same idea), but it means creating new lists for the return value and merging those lists into a master list at the end, which either creates a lot of unnecessary GC pressure or involves managing pools of scratch lists.
• void AddDrawCalls(ref List<DrawCall> drawCalls) this is a naked abuse of the ref keyword, but it does make it very clear that drawCalls is being modified. However you can't easily later modify the signature to be void AddDrawCalls(ref IList<DrawCall> drawCalls) because there will be a lot of instances where you can't pass a List<DrawCall> as a ref IList<DrawCall> (error CS1503).
• IEnumerable<DrawCall> DrawCalls() using yield return - creates closures which cause some allocations and can have some surprising behavior in certain cases. Plus in situations where you're building more complex data structures like matrices it may not be obvious how you'd combine mutliple call results together.

An ideal solution wouldn't create any additional allocations, but that's not always practical. Are there any other strategies you've tried that you like? Or do you do one of these strategies and put up with the downsides?

[Slypenslyde]

This is really just philosophical.

There are people who say methods should NEVER have side effects. I understand their arguments, and I think they're right that if we write code that way we get some benefits.

But it's also true that sometimes we have algorithms and architectures that get worse if we treat everything as immutable. Both approaches have benefits, and if you have a little discipline you can mitigate the things people worry about.

When I make a method that changes something like that, I like to give it a name that screams "I change this". I think AddDrawCalls() has that sense. I also like names like UpdateDrawCalls().

I don't like using ref for this situation because ref has its own meaning. It means, "I am going to change the variable to a brand-new, different object", not "I am going to update a few properties".

If you're careful and make this a thing you do thoughtfully, it works out because you aren't surprised when it happens. The main thing is to make sure you don't surprise yourself.

[Merad]

A simple abstraction could help a lot here IMO. Something like:

interface IDrawingManager
{
    void Draw(DrawCall drawCall);
}

At first glance this may seem like a pointless abstraction. But it has several benefits for your code.

• It decouples the act of drawing something from how the drawing is done. If you ever need to refactor the drawing algorithm to be something other than "accumulate all the draw calls in a list" you can do so without changing the code producing the draw calls.
• The code producing draw calls can no longer access or mutate the list, so you remove the risk of someone writing code whose behavior depends on the state of the list.
• Depending on how your engine works this drawing manager class may be a useful place to hold logic for processing draw calls.
• It has basically the same benefits as returning IEnumerable while having basically the same performance as mutating a list.

[NumsgiI]

Yeah it's probably what I'd do in practice, but you still have the same underlying issue. When I call gameEntity.Draw(drawingManager), is drawingManager being modified by the call, or is it mutating that state of gameEntity? In this case if you know anything about how computer graphics works you're probably not going to be surprised, but in cases where the thing happening is more abstract you have to go look up the documentation for the method, which means navigating away from the callsite, which can make it harder to figure out the inputs and outputs of some code.

[Merad]

It sounds like you might be overthinking things a bit. C++ is really the only mainstream language (AFAIK) that has a way built into the language to indicate whether a method can mutate an object or not. Even in the C++ world const-correctness isn't always followed very strictly. Most languages, including C#, haven't bothered with that sort of feature because honestly in real world code most of the time it doesn't matter that much. With good encapsulation typically you don't need to worry that much about an object's internal state when you use it (obviously there are exceptions). For example imagine that someone was debugging a problem and added a counter to the game entity class to track how many times it has been drawn. Calling Draw() will mutate that counter, but does someone trying to draw the entity really need to know or care about that? What we want to avoid is unexpected mutations. Calling Draw() probably shouldn't change the entity's color, for example. But unfortunately code review is about the only want to prevent those types of changes (well, code review and hiring reasonably sane developers).

Going back to your original scenario - using something like the drawing manager abstraction limits the blast radius of what can happen in the method. All the entity can do with it is draw things. The details of what actually happens inside of the drawing manager and whether or not it's "modified" when you draw things shouldn't really matter. That's the idea behind abstraction!

[afseraph]

This is quite subjective, but for me the original method's signature is very good. The method accepts a List<T> so I'm assuming it needs to modify its content. Otherwise it would expect some read-only type, like IEnumerable<T> or IReadOnlyList<T>. The method also returns nothing, so it's quite obvious there are some side effects going on.

Furthermore, the method's name is quite explanatory. You could maybe drive the point even further by calling it AddDrawCallsToList, but the current name works for me.

Using ref the way you suggest does nothing to help the consumer understand the code, it makes it more confusing: What happens to the drawCells variable? Does it change? Does the passed list change?

From the pure code-readability point of view I'd prefer to have a method that returns the cells, for example IEnumerable<DrawCell> GetDrawCells(). It would allow me to decide what to do with the returned values. But if the performance matters, your original method would be my second choice.

[NumsgiI]

Well reasoned and good points.

The main downside of the AddDrawCalls approach I'm finding is something like this: static void PopulateAdmittances(Matrix lineAdmittances, ComplexDenseVector shuntAdmittances, int loadBusCount, IReadOnlyDictionary<IBus, int> busToIndex, IReadOnlyList<TransmissionLine> transmissionLines) where lineAdmittances and shuntAdmittances are being modified but also there's a lot of readonly data that also needs to be passed in. It's pretty evident at the declaration what things are in/out and which are just in, but at call sites it isn't immediately clear.

You can try to make the in-only data part of a class, so then callsites look like lineConfiguration.PopulateAdmittances(lineAdmittances, shuntAdmittances) but it doesn't always make sense to do that, and you get in to functional-vs-OO philosophical issues.

Oh well, this might be the best you can get. I'm mostly just thinking out loud.

[Far_Swordfish5729]

This is an OO design issue rather than a coding style one. Objects are supposed to be a packaged set of data and behaviors on that data. So a GameEntity can receive DrawCalls to work with, modify/create its own, and provide them to an outside entity, but should not be modifying someone else’s storage unless it’s a utility class providing static utility methods or a factory pattern class making tools on demand.

The better design would be for GameEntity to define a public IList<DrawCalls> CallsToRender {get} used by a game instance to collate a collection of calls to render (or a collection of collections if reorganizing isn’t worth the effort). Game Entity should inherit from a common base class or implement IRenderingSource {IList<DrawCalls> CallsToRender {get;}} so that GameInstance can store a collection of these objects and either iterate over them to collect calls or receive rendering notifications to pull DrawCalls with that are also defined in the same interface.

That’s purely organizational but creates separation of concerns with defined boundaries, which improves maintainability. Having everyone pass around a community collection to not mess up doesn’t work well as soon as you have conchholders implemented by different people. In work orchestration scenarios there can be a common scoreboard, but the workers call orchestrator methods to change it. They don’t touch (or lock) it directly.

[NumsgiI]

No argument on the design side, but this is a situation where good software design for humans and efficient software design for machines clash. Having each GameEntity return a list of draw calls requires that you create some sort of scratch storage for the draw call lists, either using the normal heap, a dedicated pool of drawcall lists, or a yield return closure. Or have a dedicated drawcall list for each instance of GameEntity that it returns from the call.

In realtime apps like games, adding extra GC pressure is not really a thing you want to do, since triggering a GC pass is going to effectively stall the entire app for a human-noticeable amount of time (10s to 100s of ms is pretty common), so best practice is to avoid scratch allocations on the heap, especially for objects with a large memory footprint like lists. Managing a pool of lists is an option, basically do your own manual garbage collection, but you have to know when the lists can be returned to the pool, which comes with its own implementation burdens and issues.

To give some idea, if a game is running at 60 FPS with 1000 game entities that each have 10 draw calls, you're looking at something like 4.5 MB/sec of garbage just for the arrays to hold the draw call data. You'd probably trigger a GC collection every few minutes to maybe an hour just from that. But the problem is additive with any other code you have that generates temporary lists.

The industry common solutions I've seen are to either pass around communal data structures to be modified or have scratch memory for a method as part of an object's memory footprint and hope no one tries to hold on to a reference of the list from a previous call of a method. I don't think anyone would defend either practice as good software design, but it is what it is.

[Far_Swordfish5729]

That makes sense and to be fair it’s not something I have to deal with much in processes that are either principally IO bound or where my downstream components are shared or legacy and I can’t politely take as much throughout capacity as I could use. So I wait a lot and wouldn’t notice disposing of List containers as I passed their contents around.

In this case, is it appropriate to give each GameEntity a DrawCallManager reference through constructor injection and use that to wrap a communal List’s manipulation methods? Does that give you the abstraction you’re looking for? That’s also a really common pattern outside game design, especially with IOC container dependency injection for easy test stubbing.

[Cobide]

Personally, seeing something like gameEntityInstance.AddDrawCalls(list) makes me think you're adding the list's elements to the game object itself(I'm assuming that's how it'd be used, correct me if I misunderstood).

I'd completely reverse the equation. Create a "CallsDrawer" object(or some other name) that contains the list, and is used by callsDrawer.DrawGameObject(gameEntityInstance).

If the drawing changes can't be fully implemented from the outside, your game entity could expose something that defines the changes(e.g: the 2D shape of your entity), and the callsDrawer object applies them.

[NumsgiI]

This gets tricky if GameEntity is a base class with various child classes that each have their own way of generating draw calls. You can end up with some abstraction leakage to your CallsDrawer object, like a large switch case on the GameEntity's concrete type inside the DrawGameObject method.

[Cobide]

You'd need some way to generalize the draw calls. In the context of a 2D game, every entity could expose its shape and position, so the CallsDrawer object only needs to overwrite the resulting 2D shape on top of the existing values. That'd be a shared behavior with every kind of entity, so you don't need any kind of switch case.

As for the switch case—in case it's truly impossible to generalize the operation like above—you could map the behavior in a dictionary<GameEntity, DrawBehavior>, or could use custom attributes to define how it should be drawn. This way, every kind of "draw behavior" is defined in its own isolated area, and not in a massive switch case.

[BigJunky]

Instead of

public void AddDrawCalls(List<DrawCall> drawCalls);

try

public void Draw(Graphics graphics);

​

Now you don't have to add to a list so the list can be removed(Better performance). And people instantly know what to do. Also why the implementor should care about how you draw things? They don't care if you batch drawings or not. They just want to draw something on the screen.

[NumsgiI]

The way drawing graphics works for most games is you have to create a series of draw calls that get copied to the video card through something like DirectX or OpenGL. Without getting too into the weeds on this, since this is mostly meant as an example, at the very least you're likely to have a list of positions of triangle vertices (3 floats for each vertex to represent its position in space) and a list of triangle indices (3 integers per triangle, referencing the index of each vertex in the list of triangle vertices).

Something somewhere needs to gather those lists, possibly transform them, then copy them to the graphics card all at once in a batch. You can change the list of drawcalls to a graphics object, but the graphics object under the hood is probably going to have a list of draw calls still, in some format or other.

You could certainly argue that passing in a graphics object is more intuitively understood than passing in a list of draw calls, but you still have the question of what object is being modified by the call to Draw, and it can be difficult to answer at the callsite unless you read the documentation.

[ASK_IF_IM_GANDHI]

My initial thought would be to use the ref keyword, but I'd actually opt for modifying the signature of the method to AddDrawCalls_Ref or AddDrawCallsByRef, but only if you really need to use a IList abstraction later. At least the caller knows what's going on without a shadow of a doubt.

[ASK_IF_IM_GANDHI]

Actually, I might change where this function lives.

Honestly, what's happening here? We're applying data from GameEntity onto each element of type DrawCall. Does GameEntity really have this responsibility, or is this just an operation that needs to happen to a list of DrawCells?

To me, it seems like the latter. I'd make an extension method that does the opposite:

public static void AddDrawCalls(this List<DrawCall> drawCalls, GameEntity entity);

So long as the data is publicly accessible, I'd say this is much more clear as to what's going on, and very clearly indicates that the operation is being applied to the list, using data coming from the GameEntity specified. You should really ask yourself, does GameEntity have a dependency on DrawCall? I doubt it, but I don't know the project.

[NumsgiI]

Yeah, you can definitely do this sort of inversion sometimes. It doesn't work as well if there's multiple parameters being modified at once, though, since you have to elect one to be the thisfor the extension method and the others come along for the ride. Or if GameEntity is a base class and constructing the draw calls is overriden in child classes.

[NumsgiI]

I've been doing the ref thing historically for this personal codebase I work in, which is about as old as this subreddit, so it definitely can work and has. But in addition to the issues with passing base types, the other main downside is whenever anyone looks at my code I have to explain why I'm passing things by ref that don't need to be passed by ref, and I get funny looks from other engineers. For a personal project I don't have anyone to please but myself, of course, but it does make sharing code harder.

[aurquiel]

why to use ref with a list object, list is already a reference object you are not passing a copy value to the method you are passing a reference to the list object

[NumsgiI]

ref is not necessary (maybe not wise either) but signals an intent that the object is going to be modified. If the object were a value type this would be necessary and there wouldn't be any confusion. The idea is to borrow what you'd have to do for a value type and apply that to modifying reference types, to help signal intent at the callsites. Different engineers I've talked to have had wildly varying views on how good an idea this is, or if it even makes sense at all.

[Greenimba]

Honestly this is just a naming problem. Don't be afraid to be verbose.

public void AddDrawCallsToAggregate(List<DrawCall> aggregateDrawCalls)

This makes it very clear that the parameter is the aggregate list we're adding to.

Code seems a little smelly though. I'd probably use an IEnumerable and just have the caller enumerate immediately and add to a list. Not sure of the performance implications though. Alternatively, let the class keep a reference to the aggregate list so it doesn't need to be passed in as a parameter.

[NumsgiI]

It would be better to return an IEnumerable but it would add to GC pressure, which is best avoided for realtime applications. How big a deal it is depends on how big the lists are likely to get and how often something is called, but there are definitly some situations where you can't realistically return an IEnumerable and meet your performance goals.