Replace methods that return strings

[hakusaro]

This was pointed out by @ProfessorXZ: a lot of the methods in GroupManager pass around strings. They shouldn't. All of those except ones that are actually useful as strings should be switched to return booleans for success/failure & throw exceptions for irregular behavior.

All of the methods that take an argument called "exceptions" should also be overloaded with better conventions (always use exceptions? duh?).

All methods overloaded/upgraded should be marked obsolete for removal in the next TShock maintenance release so that we can progressively upgrade the code quality of the codebase sooner rather than later.

[hakusaro]

I'm actually just going to work this.

[hakusaro]

There's a slight problem with trying to do this, actually. Lots of methods take proper signatures, e.g., String AddPermissions(String name, List<String> permissions) which means we can't override them with better methods -- the compiler obviously won't let two methods that have identical signatures exist because that's not overloading. So, we can't just create void/bool AddPermission(String name, List<String> permissions) because the signatures don't work.

In other words, if we want to make this work, we have to make either a breaking change on all of the methods or come up with some clever new signatures to work around the problem.

Further, this raises an interesting debate: do we use booleans to return status of whether or not something completed okay, or do we throw exceptions? As far as I know, the school of thought is that you shouldn't use exceptions for control flow, and this is one of those cases were we kinda do that, a lot. If we switch everything to booleans, though, we'd probably still have a reason to use exceptions in a lot of cases, but it would be much more inline with what we want to do.

[hakusaro]

Looking for thoughts on this @NyxStudios/tshock.

[DogooFalchion]

Can we consider going with unix style and have an DB.ErrorString and return an errorcode (0 being no error) as our returns. Only throw an exception for something server breaking(?).

[hakusaro]

It's certainly worth considering. I don't really know what the best practice/convention is for C#, tbh.

[QuiCM]

Returning an enum instead of an integer code would be better for readability and usability from my perspective

[hakusaro]

I agree with @WhiteXZ and @DogooFalchion on this one -- returning enums across the board is probably the best.

[DogooFalchion]

Perhaps we can start here and grab what we want to use: http://www-numi.fnal.gov/offline_software/srt_public_context/WebDocs/Errors/unix_system_errors.html

An extract from that:

define EPERM 1 /* Operation not permitted _/

define ENOENT 2 / No such file or directory /

define ESRCH 3 / No such process /

define EINTR 4 / Interrupted system call /

define EIO 5 / I/O error /

define ENXIO 6 /_ No such device or address */

[QuiCM]

This will be taken care of in the various rewrite projects.

Closing