7

Although this question might sound stupid at first glance, please hear me out.

c#'s get{} and set{} methods are increadibly usefull in situations when you do not know how you porgramming goals will evolve while you build your code. I enjoyed their freedom many a time and now I wonder if there is something similar for methods but in a bit different light.

As I am working in gamedev, it is a very common practice to extend/update/improve existing code day-in day-out. Therefore, one of the patterns I taught myself to follow is to never use "return" statement more than once in the most of my methods.

The reason why I do this is to always be able to write something at the bottom of the method and be sure that the line I have written is always called 100% of the time once my method ENDS.

Here is an example:

    public void Update()
    {
        UpdateMovement();


        if (IsIncapacitated)
            return;

        if (IsInventoryOpened)
        {
            UpdateInventory();
            return;
        }

        if (Input.HasAction(Actions.Fire))
        {
            Fire();
            return;
        }
        else if (Input.HasAction(Actions.Move))
        {
            Move(Input.Axis);
            return;
        }


    }

Now imagine that this method is called dozens of times in many places across the entirety of your project. And then the next day you decide that you need to call UpdatePhysics() method at the very end of your Update() method. In this case there are only 4 returns, it could be much worse in reality.

Then imagine that such decesions happen several times a day every day. Bad planning you may say? I might agree with you, but I do think that freedom of development is essential in modern coding. I don't think you should kill yourself trying to anticipate every turn your project might take before you start writing code.

One way to insure that problems like the one I described above never happen is to rewrite the method as follows:

    public void Update()
    {
        UpdateMovement();


        if (!IsIncapacitated)
        {
            if (IsInventoryOpened)
            {
                UpdateInventory();
            }
            else
            {
                if (Input.HasAction(Actions.Fire))
                {
                    Fire();
                }
                else if (Input.HasAction(Actions.Move))
                {
                    Move(Input.Axis);
                }
            }
        }

    }

In this case you can always add a line at the bottom and be sure it will always get called nomatter what.

So I wanted to ask if there is another approach that could allow for placing "return"-s wherever you wish while still being able to add extra code easily at the bottom of the method any time. Maybe there is any form of syntax in c# that does it for you? Or maybe there is a better coding practice that eliminates such problem?

UPDATE: As I started receiving answers, I realized that I need to clarify things a bit.

  1. 'try/catch/finally' is an overkill - I will never use them. They have severe performance penalty on catch(), they screw up 'Edit and continue' feature in Visual Studio and they just look ugly.

  2. Idealy I need to be able to access local variables in the Update() method from any code I decide to add at the end of the method,

  3. When I wrote the question, I already had an answer - nesting. My second code sample has no returns and, therefore I can add code to the very bottom of the method and it will work 100% of the time, while I will be able to use local variables. Nesting is bad though, and that is why I am here searching for a BETTER solution.

UPDATE 2: I was actually mistaken about try/catch because I did not know that you can skip catch alongside it's performance penalties and only have finally. However, this solution is still worse than the nesting solution provided in the question, because in your newly added finally block you no longer can use return statements. So basically you can do whatever you want when you write the method the first time, but once you extend it - you are back to nesting.

cubrman
  • 884
  • 1
  • 9
  • 22
  • 4
    try/finally or (when applicable) Task continuation... – Adriano Repetti Jun 14 '16 at 12:13
  • I guess you're looking for a "onReturn" or "beforeReturn" event to subscribe to? I'd go with Try/Catch/Finally though. Depends on your error handling – Jamadan Jun 14 '16 at 12:17
  • 'finally' does not work without 'try'. Covering the entire method in 'try', on the other hand, can lead to increadibly weird behaviour as errors would be completery skipped. Moreover, try cannot be used in release builds due to performance reasons (I do know that penalty only happens when the error is caught) except for special cases. Did not understand what you ment by "Task continuation" though... – cubrman Jun 14 '16 at 12:19
  • Be aware, the code you put in finally will get called even if there is an exception in the try (handled or not). Not saying not to use it, just be aware of the effects. – Kevin Jun 14 '16 at 12:19
  • Guys I would never use try/catch - that's really over the top. Not to mention that I like to use 'edit and continue' feature and try/catch would really screw things up in large methods. – cubrman Jun 14 '16 at 12:21
  • Why not move the original code to a private method, then in your `Update` just call that private method and then call `UpdatePhysics()`? – juharr Jun 14 '16 at 12:29
  • @juharr see the Joe C's answer below and my comment to it. Basically you cannot access local variables in that case. In my example - you can. – cubrman Jun 14 '16 at 12:31
  • So you're actually passing values to `UpdatePhysics`? You could return those values from the private method I mentioned to then be used by `UpdatePhysics`. Mostly I think this is just a matter of breaking down what you want into the correct number of discrete units of work (methods) and then composing them together. – juharr Jun 14 '16 at 12:41
  • @juharr what you say is very logical and you are absolutely right, things should be divided into chunks. However, the nature of my question is in dealing with uncertain projects without clear roadmap. If I have to revrite my method, so that it returns some extra variables every time I need to change something - it's insanely uncomfortable. I'd rather nest my returns and live with it - then I can extend things whenever I please. Obviously I would use standard 'divide into chunks' approach if I see that it's absolutely nesseccary, but not day-in day-out. – cubrman Jun 14 '16 at 12:47

8 Answers8

6

One simple suggestion is to wrap your function. For example:

public void UpdateCall()
{
   Update();
   AfterUpdate code goes here.
}
Joe C
  • 3,925
  • 2
  • 11
  • 31
  • Really the `UpdateMovement();` call should be pulled out to this and the `Update` method called something more appropriate. Then you end up with `public void Update() {UpdateMovement();UpdateAction();UpdatePhysics();}` – Lithium Jun 14 '16 at 12:21
  • The code was just an example - never used anywhere. – cubrman Jun 14 '16 at 12:23
  • Joe, Although the answer looks brilliant and should be exaclty the one I am looking for, it has a significant drawback: what if I need to access local variables from Update() in AfterUpdate? I can do it in my solution, I cannot do it in yours. – cubrman Jun 14 '16 at 12:28
  • Joe, I am not being picky, just I really need to decide for myself whether my coding pattern is the only best and easiest solution for the absolute uncertanty. I HAVE an answer to my question - I provided it in the question itself. I am searching for something 100% better. – cubrman Jun 14 '16 at 12:30
  • 2
    Your suggestion doesn't meets the requirements of **"be sure that the line I have written is always called 100% of the time once my method ENDS"**. In case of an exception, the `AfterUpdate` won't be executed here (in comparison of a `finally`) – Panda Jun 14 '16 at 13:37
  • I took the requirements that 'my method ends' means that it executed all code without exception. As opposed to 'when execution of the method ends'. I consider an exception an interruption of execution this preventing the method from ending at all. Just a matter of semantics that would need to be cleared up. – Joe C Jun 14 '16 at 13:42
  • To be honest, @Panda brought up a very important point for anyone who decides to go down this road (and judging by the answers in this topic there are quite a few). – cubrman Jun 15 '16 at 06:32
6

Using a try/finally block should work;

    public void Update()
    {
        try
        {
            UpdateMovement();


            if (IsIncapacitated)
                return;

            if (IsInventoryOpened)
            {
                UpdateInventory();
                return;
            }

            if (Input.HasAction(Actions.Fire))
            {
                Fire();
                return;
            }
            else if (Input.HasAction(Actions.Move))
            {
                Move(Input.Axis);
                return;
            }
        }
        finally
        {
            //this will run, no matter what the return value
        }
    }

The performance costs of using try/finally (not try/catch!) are minimal

You cannot use return in the finally block;

If you were able to return a different value from the Finally block, this value would always be returned, whatever the outcome of the instructions above. It just wouldn't make sense..

Community
  • 1
  • 1
mark_h
  • 5,233
  • 4
  • 36
  • 52
  • I hope that people that upvote this understand the performance side of try/catch as well as their interaction with Visual Studio. Sure this is a solution, but not a solution that would work for uncertain projects where you aim for the fastest development using all of the modern tools to do it. – cubrman Jun 14 '16 at 12:50
  • 1
    Hey thanks for the link, mark_h. It's actually quite helpful. I did not know that 'catch' can be skipped. However, after trying your solution I found one important disadvantage: the code in the 'finally' block cannot have it's own returns. Basically this means that you can be as free as you want placing returns in the original method, but once you want to extend it, you immediately loose the freedom you once had :). I would call this a situational solution and I would highly suggest you add this information into your answer as I think it is quite important, given the nature of the question. – cubrman Jun 15 '16 at 06:52
3

I suggest wrapping the code into try..finally block:

  public void Update() {
    try {
      ...
      // you can return  
      if (someCondition)
        return;
      ...
      // throw exceptions
      if (someOtherCondition)
        throw... 
      ...
    }
    finally {
      // However, finally will be called rain or shine
    }
  } 
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
  • 1
    wow, didn't knew, after years of programming, that finally even gets called after a return statement. Thanks. – M. Schena Jun 14 '16 at 12:24
  • Sorry, but that is just an overkill - I discussed it in comments to the original question. – cubrman Jun 14 '16 at 12:34
2

You can use try-catch-finally (C#-Reference) without a catch block.

try
{
    //your business logic here
}
finally
{
    //will be called anytime if you leave the try block
    // i.e. if you use a return or a throw statement in the try block
}
Maximilian Ast
  • 3,369
  • 12
  • 36
  • 47
  • See comments to the original question. – cubrman Jun 14 '16 at 12:34
  • @cubrman, there is a difference between `Try/Catch` and `Try/FInally`. Yes `Try/Catch`-Blocks have performance downsides, but if you use them without a catch block there are minimal performance costs as @mark_h posted a link to a post about it in his [answer](http://stackoverflow.com/a/37811861/1466583) – Maximilian Ast Jun 14 '16 at 13:07
  • Maximilian, that was my bad - I had no idea you can skip 'catch' alongside it's performance cost. However, once i tried this approach in practice I realized a significant drowback: you can no longer use 'return'-s in the 'finally' block and that limitation actually negates all benefits the solution brings, as the nature of the question imples free, continuous iteration on a single method. – cubrman Jun 15 '16 at 06:56
  • Maximilian, basically with this solution I am free to do what I want the first time I write the method, but once I want to extend it (even once!) I am back to nesting :). – cubrman Jun 15 '16 at 06:58
  • I see. There is a reason why a `return` statement can't be called in the `finally` block. I.e. if you call `return` in the `try` block, the `return` in the `finally` block would be the 2nd return. So it isn't supported in C#. – Maximilian Ast Jun 15 '16 at 08:09
1

With the modern c# 8 syntax you may introduce some disposable 'ScopeFinalizer' object or name whatever you want:

public class ScopeFinalizer : IDisposable
{
    private Action delayedFinalization;
    public ScopeFinalizer(Action delayedFinalization)
    {
        this.delayedFinalization = delayedFinalization ?? throw new ArgumentNullException(nameof(delayedFinalization));
    }

    public void Dispose()
    {
        delayedFinalization();
    }
}

//usage example
public async Task<bool> DoWorkAsyncShowingProgress()
{
    ShowActivityIndicator();
    using var _ = new ScopeFinalizer(() =>
    {
        // --> Do work you need at enclosure scope leaving <--
        HideActivityIndicator();
    });
    var result = await DoWorkAsync();
    HandleResult(result);
    //etc ...
    return true;
}

Useful link: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-8#using-declarations

Igor B.
  • 2,219
  • 13
  • 17
0

Don't use the returns as it makes your code smelly.

public void Update()
{
    UpdateMovement();


    if (IsIncapacitated){
        return;
    }
    if (IsInventoryOpened)
    {
        UpdateInventory();
    }
    else if (Input.HasAction(Actions.Fire))
    {
        Fire();
    }
    else if (Input.HasAction(Actions.Move))
    {
        Move(Input.Axis);
    }
}

Also, your second solution has too much nesting, also confusing and smelly.

Timothy Stepanski
  • 1,186
  • 7
  • 21
  • In you answer you both: use returns AND have nesting, what is the point? Nesting is bad, that is why I am here asking my question. – cubrman Jun 14 '16 at 12:33
  • I didn't use returns redundantly, also, I didn't nest any if statements. – Timothy Stepanski Jun 14 '16 at 12:53
  • Yes, my bad, there is no nesting, but I would rather live with nesting than use returns anywhere in my method (but in the very end if needed). – cubrman Jun 14 '16 at 12:55
0

A problem with the current approach is that it requires changes to the Update() method whenever we want to add a new action.

Another approach is to remove the hard-coding of the update actions and configure the class with a set of update actions.

From the code given here we have

  1. Actions that always happen (e.g. UpdateMovement)
  2. Actions that happen if a test is passed (e.g. UpdateInventory)
  3. Actions that cause a return if they are executed (e.g. Fire())

We can encapsulate these in an interface

public interface IUpdateAction
{
    bool ShouldUpdate();

    // return true if we want this to be the last action to be executed
    bool Update();
}

and wrap various actions and decisions in the class using

public class DelegateUpdateAction : IUpdateAction
{
    private Func<bool> _updateAction;
    private Func<bool> _shouldUpdateCheck;

    public DelegateUpdateAction(Action action, bool isLastAction = false, Func<bool> shouldUpdateCheck = null)
        : this(() =>
        {
            action();
            return isLastAction;
        },
        shouldUpdateCheck)
    { }

    public DelegateUpdateAction(Func<bool> updateAction, Func<bool> shouldUpdateCheck = null)
    {
        if(updateAction == null)
        {
            throw new ArgumentNullException("updateAction");
        }
        _updateAction = updateAction;
        _shouldUpdateCheck = shouldUpdateCheck ?? (() => true); 
    }

    public bool ShouldUpdate()
    {
        return _shouldUpdateCheck();
    }

    public bool Update()
    {
        return _updateAction();
    }
}

To replicate the example we could use

public class Actor
{
    private IEnumerable<IUpdateAction> _updateActions;

    public Actor(){
        _updateActions = new List<IUpdateAction>{
            new DelegateUpdateAction((Action)UpdateMovement),
            new DelegateUpdateAction((()=>{ }), true, () => IsIncapacitated),
            new DelegateUpdateAction((Action)UpdateInventory, true, () => IsInventoryOpened),
            new DelegateUpdateAction((Action)Fire, true, () => Input.HasAction(Actions.Fire)),
            new DelegateUpdateAction(() => Move(Input.Axis), true, () => Input.HasAction(Actions.Move))
        };
    }

    private Input Input { get; set; }

    public void Update()
    {
        foreach(var action in _updateActions)
        {
            if (action.ShouldUpdate())
            {
                if (action.Update())
                    break;
            }
        }
    }

    #region Actions

    private bool IsIncapacitated { get; set; }
    private bool IsInventoryOpened { get; set; }

    private void UpdateMovement()
    {
    }

    private void UpdateInventory()
    {
    }

    private void Fire()
    {
    }

    private void Move(string axis)
    {
    }

    #endregion
}

The actions are executed in the order in which they are registered, so this gives us the ability to inject a new action into the execution sequence at any point.

  • UpdateMovement() always happens and doesn't return
  • IsIncapacitated() is a test with a null action. It returns if executed so we get our 'do-nothing-else-if-incapacitated' behaviour
  • UpdatedInventory() occurs if the inventory is open and then returns
  • Each of the HasAction checks return if executed.

Note If I have read the question better before writing the code I would have reversed the defaults as most actions seem to be 'return if executed'.

If we need to add 'UpdatePhysics()', we add a method to the class and add an entry in the appropriate place in the list of update actions. No changes to the Update method.

If we have derived classes with different actions we can add the facility to add (or remove) actions in the derived classes and either inherit and modify the default actions or replace them with a different set.

AlanT
  • 3,627
  • 20
  • 28
  • Thanks for a very detailed answer, but I am searching for a lightweight approach to use in every method i write. I brought an example with Update(), because it was the first thing that came to my head, I am afraid you gave a solution to a very differnent problem. – cubrman Jun 15 '16 at 06:30
0

After seeing the other solutions I can't think of a truly dynamic solution that has only the functions you want to call in the update loop.

Here are some ideas though I doubt any of them are better than making a good design. Joe C has the correct idea of how you should structure this kind of thing.


You could make a container of actions that need to be performed each update loop. Remove and add specific actions depending on the changes to circumstances. Such as a IsNowIncapacitated event that remove the Handling action from the list. Though I have little experience with actions, I believe you can set up delegates that the actions point to. Not sure what the cost to performance is.

A temporary thing you could do so you can keep adding logic is have your return statements return a void function with some constant logic you want performed, though all it really will do is separate your update code between two methods. It is not very neat or as efficient as structuring your code appropriately like in Joe C's example.

public void PostUpdate()
{
    //stuff that always happens
    PhysicsUpdate();
}

 public void Update()
{
    UpdateMovement();

    if (IsIncapacitated)
        return PostUpdate();

    if (IsInventoryOpened)
    {
        UpdateInventory();
        return PostUpdate();
    }
}
Jakey113G
  • 168
  • 1
  • 9