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.

Sign up for GitHub

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

Chikus sniffer revision #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Chikus sniffer revision #12

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d34309
Chikus sniffer revision
Chikus Oct 27, 2019
5187d7a
Chikus sniffer 2
Chikus Oct 31, 2019
27e090b
Chikus sniffer 3
Chikus Nov 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions submissions/Chikus/port-sniffer/sniffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
const Net = require('net');
var host = '';
var lower_port = 0;
var higher_port = 65535;
var port_list = [];
var timeout = 300;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use var if you are not looking for exactly function-wide scope. It's better to be consistent and use let instead. Or maybe try to reduce amount of mutable global variables by passing them as arguments from function-to-function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and thanks


function check(host, port, callback) {
var socket = Net.createConnection(port, host);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to wrap this function in promise and print to console from caller function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In progress

var timer = setTimeout(function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

socket has built-in api for timeout https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

socket.destroy();
callback(false);
}, timeout);
socket.once('connect', function () {
clearTimeout(timer);
socket.destroy();
process.stdout.write('.');
port_list.push(port);
callback(true);
});
socket.on('error', function () {
clearTimeout(timer);
callback(false);
});
}

function run_ports(){
check(host,lower_port, function next_check(result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above try to use Promises chaining instead of callbacks with recursion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but can you tell me more about why this is better??

Copy link
Member

@lempiy lempiy Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Callbacks usage may lead to "callback-hell".
    https://medium.com/codebuddies/getting-to-know-asynchronous-javascript-callbacks-promises-and-async-await-17e0673281ee

  2. In your example you have "circle" references, which, in some cases, may lead to memory leaks like this:
    leak.png
    Of course, it may not be the case, but I would advice, to avoid such kind of "double self referenced recursion". Especially, if some one else (other dev) may add something to your code.

if (lower_port == higher_port) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its better to always use === to prevent assertion bugs

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, Thanks!

if (port_list.length) {
process.stdout.write('\n'+port_list.join()+' ports are opened \n');
}
else {
process.stdout.write('No ports were found \n');
}
}
else {
check(host,++lower_port,next_check);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double self-referencing (check, next_check) is not obvious and a bit dangerous code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will change this, but what you are saying is that, recursion with the same function is not a good practice even if I use a call back, I think after I destroy the socket there is nothing behind I start with the new function. but no problem consider this done. Thanks :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying you have leak here, I'm just emphasising that it's easier to make mistake in such code.

}
})
};

switch (process.argv[2]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use global instructions in your code - wrap them into main function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and can you tell me why avoid to use them? thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Modularize — one function per task
    https://www.w3.org/wiki/JavaScript_best_practices
  2. It makes your variables scope more clear and may prevent some unexpected variables shadowing.
  3. In this case you have only one module, but in more complex apps you may have many other modules. Your code outside functions will be executed upon require/import, which is hard to track in big apps.

case '--ports':
var ports = [2];
ports = process.argv[3].split('-');
lower_port = ports[0];
higher_port = ports[1];
Copy link
Member

@lempiy lempiy Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const [lower_port, higher_port] = process.argv[3].split('-');

Also check for errors (string with no '-', non-number values)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

if (process.argv[4] == '--host') {
host = process.argv[5];
process.stdout.write('Please verify your port range and your host name , for performance you can modify the variable timeout when you be in localhost, one suggestion use ping to know which will be the ideal timeout\n');
run_ports();
}
else {
process.stdout.write('Please type node sniffer.js --help to know the usage \n');
}
break;
case '--host':
host = process.argv[3];
process.stdout.write('Please verify your port range and your host name, for performance you can modify the variable timeout when you be in localhost, one suggestion use ping to know which will be the ideal timeout \n');
process.stdout.write('THanks you dont provide ports u will scan from 0 to 65535 \n');
run_ports();
break;
case '--help':
process.stdout.write('Please go to this website to read how to use this program \n https://github.com/kottans/backend/blob/master/tasks/network.md \n');
break;
default:
process.stdout.write('Please type node sniffer.js --help to know the usage \n');
}