Abstract a lot of code, clean up and improvments

[MrJacz]

This PR serves to abstract a lot of code, clean up and add some improvements

• [x] Rewrite command & event loading
• [x] Rewrite logger class

[MrJacz]

Requesting a review: @kyranet @Dev-Yukine @YorkAARGH I now feel this PR has done enough/served it purpose to rewrite/refactor/abstract/improve/clean the code, This has been a fun time, Doing something for a bot that doesn't use a framework is a little refreshing. I love writing my own things and not using framework methods, Misaki is a good example of how good a discord bot can be without a framework.

[MrJacz]

@kyranet

CommandStore, EventStore and many other things (Command and Event aswell) are very similar to Klasa's old code with the RAM optimization PR patch applied. Nonetheless it's under a license that is not included.

In fact, The EventStore was made by me. Originally it came from PenguBot where it had no loading stuff in the class but was the same (except it actually extended Base a class with some basic properties like client but nothing else) which I obviously removed that in this implementation but its really not a copy of Klasa's ways, Its inspired but it no way is it Klasa's code.

There's also a lot of things that are strongly inspired by Klasa, for example, the misakiReady event (from klasaReady), our stores and pieces (without the Store and Piece abstract classes).

Funny thing is, I did this before I knew klasa was a thing

A lot of our style is applied in this PR, in case @YorkAARGH isn't aware, this code is way much better than the previous, yes, but it's so similar to Klasa, porting it would literally take hours as the way we handle the pieces, stores, loading, initialising, reloading, loading... are all identical.

I'm quite sure York is aware but, The whole point of this PR was to make Misaki more understandable. That is very hard when you have A Client class in one file that handles the loading over commands & events with a directory scanning module (klaw) A couple of weeks ago I was whing about York using klaw you suggested the use of fs-nextra scan (Got to admit I had no clue on how to use this function) so me being me I looked at Klasa's store walk code (I knew it was used here) and used it. As I most likely would've messed up something if I didn't use a snippet so I did. But I really can't regret that as from the moment I say klaw in guidebot I disliked it so the use of a module we already have and use is put to use 🎉

Another thing that I highly dislike about this code is the amount of uncaught promises, there are many message.channel.send that aren't either awaited nor has a .catch callback. What does this mean? This leads to unhandled promise errors.

I can totally agree

Another thing that I have noticed is that this code also lacks of documentation. Reducing lines of code isn't always a good thing, if it's undocumented, it's hard to read and understand what's going on. While the old code (and GuideBot) are highly documented, Misaki's seems to get less and less documentation. This leads to the project being harder for an external contributor to understand what is the code doing.

Yeah I can also agree on this, I'll work on adding documentation in the future + JSDoc

I must include that I have talked privately with you about MisakiConsole being ripped off from KlasaConsole (from Klasa 0.5.0) with the difference that colours are rather... hardcoded. But the design is very similar to our KlasaConsole rewrite made by bdistin. (#227 in dirigeants/klasa). I have, however, kindly asked for our license to be put in the top of the file. Instead, you renamed a method, removed the shard support, and also removed the notice about the code being inspired.

Yes, we spoke, I don't see how this can really be a rip off honestly... Everything was off the top of my head except write, Now yes I had to look at snippets of code, no I didn't only look at klasa... I spent about 30 minutes reading a couple guides on the Console class & NodeJS stream's interesting, to say the least, but I really can't say this is a rip-off, I'd say write was inspired by KlasaConsole but that's it honestly

[bdistin]

There's also a lot of things that are strongly inspired by Klasa, for example, the misakiReady event (from klasaReady), our stores and pieces (without the Store and Piece abstract classes).

Funny thing is, I did this before I knew klasa was a thing

Funny thing is, that almost all of the code used here in EventStore/Event (and more) is line for line (variable name for variable name even), identical code to a mixture of current and past implementations of klasa's version and by extension klasa's Piece/Stores. A coincidence you don't get without copying.

[MrJacz]

@bdistin I've been looking at Event, Command pieces in klasa for the last 15 minutes, I can't find anything that even hints that them classes are a copy. Considering Command was a thing before I PRed that's a little hard to believe, the same variable names? So calling a variable name or file or client is copyright now? other then walkFiles the code is my own

[appellation]

I realize that I am not heavily involved in the development of Misaki, but york requested some code review so I will give it my best shot.

My main criticism of this project lies in ineffective application of objects and object prototypes. While I will not take the time to make a fully-fledged, line-by-line review of this PR, I can generalize my findings and hopefully provide some insight on how to better utilize Javascript. I realize that a lot of these issues existed prior to this PR, so don't take this as a criticism of only the OP.

1. Unnecessary instance properties/methods: while browsing the code in this PR, I noticed numerous classes that had instance properties that were constant or otherwise completely unrelated to the state of the class instance. Class properties and methods should be reserved for things that either rely on or modify the instance state. client.methods is a prime example of this, where every property is not reliant on the state of the client or any of it's properties. Most people define methods on the client for convenience, which is what I imagine happened here. However, it's is incredibly simple to just import the static util methods you require. Oftentimes, these properties/methods should in fact be defined on another prototype, which lends the same utility in a more idiomatic and accessible fashion.
   This is by far my most significant criticism: throughout every command there seems to be at least one access of some client property that is actually static. Besides being bad practice, you could see increased memory usage (from declaring multiple copies of the same static information) and bug occurrence (from inconsistent object mutation across instances). My suggestion would be to enable the eslint rule class-methods-use-this; while it is not a rule that should be followed religiously, it will help you discover where you can cleanup some code and will cause some pause for thought before implementing new methods.

2. Incorrect instance property/method location: stuff like message.settings is immediately an example of such a problem. A message cannot have settings, and so it's strange to see a property of them defined on the prototype. Again, this frequently occurs in an effort for convenience, but is in fact confusing and could easily be defined on a more appropriate prototype.

Basically, my suggestion is as follows. Don't be afraid to declare new classes as you need them; if you find yourself doing a lot of boilerplate to implement a feature, a class may be the thing for you. For example, reminders might be something to consider writing a simple class for: perhaps you could consider defining Member#reminders as a store of reminder classes. However, really think about why you're adding that particular instance property/method to that particular class and if it's not immediately related to what that class is about, perhaps you're better off finding another class (or making a new one).

Personally, I've found that defining a Command class (accepting a message in the constructor) with all your validation and command instance validation methods has been the most effective solution. This also leads to your command execution being more object-oriented and thence friendlier.

When looking at a method signature, oftentimes the prototype that it should be defined on is accepted as one of the first few parameters.

On a personal note, I believe this PR achieves very little in the way of significant change. Most of the work has been on the cleanup side of things, and calling this an abstraction is somewhat of a misnomer.

Other suggestions:

• The console dependency is entirely useless (and the internal console module is another example of point 1). If you want decent logging, I would recommend switching to an async logging library like winston rather than one that just checks for console methods.
• I noticed that you're throwing custom error classes, which is great error handling practice. However, there are some cases where you're using if (!condition) return message.response(undefined, 'blah');. I'm unsure what the difference is, and I would recommend switching everything to throw.
  • Please fix that method so you don't have to send undefined as the first parameter. It hurts.
• You should consider abstracting the argument system a little bit more. I see that you have awaitReply, but numerous places duplicated code in the argument resolution.

Edit: I didn't realize that the console module was native rather than from npm when writing this, so most of that comment is obsolete. There already exist similar logging modules, but writing your own is fine if you want to put the effort in.

[MrJacz]

@appellation you're better to file a issue as half of this is not related to this PR what so ever

[appellation]

This PR being titled "abstract code, cleanup, and improvements", I assumed that my comment was well within scope.