Closed Lioness100 closed 3 years ago
thanks a lot for this detailed review, sorry i didnt saw this before, btw i am working on the newer version and i will follow make the required changes , and again thank you very much, it will be really helpful for me
btw sadly when i made this i didnt new about import( )
so yea the loading command and event this is totally shit, but i made some changes also the ...args
thing is added sorry for that weird shit
well luckily i recently worked on this repo, So i got some more idea about TS and did some imporvements
Just added the new version.
Looks a lot better! I added a few comments to the commit for my personal pointers (that you obviously don't have to pay any attention to if you don't want to).
I also recommend mentioning that users shouldn't npm run dev
to run their bot in prod in the README, as ts-node
is pretty bad performance-wise in prod and should only be used while actively developing. Instead, opt for npm run build && npm start
Hello! I was looking through this repository (thanks for making it, btw) and saw a few bits of code I thought could be improved. All of this is my opinion, so I just wanted to bring it up and submit a pull request if you agree on some of the fixes I propose.
tsconfig.json
Typescript projects cannot build without a
tsconfig.json
. Typescript can make one for you if you doesnloadtypescript
globally and runtsc --init
, but I recommend you put in some research about it yourself.Dev Dependencies
First of all,
axios
provides it's own types, so you should uninstall@types/axios
. Next,typescript
should bedevDependencies
, notdependencies
because neither are used in the code itself. Furthermore, you usets-node
in thedev
script, but that isn't installed, and should also be added todevDependencies
. Lastly, you should create astart
and especially abuild
script usingnode
andtsc
(typescript compiler) respectively.Note that obviously just moving and adding lines around won't affect the dependencies- you'll need to
npm uninstall ...
andnpm i -D ...
individuallyDotenv
This is totally a personal preference, but note that instead of this:
You can just do this:
If you want to go even farther, you could omit
dotenv
from the code altogether, and simply add-r dotenv/config
before the.
in thestart
script. If you want to do this with ts-node too, you can replace thedev
script with:Import Order
Another totally subjective code styling preference, but you might want to adjust the import order to look nice and pretty :). Usually this means:
Non-default imports on top, sorted by verbosity Default imports in the middle, sorted by verbosity
(This would be repeated for
import type
statements, which would go on top of regular statements)Intents
This might be a conscious readability decision on your part, but why go through the process of importing
Intents
, creating a new bitfield, and adding the needed intents, when you can just put an array and d.js will do the rest for you?Typo
Event Subscription
Optimization
Doing this:
Is incredible unnecessary, verbose, and slow. Instead, just use
Object.entries()
which will provide you both the key and value at once (in a tuple)Scalability
Accepting the parameters
(a, b, c, d)
is a recipe for disaster. It's not reliable or scalable at all. What if you have an event that has five parameters? Maybe you could add ane
, but what if there were 15 parameters? 200?Instead use a rest parameter
(...args)
, which will account for every parameter passed to it (args
will be an array). Furthermore, if you want to be a bit fancier thanclient.on(event, (...args) => run(client, ...args))
, you can useFunction#bind()
.Scalability Part 2
It's going to be tedious to import and export each command and listener you make. Instead, consider reading the directory dynamically with an external package such as
klaw
, or the builtin package,fs
For the command handler:
You'll have to delete the
index.ts
files in the respective directories for this to work. Also, this would obviously make part 1 of this obsolete.Typing
Since we're dynamically requiring, none of the imported objects will have types. If you care about not having
any
s in your code, you'll want to export the command interface frombot.ts
and assign it tocommand
, and make a new event interface for events.Slash Command Registering
Don't use
// @ts-ignore: Object is possibly 'null'.
. There's already a feature for this. If you put a!
after a variable that is possiblynull
, ts will get off your back.But that doesn't matter, because:
Why are you using axios? You're using v13, d.js already has a function for this. You should uninstall
axios
and@types/axios
.But wait, this is also a bad idea. Sending potentially tens to hundreds of requests in parallel will absolutely get you ratelimited. Instead, you can do it all in one request:
Bam! Note that for both of these to work, you'll need to type the
slashCommands
array....useless: Array<any>
You use this a lot in your code and I don't understand why. You don't need it, at you're obviously not using it.
Naming Convention
Classes, such as
bot
, and interfaces, such ascommand
, should always be pascal-case.bot
->Bot
andcommand
->Command
bot
Categories
First of all, typo.
categoires
->categories
. Second of all, as I said, stuff like this doesn't scale. You should get the category list dynamically.Properties
You should follow the DRY formula and not type and assign properties. Just assign, and have typescript infer the type. (Also, never use
String
as a type. Always usestring
.Since your constructor is only calling
super
, you can omit it entirely (and remove theClientOptions
import.Command
interfaceThe
run
function needs a type more specific type thanFunction
. Also, you can export it, and then assign it to the command object in every command, which will remove the need to specify the argument types in every single function.message
will automatically be cast as aMessage
orCommandInteraction
(same idea forargs
) as long as you specify whether it's a slash command in the generic, so there will be no need to cast. You'll need to setcommands = Collection<string, Command>
tocommands = Collection<string, Command<boolean>>
All of the above can be repeated with events.
Useless
.map
This is absolutely useless. If you're adding all of one array's elements to an empty array, they're both going to have the same elements. What's the point?
Ending Notes
You really should add in eslint and prettier with the template