Defensive programming

[XanHou]

While looking through the code and trying to give methods concrete contracts in javadoc I noticed that there are a lot of missing checks. For example. When starting a GameEngine you give an InitialState. After looking at the code I noticed that an if this state is null, it will result in a crash without a decent error message.
So I have two questions.
1. Do people approve if I add simple checks that check if the contract on on the state and parameters of a method are followed and throw a RuntimeException* if the contract of the method is broken? Ofcourse I also write an error to the logger before throwing the exception!
2. Is it ok to add the "assert" statement in private methods or methods that occur very often instead of throwing an exception?

* Not a RuntimeException itself, but various sub classes ofcourse! For example an IlligalArgumentException in the case of a null parameter where not allowed.

 

[Immortius]

Hmm...

Firstly, I would suggest using Guava's Preconditions as it should keep the checks terse. I would want to avoid large amounts of code bloat for this.

Secondly, don't log and throw exceptions. Thrown exceptions are always logged anyway, so you'll just end up logging things twice. Log or throw. Also consider whether the contract can be loosened as well - if there is a sensible action in the presence of null, do that instead. Keep in mind that as we're designing for modding and that involves coders of all skill levels, having Terasology continuing to work in the presence of error is desirable (Unreal did this best with UnrealScript, as it would log an error and continue on the next line of code for NPEs).

Thirdly, I don't feel private methods need preconditions. Preconditions alert developers of misuse of a public API. Unit tests should be used to exercise the internal workings of a class instead.

Fourthly, assert... is probably wrong for preconditions. Assert should only be used where something should never happen - being called with illegal arguments is absolutely something that can happen. They are more often used to check post conditions. Again I would caution against code bloat - unless it helps explain an expected result.

Finally... I would hold off on all of this for the moment. I am not going to be merging scattered javadoc and preconditions into multiplayer.

 

[XanHou]

Thanks for the fast reply.

1. I did not know of the existance of these precondition methods. Sounds indeed like a much better option.
2. Hmm I should indeed not log and throw. But with regards to modding: I much rather have a proper error message then a bug that slips past all checks and causes chaos. Not doing something can often be more frustrating than giving an error at the right place. At least logging the stacktrace is then required. Is that done by default somehow, should I just use Thread.dumpStack(), should I add it to the message in the logger or should I add a throwable to the logger?
3. Not only an external piece of code can cause a bug! It is a very good practise to let private methods also have a contract. This also makes reading the code a lot easier.
4. That is why I made it a 2e question. Some people love the assert statement because it is compact and can be turned of. However, I agree that there are very good reasons not to use it too often. So I won't use it for these checks then.
5. Good point.

 

[Immortius]

On 2. I definitely agree it should log. Throwing an exception is probably ok, it will get caught at a higher level in most places and the stacktrace will be logged - if it doesn't we need to add better catching (I think exceptions in UI code aren't caught early enough at the moment, which needs to be fixed).

On 3. To clarify, I'm not saying private methods cannot cause bugs. What I mean is they aren't going to be called outside of their class, so preconditions on them have much less value - you would be protected them from misuse from a small number of invocations within the same class, or rather the occasional work by a programmer to that class. This is opposed to public methods which have potential infinite callers and may be touched by any coder at any time. I believe the most readable code is reasonable short with well named variables and submethod calls, and I believe that unit tests offer more value in this space (I believe unit tests are extremely valuable in general).