zturtleman / mm3d

Maverick Model 3D is a 3D model editor and animator for games.
https://clover.moe/mm3d
GNU General Public License v2.0
114 stars 22 forks source link

Error masking defensive programming? #117

Open m-7761 opened 4 years ago

m-7761 commented 4 years ago

After some refactoring I think it best to get rid of as much of the Model API as possible... but it will be around until everyone agrees to drop it out.

It's mainly procedural... an odd choice for C++... and much of that seems to be so it can do "defensive programming" style checks. Problem is this kind of programming is usually an anti-pattern since it mainly serves to mask errors so they aren't immediately caught.

In practice it makes code look like it's not confident, and doesn't let STL bound-checking code do its job. Of course it's more to maintain too, and may not be implemented always.

So I'm basically wondering if it should be maintained... especially since there is a somewhat mature 3D modeling software that demonstrates most of the system works well, and so we should be more interested in finding parts of the software that isn't working correctly by removing these tests. Especially since call sites aren't generally testing the returned error status codes, which are usually just bool anyway.

Sorry for long winded description of this, but as long as I'm here I feel like I should at least explain why I think this is unhelpful. By removing this stuff it can be easier to move the deprecated APIs into the header with one-line definitions.

The definition files are way too busy. If refactored well a lot of the APIs should shrink down to one-line dispatches. I think too that Model itself should not be implementing anything other than getters. I feel the Undo system should execute most operations so we can shrink Model down and move subroutines into their respective Undo operators. The const getter logic should be simple enough in most cases to just implement encapsulation logic.