Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider renaming local variable in 'src/Server.ts' #180

Closed
Soupertonic opened this issue Nov 5, 2020 · 0 comments
Closed

Consider renaming local variable in 'src/Server.ts' #180

Soupertonic opened this issue Nov 5, 2020 · 0 comments

Comments

@Soupertonic
Copy link

Soupertonic commented Nov 5, 2020

I was having a look at the source and noticed this ambiguous named local variable. It's exitEvents at line 22.

let exitEvents = ['SIGINT', 'SIGUSR1', 'SIGUSR2', 'SIGTERM'];

It's being used in a for-loop to declare a listener that """kills""" (this is a bad name choice too, just for the record) the server if any of the declared "exit events" occurred.

let exitEvents = ['SIGINT', 'SIGUSR1', 'SIGUSR2', 'SIGTERM'];
for (let event of exitEvents) {
process.on(event, () => {
Server.kill();
});
}

While I'm pretty okay with exitEvents, I still have to complain and would like to suggest something better. "exit events" could mean anything. For example if the server happens to decode something invalid and crashes... that could be considered an exit event. But an exit event isn't the same as an interrupt signal. Therefore I'd like to suggest to you to change the variable name to interruptSignals and the for-loop to for (let interruptSignal of interruptSignals).

process.on(interruptSignal, ...) reads alot better than process.on(event, ...) because interrupt signals are closely related to processes and will be understood in an instant.

Here are the changes for copy-paste:

let interruptSignals = ['SIGINT', 'SIGUSR1', 'SIGUSR2', 'SIGTERM'];
for (let interruptSignal of interruptSignals) {
    process.on(interruptSignal, () => {
        Server.kill();
    });
}

Also, optionally, you could inline the array in the for-loop:

for (let interruptSignal of ['SIGINT', 'SIGUSR1', 'SIGUSR2', 'SIGTERM']) {
    process.on(interruptSignal, () => {
        Server.kill();
    });
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant