Would it be a good idea to replace the if statements with try-catch in the following usecases (performance and readability wise?):

Example 1

public static void AddInitializable(GameObject initializable)
{
    if(!HasInstance)
    { // this should only happen if I have forgotten to instantiate the GameManager manually
        Debug.LogWarning("GameManager not found.");
        return;
    }

    instance.initializables.Add(initializable);
    initializable.SetActive(false);
}

public static void AddInitializable2(GameObject initializable)
{
    try
    {
        instance.initializables.Add(initializable);
        initializable.SetActive(false);
    }
    catch
    {
        Debug.LogWarning("GameManager not found.");
    }
}

Example 2

public static void Init(int v)
{
    if(!HasInstance)
    {// this should happen only once
        instance = this;
    }

    instance.alj = v;
}

public static void Init2(int v)
{
    try
    {
        instance.alj = v;
    }
    catch
    {
        instance = this;
        Init(v);
    }
}

Edit:

Question 2: How many Exceptions can I get to be still performance positive?

CodePudding user response：

It depends.

Try-blocks are generally cheap, so when the exception is not thrown, that would be an acceptable solution. But: In your case, if the condition is not satisfied (meaning the thing was not initialized before that method was called), this is a programming error, not something that should ever happen in the finished program. It is perfectly valid that such errors crash the program. Makes spotting the bugs and fixing them much easier in development, and avoids that you silently hide it (in example 1, you silently don't do anything, which might cause confusing behavior later).

So: If it would be a programming error, don't use an exception handler, nor a test (except maybe an Assert). Just let the program crash (with a NullReferenceException in this case).

CodePudding user response：

To my point of view, this is not a good idea.

We usually use try catch when we know what kind of exceptions will appear in the context, and hence a catch without exception type is not a good practice.

In your scenario, since you already know the only problem is that the property HasInstance may be false, you could directly check it with if statement. Using try catch seems meaningless here, although it works. This seems like you are expecting an error, and you just ignore that error because its message does not matter.

Besides, I see you are using Unity and is creating a singleton GameManager, and actually I think the singleton pattern here might not be quite correct.

For example, if you use the code like this, actually there is virtually no possibility it does not have an instance if you treat your scene and gameobjects properly :)

CodePudding user response：

So you're kind of along the right lines here.

Unless you are in dire need of increasing performance, don't try to optimize, and if you do need to optimize, make sure you're doing it right (exceptions are more expensive that if statements, especially if you know they're going to happen)

The first example you've given, I can kind of get behind. You're making the assumption that something was initialized, and if it turns out it wasn't, throw an error. You're logging it, it's ok, you initialize it and you'll probably never have to worry about that exception again.

The second example you've given is a big no no. You should not use exceptions to fall into other logic in your application. Instead, in the Init() method, just always have the line 'instance = this', don't do the if statement. Once you know it's initialized, there should never be a reason for it to throw an exception when used.

Of course, don't go crazy with this, exceptions should only be used for exceptional circumstances. If you write your code and are thinking 'Hmm, so it could be either A scenario or B scenario, and in B scenario I want this to happen, so I'll throw an exception' that's completely the wrong line of thinking. Instead it should be 'Hmm, so all this will happen, but just in case something breaks, I'll put it in a try catch and log it, as who knows, I'm not infallible'

You can see how I've applied the above logic to your two examples,

CodePudding user response：

I would agree with PMF: Depends on your specific use case and whether something is your fault or something you can't control / predict.

So in general I'd say there are three ways of how to handle stuff that isn't behaving as expected

• A) let throw an exception to indicate that this is really bad and there is no way to recover => and most probably crash your app

  This usually makes totally sense on development time because while debugging you explicitly want your app to crash so you can find and fix the issue.

  This should be happening for everything where the cause is basically something that you messed up and can be fixed by you. (In your case instance not initialized correctly)

• B) return something e.g. false to indicate that something went bad but allow this to be handled by the code and e.g. try something else.

  In my eyes this should be the preferred way of dealing with stuff you can't control yourself like e.g. user input and other unpredictable conditions like internet connectivity etc.

• C) Just ignore it and do nothing at all.

  Depends of course on what exactly you are doing but this should actually happen almost never. For a User this can be extremely frustrating and also for you as developer it makes debugging hard to impossible!

  In combination with B of course this is valid since something else will already have delt with the issue.

And to add just in general unless you work on some core and reused library I would actually never throw exceptions myself except you are re-throwing caught ones to add additional debugging information.

Now all three options can be achieved by try - catch or if checks internally of course and it depends on your specific case which way you want to go.

Exceptions are quite expensive! So performance wise I would say use if wherever possible.

There are cases though - and in my opinion these are the only ones where try - catch would be allowed - where you use a library and there simply is no way to prevent an exception.

Example: FileIO

• the file you want to access does not exist

  -> You don't need try - catch for this (in my eyes it would be the lazy way). This is something you can and should actually check first if(!File.Exists(...)) so your program can correctly deal with it and handle that case (e.g. you might want to tell the user instead of simply crash or doing nothing).

• The file is currently opened by another program so you can't write to it.

  -> There is no way to find this out beforehand. You will get an exception and can't avoid it. Here you want to try - catch in order to still allow your code to deal with such case (as before e.g. tell the user instead of simply crash).

But then how you actually deal with them again depends:

• If you e.g. use some hardcoded paths and these files definitely should be there -> Exception because it means you as developer messed something up.

• If the path comes from user input -> Catch because this is something you as developer can't control but don't just want your app to crash, rather show a hint to the user that he messed it up.

Now in your use case the Example 1 both of your solutions seem pretty bad to me. You go with the last option C and just ignore the call - a user won't see the warning and also a developer might just not note / ignore it.

You definitely want to get an Exception here if this means that your app will not behave correctly and not catch it at all!

In general there is no need for a special bool flag. I would rather go with

if(instance == null)
{
    Debug.LogError(...);
    return;
}

Because this is most probably a more severe error not only a warning so it at least gains visibility.

In your Example 2 you actually have kind of a lazy initialization anyway so either way the call itself is basically valid.

In such case though again this is something you can easily check and I would not wait for an exception (especially not simply any) because I already know that there definitely will be one at least once.

In my opinion this should rather be

if(instance == null)
{
    // I have put `???` because, well, in a "static" method there is no "this" so 
    // I wonder where the instance should come from in that case ;)
    instance = ???;
}

instance.alj = v;