Feature Freeze / 1.3 Code Cleanup / Defeat the Kishin

[hakusaro]

This started as an idea I presented in Slack. Here's the basic premise of what I want to do before 1.3 hits:

1. Improve the codebase significantly using best practices.
2. Improve the documentation of the codebase, so that we generate zero warnings on compiling TShockAPI.csproj (this is not the API, so it should be possible).
3. Update the wiki with any changes we make in doing so.

In order to accomplish this, we're going to need some effort to list out some code quality targets that we want to meet, aside from the goals above. Some preliminary targets:

• AnythingManager - There are a lot of "Manager" classes that do a variety of database calls and use classes like "Group" and "User" as simple datastores. These classes are basically assorted SQL functions running and executing, returning errors as strings or throwing exceptions depending on the implementation date at will.
• Many of TShock's hooks - There are a lot of times when TShock hooks the API, then does around 20 different, completely isolated things inside the hooks. For example, when do we setup the config file? When do we initialize the log file? Those are all stuck inside similar giant hook objects.

Lastly, we need to put a freeze on the creation of new feature sets. Right now, this is planned to start after new-permissions is merged in by @Enerdy. In other words: no new functionality that is added on top of existing layers for the sake of adding new functionality. Log file size splitting, for example, would be nice, but should only be added if it comes as a side benefit from switching to Log4net during the code cleanup. New commands should also be avoided, while things like the config file are fine to modify (because simplifying or modifying subsystems might grant the ability to turn them off or modify them). This will be lifted once this issue is closed.

[hakusaro]

Please report in if you want tasks assigned to you for the cleanup. If you present an issue and want to tackle it, you get that task instead of getting one randomly assigned. cc. @NyxStudios/tshock-contributors @NyxStudios/people

[QuiCM]

Some things to decide upon so we have 1 style in the codebase:

• use var or type. E.g., var player = args.Player or TSPlayer player = args.Player
• private/public/property naming. Re# suggests private _name public name Name {get;set;}. Re# is not god so we should go with whatever everyone is most comfortable with
• Documentation - We currently have some things documented in a ///Name - Description format and others in a ///Description format.
• Handling obsolete methods - There's no conflicts that I'm aware of at the moment, but I feel that our process of simply marking things obsolete could use some improvement
• if (...) { } else if { } else { DoStuff(); } or if (...) { return; } if (...) { return; } DoStuff();

There may be more. Things we decide upon should go into CONTRIBUTING.md so that contributors know what style they should be using

[hakusaro]

• Right, this is worth bringing up @WhiteXZ. @Enerdy and @Ijwu commented on that doc style, which is apparently not what the proper way to document things is. I was using XmlDocs for Sublime which doesn't exactly make it clear that part should be removed, so the convention we want to move to is <param>Description</param> rather than <param>Name - Description</param>.
• The public/private naming property thing was discussed long ago by bc when we first started using ReSharper, that is to say, we decided not to use the private _name format, so I've been sticking to that premise and using private Name instead of that. However, that being said, this goes against code style guidelines for C# as set by Microsoft, so I'd be in favor of a change that made it more standardized if we really want to move that direction.
• I'm fine with the current obsoletion system, though this does pose interesting issues for this code cleanup. It means that a lot of shim methods will be in place during the release of TShock 5.0, and that deprecated methods won't be removed until a release after, but this isn't a big deal to me.
• I like both styles of if statements for different things, unfortunately, so I don't have much of an opinion on that.

[AxisKriel]

• We should make our code as easy to reas as possibly, which means always explicitly declare a type unless you're calling a constructor or anything where the type would be implicit. An example would be List<TSPlayer> = TShock.Utils.FindPlayer("test"); and var lines = new List<string>();
• I personally tend not to explicitly use private unless the method is also static (???). I'm weird like that. We should probably always use keywords though.
• The former documentation style has been adopted by @nicatronTg since he doesn't use an IDE. We decided to go with VS's styling, aka the latter, because it is cleaner.
• Honestly, whether to use if else or if return heavily depends on the case. If you overuse the latter you'll eventually get heavily indented statements, where the former might become a little redundant. I personally tend to use if return for small checks and encase the main code in if elses.

[hakusaro]

Oh yeah -- I definitely agree with what @Enerdy just said about implicit vs explicit typing. Easier to read is always better, and explicitly defining a type is the best way to do that.

@Enerdy fields are private by default. By not using the private keyword, you're still creating private fields that need to be documented somewhat unless they're explicitly set to protected, public, or internal.

[hakusaro]

Random thought: The fact that this snippet is so heavily copy pasted around in plugins makes me think we should have a more natural way to remove commands than TShockAPI.Commands.ChatCommands.RemoveAll(c2 => c2.Names.Select(s => s.ToLowerInvariant()).Intersect("rules")).Any();.

[Ijwu]

My comments for now, I'll likely be slow to follow up on this discussion in the future as I will be on my way overseas.

• Documentation style should be as <param name="Name">Description</param> for parameters.
• I prefer private _varName as is MS standard.
• I agree with @Enerdy as var should be used whenever the type is easily inferred by the programmer reading the code and an explicit declaration for return values and non-easily inferred types.
• We should definitely, always, put the access level down explicitly in my opinion.
• In the case of the if chains, they should be reasonable in the sense that they follow human logic. If you're pouring milk first you check if you have any milk, if not you abort.

if (!milk) 
{
    return;
    //go to the store and buy some milk
}
//continue pouring milk

Otherwise if you have a legitimate choice between multiple items for breakfast then you may want to chain it.

if (milk)
{
    //pour milk
{
else if (orangeJuice)
{
    //have OJ instead
}

It's most dependent on the individual situation and I'd say it's a case by case thing which each individual person can judge.

[tylerjwatson]

If I may chime in about access modifiers.

The use of private should only be used with good reason and when the class is sealed or the field is genuinely private, and I would advocate the use of the protected modifier if it is actually at all possible to derive from the classes in TShock.

Deriving from classes is a good way to extend functionality and by marking members as private without reason deliberately gimps the extensability of classes for those who need them. I'd admit the need to derive from things in TShock might not be pressing at the moment, alas whilst we're in the middle of a sweeping change it's far easier to change to gain a better OO design.

Random thought: The fact that this snippet is so heavily copy pasted around in plugins makes me think we should have a more natural way to remove commands than...

+1 million for this. Implementing threadsafe wrappers around the command delegate collections would be a huge benefit, at least to me, to solve the issue of manipulating commands at runtime, since I do quite a lot of it.

A few ideas:

• TSCommand GetCommand(string name) : returns the first name or alias match of a command
• TSCommand[] FindCommand(string name): returns a list of commands that match a wildcard
• TSCommand[] FindCommand(Expression<Func<TSCommand, bool>> predicate): returns a list of commands that match a predicate
• void DeleteCommand(string name) : Deletes a command by its name or alias
• void DeleteCommand(Expression<Func<TSCommand, bool>> predicate): Deletes commands based on a predicate, useful for sweeping deletes, etc.

Then the internal command collections may be hidden or marked internal to prevent modification to the collections whilst someone is executing a command delegate, causing an enumeration exception.

[QuiCM]

@tylerjwatson this was discussed in Slack and probably will not be implemented in TShock itself (as TShock is not meant to be an API) It will probably fall in under #912

[hakusaro]

There is no feature freeze on feature development / TShock until a version of TShock is generated and running on Orion.