1

Sounds pretty basic, but I did not find an existing answer. Sorry if it's a duplicate.

I have never used assertions much in the past and I may not have understood the spirit behind them yet. Is it recommended or standard practice to write something like the following in method Remove()?

public class Model
{
    public Model ParentModel {get; private set}
    readonly List<Model> submodels = new List<Model>();

    public Model AddSubmodel(Model m)
    {
        submodels.Add(m);
        m.ParentModel = this;
    }

    public void RemoveSubmodel(Model m)
    {
        submodel.Remove(m);
        m.ParentModel = null;
    }

    public void Remove()
    {
        Debug.Assert(ParentModel != null);

        if (ParentModel != null) ParentModel.RemoveSubmodel(this);
    }

    // ...
}

The condition is something I am in control of (i.e. it doesn't depend on user interaction or something) and it determines the correctness of my code, so exceptions are out.

My rationale behind this is, I want it to fail in Debug mode, but I want to repair it as much as possible in Release mode.

Edit: as already mentioned in the comments,

The condition violation is not terribly fatal as such. If there is no parent, other than the the method call failing without the null check, nothing much will happen (in release build).

But what is more important, it indicates that the client of the class Model is doing something that is illogical. I want to be pointed to the fact (at least during debug) that there is something in my client code that I obviously haven't thought about, because if i had, the condition would have never happened.

oliver
  • 2,771
  • 15
  • 32
  • 1
    It´s not a duplicate, it´s just opinion-based and therefor there´s no right or whrong answer to it. It depends on your surrounding,l your code-style in your company and your personal preferences. – MakePeaceGreatAgain Mar 06 '18 at 07:37
  • 3
    If you can handle `null` then handle it. If you can retrieve value that is needed then do that. If you are 100% sure this can never be null, then `Debug.Assert` will allow you to easily debug your code. – FCin Mar 06 '18 at 07:37
  • @HimBromBeere: okay, but then the useful information for me is, that it is opinion based in your sense. Thought there was a quasi-standard about this topic, that every professional programmer obeys. (like most of those derived from OO for example) – oliver Mar 06 '18 at 07:41
  • 1
    `Debug.Assert` shows a MessageBox if the condition is not met, it is not the same `Assert` as in Unit-Tests. Is that what you want? – Manfred Radlwimmer Mar 06 '18 at 07:45
  • @Manfred Radlwimmer: I don't even know how Assert is used in unit tests, I'm afraid. I want it as a helper for surfacing bugs as early as possible. If I didn't have the assertion in the code, then the bug would be swept under the rug already in debug mode. – oliver Mar 06 '18 at 07:48
  • 1
    @oliver In short, this `Assert` shows a stacktrace depending on the passed condition, the other one throws an exception so your code never executes with unexpected/undesired parameters. – Manfred Radlwimmer Mar 06 '18 at 07:50
  • "I want to repair it as much as possible in Release mode" - better not do this. You said `ParentModel` cannot be null, so if so happens it is null in release mode - you have a bug, and not even in this place but some logical bug somewhere else. This bug will be hidden by your null check and so might be hard to diagnose in time. So either remove null check (and let `NullReferenceException` happen in case of bug), or leave it and throw appropriate exception if parent is null. – Evk Mar 06 '18 at 08:03
  • @Evk: absolutely true, but the alternative would be that the program crashes also in release mode. All data, probably hours of work, are lost then, whereas if I "try to repair it" the data might still be usable. – oliver Mar 06 '18 at 08:10
  • 1
    You should probably emit a log entry or something though. That's the kind of bug that could drive a person mad. – John Wu Mar 06 '18 at 08:34

1 Answers1

2

Assertions are used when some really erroneous condition is true. It is so wrong that an exception is not enough. It is usually use to detect logic errors in your code.

Whether or not to use it in your specific example depends. Are you 100% sure that ParentModel should be non-null, and that if it is null, it implies that something really unexpected and wrong has happened and you must stop the program? If yes, then you can use Debug.Assert. Otherwise, do a null-check and if it is null, handle it by some other way than terminating the program.

Also note that the assert won't work in Release mode. To assert in Release mode, use Trace.Assert.

Learn more here and here.

Sweeper
  • 213,210
  • 22
  • 193
  • 313
  • It is like you say, it should never happen. Objects of type Model are to be displayed in a view and then they always have a parent (except for the root of the tree, of course). So, If the user decides that a model should be removed from the view, it has a parent. They may exist without a parent temporarily during handling of user events, but then my other code knows that and is responsible for not trying to remove what has been removed already. – oliver Mar 06 '18 at 07:58
  • @oliver The question is more focused on "is it fatal if that happens?" instead of "will it ever happen?". In this case, at least from what I feel from your description, it's not fatal if the parent is null. This means that you can just do `ParentModel?.RemoveModel(this);` without the assert. – Sweeper Mar 06 '18 at 08:04
  • It is not fatal as such, if there is no parent, yes, but the method call will of course fail (in release build) if there is no null check before it. But what is more important, it indicates that the client of the class Model is doing something that is illogical (remember Spock? :-) ). I want to be pointed to the fact (at least during debug) that there is something in the client code that I obviously haven't thought about, because if i had, the condition would have never happened. – oliver Mar 06 '18 at 08:16