search

Cloud-Automation / node-modbus

Modbus TCP Client/Server implementation for Node.JS
465 stars 174 forks source link

deleted outdated exampels , using const instead of let when possible, add a test coverage check #231

Closed flottokarotto closed 5 years ago

flottokarotto commented 5 years ago

The outdated examples were confusing to me; after understanding how to use the new version, I decided to make a fork and clean up. Using const instead of let should speed the code up a bit and make it more readable. After running the tests, I was wondering how much code was covered, so I added the coverage checker.

cwg999 commented 5 years ago

Using const instead of let should speed the code up a bit and make it more readable.

Do you have a resource that mentions this...?

flottokarotto commented 5 years ago

const declarations gives the (jit) compiler more opportunities to optimize the code. They also make sure, that you don't mistakenly re-assign an object, thats what I ment with "readable". Here is a stackoverflow discussion about the topic: https://stackoverflow.com/questions/40070631/v8-javascript-performance-implications-of-const-let-and-var

cwg999 commented 5 years ago

const declarations gives the (jit) compiler more opportunities to optimize the code.

const over let performance though? That S.O. post doesn't really mention that, does it? I have yet to really see any real tests that might convince me to start using const over let for performance reasons.

https://stackoverflow.com/questions/42275425/benefit-of-const-vs-let-in-typescript-or-javascript

flottokarotto commented 5 years ago

I didn't meat the performance reason mainly, the main reason is the error prevetion / readability. He wrote in the S.O Post:

In some cases, const may well provide an opportunity for optimization that var wouldn't, especially for global variables.

But you are right, there is no 100% proof that const will speed it up. it also will depend on the js runtime.

stefanpoeter commented 5 years ago

Thats great, thanks a lot @flottokarotto

What did the test coverage tool report?

I'll have a deeper look tomorrow.

flottokarotto commented 5 years ago

@stefanpoeter It basically gives you an overview about how much of your code is actually tested, and which lines are not tested. There are also metrics about the the code paths are tested (branch => if / else / switch)

stefanpoeter commented 5 years ago

@flottokarotto I'll checked the code. I'll make it a patch (3.1.3) since this is pure refactoring. Or am I wrong?

It basically gives you an overview about how much of your code is actually tested, and which lines are not tested.

I'll know what a coverage tool does, I was curious about the results ;-)

flottokarotto commented 5 years ago

Ah, sorry ... Here you are: 

------------------------------------|----------|----------|----------|----------|-------------------|
File                                |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------------------|----------|----------|----------|----------|-------------------|
All files                           |    85.19 |    79.57 |    83.33 |    85.19 |                   |
 src                                |    79.97 |    68.75 |    88.99 |    79.97 |                   |
  buffer-utils.js                   |      100 |      100 |      100 |      100 |                   |
  client-request-handler.js         |    89.19 |       80 |       90 |    89.19 |... 56,78,79,85,86 |
  client-response-handler.js        |    71.43 |       50 |    66.67 |    71.43 |              9,19 |
  modbus-client.js                  |    96.88 |     62.5 |      100 |    96.88 |             22,28 |
  modbus-rtu-client.js              |    57.14 |      100 |        0 |    57.14 |          26,28,29 |
  modbus-rtu-server.js              |      100 |      100 |      100 |      100 |                   |
  modbus-server-client.js           |    85.71 |      100 |       50 |    85.71 |          19,23,41 |
  modbus-server-request-handler.js  |      100 |      100 |      100 |      100 |                   |
  modbus-server-response-handler.js |    38.55 |    48.08 |      100 |    38.55 |... 71,272,273,274 |
  modbus-server.js                  |    83.33 |       70 |       60 |    83.33 |             20,28 |
  modbus-tcp-client.js              |      100 |      100 |      100 |      100 |                   |
  modbus-tcp-server.js              |      100 |      100 |      100 |      100 |                   |
  modbus.js                         |      100 |      100 |      100 |      100 |                   |
  rtu-client-request-handler.js     |    89.66 |    66.67 |      100 |    89.66 |          38,44,45 |
  rtu-client-response-handler.js    |      100 |      100 |      100 |      100 |                   |
  rtu-request.js                    |    86.49 |       75 |    77.78 |    86.49 |    27,37,38,52,60 |
  rtu-response.js                   |    93.55 |      100 |      100 |    93.55 |             38,39 |
  tcp-client-request-handler.js     |    90.91 |       75 |      100 |    90.91 |          41,47,48 |
  tcp-client-response-handler.js    |      100 |      100 |      100 |      100 |                   |
  tcp-request.js                    |       90 |       75 |    90.91 |       90 |       29,34,35,81 |
  tcp-response.js                   |    94.59 |       50 |      100 |    94.59 |             40,41 |
  user-request.js                   |    94.44 |      100 |    90.91 |    94.44 |                45 |
 src/request                        |    93.75 |    92.66 |    84.29 |    93.75 |                   |
  read-coils.js                     |    92.31 |    83.33 |    85.71 |    92.31 |             36,55 |
  read-discrete-inputs.js           |    96.15 |      100 |    85.71 |    96.15 |                56 |
  read-holding-registers.js         |    96.15 |      100 |    85.71 |    96.15 |                56 |
  read-input-registers.js           |    96.15 |      100 |    85.71 |    96.15 |                52 |
  request-body.js                   |     77.5 |       80 |       60 |     77.5 |... 60,61,70,85,92 |
  write-multiple-coils.js           |    97.37 |    96.88 |    81.82 |    97.37 |           104,129 |
  write-multiple-registers.js       |       96 |    93.75 |    91.67 |       96 |            15,100 |
  write-single-coil.js              |    95.83 |     87.5 |    85.71 |    95.83 |                52 |
  write-single-register.js          |    96.15 |      100 |    85.71 |    96.15 |                51 |
 src/response                       |    88.25 |    81.43 |    74.68 |    88.25 |                   |
  exception.js                      |    93.75 |      100 |    85.71 |    93.75 |                32 |
  read-coils.js                     |    97.44 |      100 |    88.89 |    97.44 |                78 |
  read-discrete-inputs.js           |    94.59 |     87.5 |    88.89 |    94.59 |             40,75 |
  read-holding-registers.js         |    74.47 |    71.43 |       50 |    74.47 |... 01,102,103,106 |
  read-input-registers.js           |    89.47 |       90 |       70 |    89.47 |       32,69,73,77 |
  response-body.js                  |    64.29 |       50 |       60 |    64.29 |    17,19,28,40,47 |
  response-factory.js               |    97.22 |    94.44 |      100 |    97.22 |                68 |
  write-multiple-coils.js           |    86.36 |       50 |    71.43 |    86.36 |          26,39,43 |
  write-multiple-registers.js       |    86.36 |       50 |    71.43 |    86.36 |          26,39,43 |
  write-single-coil.js              |    90.91 |       50 |    85.71 |    90.91 |             30,47 |
  write-single-register.js          |    86.36 |       50 |    71.43 |    86.36 |          26,39,43 |
------------------------------------|----------|----------|----------|----------|-------------------|
cwg999 commented 5 years ago

What coverage tool are you using? :D

flottokarotto commented 5 years ago

nyc https://www.npmjs.com/package/nyc

stefanpoeter commented 5 years ago

It is done, thanks a lot.