Chikus sniffer revision by Chikus · Pull Request #12 · kottans/nodejs-2019-homeworks

Chikus · 2019-10-27T11:28:15Z

Sniffer TCP UDP Network

lempiy

Nice job, now try to improve your code quality.

lempiy · 2019-10-28T07:59:17Z

+
+function check(host, port, callback) {
+ var socket = Net.createConnection(port, host);
+ var timer  = setTimeout(function () {


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

Done, thanks!

lempiy · 2019-10-28T08:00:30Z

+var timeout = 300;
+
+function check(host, port, callback) {
+ var socket = Net.createConnection(port, host);


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

In progress

lempiy · 2019-10-28T08:02:46Z

+}
+
+function run_ports(){
+  check(host,lower_port, function next_check(result) {


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

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

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

In your example you have "circle" references, which, in some cases, may lead to memory leaks like this:

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.

lempiy · 2019-10-28T08:05:52Z

+var lower_port = 0;
+var higher_port = 65535;
+var port_list = [];
+var timeout = 300;


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.

Done, and thanks

lempiy · 2019-10-28T08:09:38Z

+
+function run_ports(){
+  check(host,lower_port, function next_check(result) {
+    if (lower_port == higher_port) {


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

Done, Thanks!

lempiy · 2019-10-28T08:11:31Z

+  })
+};
+
+switch (process.argv[2]) {


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

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

Modularize — one function per task
https://www.w3.org/wiki/JavaScript_best_practices

It makes your variables scope more clear and may prevent some unexpected variables shadowing.

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.

lempiy · 2019-10-28T08:17:17Z

+      }
+    }
+    else {
+      check(host,++lower_port,next_check);


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

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

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

lempiy · 2019-10-28T14:46:19Z

+      }
+    }
+    else {
+      check(host,++lower_port,next_check);


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

lempiy · 2019-10-28T14:51:14Z

+    var ports = [2];
+    ports = process.argv[3].split('-');
+    lower_port = ports[0];
+    higher_port = ports[1];


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

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

OleksiyRudenko · 2019-10-30T07:24:47Z

@Chikus great job so far!
However linter reports multiple warnings and errors related to the code style.

Please check code quality guidelines and fix the style of your code.

lempiy

Looks much better for me 👍 , see more comments bellow.

lempiy · 2019-11-01T12:47:13Z

+        resolve(port);
+      });
+      socket.on('error', function fail() {
+        throw new Error('Error in Socket, wrong host \n');


Use reject for error handling.

lempiy · 2019-11-01T12:47:17Z

+    throw new Error('Ports no defined \n');
+  }
+
+  function checkHost(host) {


No need to declare this functions inside sniffer function. Move functions to module scope (checkPorts, runPorts, checkHost, checkServer).

if import the function sniffer, I will not hava access to this others functions, I just did that for comfort from the user he just execute the funcion, but Ok i will change the scope.

You won't import sniffer function and any other function here, since they are not exported.

lempiy · 2019-11-01T12:52:22Z

+    return null;
+  }
+
+  switch (arg[2]) {


I think it would be great to create parseArgument function, that will return an object with parsed args. parseArgument may be called from sniffer (root) function and depending on returned object we will decide how to call runPorts .
It will make you code more flexible and extendable.

lempiy · 2019-11-01T12:54:25Z

+
+  function checkHost(host) {
+    if (host) {
+      const domains = host.split('.');


Use dns node package to validate domain

const dns = require('dns').promises; async function checkHost(host) { try { return (await dns.resolve(host)).address; } catch (err) { throw new Error('Wrong host'); } }

Cool and Done

I used this function but I consider it is not necessary to add, for example if you set in argument --host 10.4 this function will not throw any error, it will fill with zeros the IP 10.0.0.4, also I had some issues with my node version, but when I maked it work, I see it is doing the same that net.socket does, so for this consideration I didn't implemented.

DNS is not the same as socket. Socket uses TCP connection or IPC streaming. DNS is domain resolve system built on top of UDP with fallback to TCP.
If you don't want to use it - it's ok, but your current host validation is incorrect and looks ugly. Here example of correct (RFC) regexp for hostname (https://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hostname-or-ip-address). As you see its pretty big, you may use this if you want or check for Invalid host error in socket.error event to break the loop on first connect to wrong host.

You are right, Socket doesnt do the same as DNS, but what I tried to said is that socket
= required('net') inside of socket they are doing DNS validation, You can checked in this repo. https://github.com/nodejs/node/blob/master/lib/net.js , other wise how socket can work when you provide www.google.com here im sure they first contact DNS to get the IP add.

Yes, it does. Generally, all you need is to check host validity once, at the start of your job.
I think that using dns for this purpose appears to be more clear then doing it through socket.connect. Otherwise, Its like using a bicycle when you need a wheel.

lempiy · 2019-11-01T13:25:10Z

+  }
+
+  switch (arg[2]) {
+    case '--ports':


Instead of using imperative switch case which will grow very rapidly if you gonna add more params to your program, it's better to create more universal solution to parse arguments. Maybe something like this (simplified):

function parseArguments(args) { const parsedArgs = args.slice(2).reduce((acc, arg) => { if (arg.startsWith('--') && !acc.lastKey) { acc.lastKey = arg.replace('--', '') } else if (acc.lastKey) { acc.result = {...acc.result, [acc.lastKey]: arg} acc.lastKey = null } else { throw new Error(`Unexpected argument '${arg}'`) } return acc }, {result: {}, lastKey: null}).result if (parsedArgs.lastKey) throw new Error(`Unexpected key '${acc.lastKey}' without value`) return parsedArgs.result }

const args = ['node', '/home/index.js', '--one', 'two', '--three', 'four'] parseArguments(args) // {one: "two", three: "four"}

hehe you gave me the solution, thanks, I will make the changes.

This was a very challenging function, this reduce is kind of crazy, but I like it and it has a bunch of functionalities. I hope in the future I can use it again :D

In a real world apps you might be using external package for that. (like yargs) My implementation is very straightforward.

lempiy · 2019-11-01T13:48:19Z

+  async function runPorts(host, lowerPort, higherPort) {
+    const portList = [];
+    for (let i = lowerPort; i <= higherPort; i += 1) {
+      /* eslint-disable */


use

/* eslint-disable no-await-in-loop */

lempiy

Last challenge is to fix host validation.

Chikus sniffer revision

3d34309

lempiy requested changes Oct 28, 2019

View reviewed changes

Chikus sniffer 2

5187d7a

Chikus requested a review from lempiy November 1, 2019 11:43

lempiy requested changes Nov 1, 2019

View reviewed changes

lempiy reviewed Nov 1, 2019

View reviewed changes

Chikus sniffer 3

27e090b

Chikus requested a review from lempiy November 2, 2019 21:57

lempiy requested changes Nov 4, 2019

View reviewed changes

Conversation

Chikus commented Oct 27, 2019

Uh oh!

lempiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lempiy Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lempiy Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OleksiyRudenko commented Oct 30, 2019

Uh oh!

lempiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lempiy Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lempiy Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lempiy Oct 28, 2019 •

edited

Loading

lempiy Oct 28, 2019 •

edited

Loading

lempiy Nov 1, 2019 •

edited

Loading

lempiy Nov 1, 2019 •

edited

Loading

lempiy Nov 4, 2019 •

edited

Loading

lempiy Nov 4, 2019 •

edited

Loading

lempiy Nov 1, 2019 •

edited

Loading

lempiy Nov 4, 2019 •

edited

Loading