better maintainability

[sirenkovladd]

separate files
classes instead of prototype

[sirenkovladd]

@delvedor @ivan-tymoshenko @mcollina

[Eomm]

Did you run benchmarks?

Splitting into multiple files means 3 more accesses to the file system.

[mcollina]

I don't see how this would give us better maintainability.

[sirenkovladd]

I don't see how this would give us better maintainability.

My point is that it will be more convenient for people to read the library
because each file has its own area of responsibility
it will also be more convenient to read the class instead of functions and prototype
it would be good to replace everything to one style, since other files use classes

It seems to me that when everything is in one file and written in a relatively old format, it is very difficult to maintain

[sirenkovladd]

Did you run benchmarks?

find-my-way on  better-maintainability-stage-1 is 📦 v7.6.2 via  on ☁️  (us-west-2) took 15s 
❯ node ./benchmark/bench.js
lookup root "/" route.................................... x 22,029,625 ops/sec ±1.33% (580 runs sampled)
lookup short static route................................ x 11,043,140 ops/sec ±1.38% (577 runs sampled)
lookup long static route................................. x 3,692,081 ops/sec ±0.98% (582 runs sampled)
lookup long static route (common prefix)................. x 2,490,525 ops/sec ±0.87% (577 runs sampled)
lookup short parametric route............................ x 8,019,354 ops/sec ±1.42% (573 runs sampled)
lookup long parametric route............................. x 4,263,475 ops/sec ±1.44% (572 runs sampled)
lookup short parametric route (encoded unoptimized)...... x 4,063,224 ops/sec ±1.83% (575 runs sampled)
lookup short parametric route (encoded optimized)........ x 3,295,552 ops/sec ±1.35% (589 runs sampled)
lookup parametric route with two short params............ x 4,000,458 ops/sec ±1.11% (576 runs sampled)
lookup multi-parametric route with two short params...... x 3,117,017 ops/sec ±2.15% (546 runs sampled)
lookup multi-parametric route with two short regex params x 3,370,851 ops/sec ±1.29% (576 runs sampled)
lookup long static + parametric route.................... x 2,322,848 ops/sec ±1.03% (589 runs sampled)
lookup short wildcard route.............................. x 8,701,069 ops/sec ±1.37% (583 runs sampled)
lookup long wildcard route............................... x 4,548,452 ops/sec ±3.68% (572 runs sampled)
lookup root route on constrained router.................. x 17,775,709 ops/sec ±2.02% (571 runs sampled)
lookup short static unconstraint route................... x 10,287,354 ops/sec ±1.18% (574 runs sampled)
lookup short static versioned route...................... x 8,388,208 ops/sec ±2.06% (583 runs sampled)
lookup short static constrained (version & host) route... x 7,282,033 ops/sec ±1.36% (573 runs sampled)

find-my-way on  main is 📦 v7.6.2 via  v20.6.0 on ☁️  (us-west-2) 
❯ node ./benchmark/bench.js
lookup root "/" route.................................... x 21,294,883 ops/sec ±1.20% (569 runs sampled)
lookup short static route................................ x 11,331,300 ops/sec ±1.30% (575 runs sampled)
lookup long static route................................. x 3,477,830 ops/sec ±1.41% (584 runs sampled)
lookup long static route (common prefix)................. x 2,157,526 ops/sec ±0.74% (585 runs sampled)
lookup short parametric route............................ x 8,022,611 ops/sec ±1.29% (577 runs sampled)
lookup long parametric route............................. x 4,543,553 ops/sec ±0.68% (585 runs sampled)
lookup short parametric route (encoded unoptimized)...... x 4,669,611 ops/sec ±1.13% (578 runs sampled)
lookup short parametric route (encoded optimized)........ x 3,085,747 ops/sec ±1.01% (573 runs sampled)
lookup parametric route with two short params............ x 4,314,215 ops/sec ±1.22% (584 runs sampled)
lookup multi-parametric route with two short params...... x 3,570,758 ops/sec ±1.32% (571 runs sampled)
lookup multi-parametric route with two short regex params x 4,071,093 ops/sec ±0.96% (576 runs sampled)
lookup long static + parametric route.................... x 2,141,539 ops/sec ±1.15% (578 runs sampled)
lookup short wildcard route.............................. x 9,164,402 ops/sec ±1.17% (575 runs sampled)
lookup long wildcard route............................... x 4,592,144 ops/sec ±1.22% (572 runs sampled)
lookup root route on constrained router.................. x 17,828,236 ops/sec ±1.13% (572 runs sampled)
lookup short static unconstraint route................... x 9,898,024 ops/sec ±1.38% (565 runs sampled)
lookup short static versioned route...................... x 6,427,963 ops/sec ±1.04% (584 runs sampled)
lookup short static constrained (version & host) route... x 5,399,976 ops/sec ±1.12% (583 runs sampled)

[sirenkovladd]

Splitting into multiple files means 3 more accesses to the file system.

I'm sure it didn't affect the benchmark because the library is imported there 1 time

But if you're really worried about importing multiple files, you can add a build that will put everything into 1 file without hurting the developer experience

[mcollina]

Looks like there are some regressions.
I think it's better to keep this module as-is.

[sirenkovladd]

lookup root "/" route. . . . . . . . . . . . . . . . . .  PR is faster by 3.45 %
lookup short static route. . . . . . . . . . . . . . . .  Main is faster by 2.61 %
lookup long static route. . . . . . . . . . . . . . . . . PR is faster by 6.16 %
lookup long static route (common prefix). . . . . . . . . PR is faster by 15.43 %
lookup short parametric route. . . . . . . . . . . . . .  Main is faster by 0.04 %
lookup long parametric route. . . . . . . . . . . . . . . Main is faster by 6.57 %
lookup short parametric route (encoded unoptimized). . .  Main is faster by 14.92 %
lookup short parametric route (encoded optimized). . . .  PR is faster by 6.80 %
lookup parametric route with two short params. . . . . .  Main is faster by 7.84 %
lookup multi-parametric route with two short params. . .  Main is faster by 14.56 %
lookup multi-parametric route with two short regex params Main is faster by 20.77 %
lookup long static + parametric route. . . . . . . . . .  PR is faster by 8.47 %
lookup short wildcard route. . . . . . . . . . . . . . .  Main is faster by 5.33 %
lookup long wildcard route. . . . . . . . . . . . . . . . Main is faster by 0.96 %
lookup root route on constrained router. . . . . . . . .  Main is faster by 0.30 %
lookup short static unconstraint route. . . . . . . . . . PR is faster by 3.93 %
lookup short static versioned route. . . . . . . . . . .  PR is faster by 30.50 %
lookup short static constrained (version & host) route. . PR is faster by 34.85 %

I'm sure that you can't rely on this benchmark, since you can take lookup short static versioned route as an example

30% difference in favor of PR (when nothing logically changed)
PR - 8,388,208 ops/sec ~ 119 ns/ops
main - 6,427,963 ops/sec ~ 155 ns/ops

node is not accurate enough to see the difference in 20988 ns (difference * sampled) for 574 samples
after all, these are external factors of my computer and GC

[sirenkovladd]

I believe that such quick tests have values on a logarithmic scale

[sirenkovladd]

I repeated the benchmark for the main and got this result

lookup root "/" route. . . . . . . . . . . . . . . . . .  second call is faster by 0.91 %
lookup short static route. . . . . . . . . . . . . . . .  first call is faster by 0.35 %
lookup long static route. . . . . . . . . . . . . . . . . second call is faster by 4.99 %
lookup long static route (common prefix). . . . . . . . . second call is faster by 11.89 %
lookup short parametric route. . . . . . . . . . . . . .  second call is faster by 3.81 %
lookup long parametric route. . . . . . . . . . . . . . . first call is faster by 0.79 %
lookup short parametric route (encoded unoptimized). . .  second call is faster by 2.87 %
lookup short parametric route (encoded optimized). . . .  second call is faster by 9.66 %
lookup parametric route with two short params. . . . . .  first call is faster by 1.26 %
lookup multi-parametric route with two short params. . .  second call is faster by 6.63 %
lookup multi-parametric route with two short regex params first call is faster by 22.97 %
lookup long static + parametric route. . . . . . . . . .  first call is faster by 50.43 %
lookup short wildcard route. . . . . . . . . . . . . . .  first call is faster by 26.39 %
lookup long wildcard route. . . . . . . . . . . . . . . . first call is faster by 16.83 %
lookup root route on constrained router. . . . . . . . .  first call is faster by 30.49 %
lookup short static unconstraint route. . . . . . . . . . first call is faster by 25.06 %
lookup short static versioned route. . . . . . . . . . .  second call is faster by 13.83 %
lookup short static constrained (version & host) route. . second call is faster by 42.63 %

So I can say that several tests work better in the morning than in the evening 😅
of course, it's a joke
sorry if this sounds rude, but it seems to me that you are relying heavily on very unstable benchmarks

[sirenkovladd]

Sorry, but I can't find a way to test these quick functions

• I tried switching to a virtual terminal (on linux with no GUI), disabled my GUI, killed all unnecessary processes
• ran each test from 500ms to 15 seconds
• tried different test libraries

in any case, after several runs of the same benchmark, I was getting more than 10% difference

[sirenkovladd]

Do you have any comments on PR?

[mcollina]

@ivan-tymoshenko @delvedor wdyt?

[mcollina]

lgtm for me

[ivan-tymoshenko]

'use strict'

[ivan-tymoshenko]

I don't see how it improves maintainability, but I'm not against it.

[ivan-tymoshenko]

'use strict'

[ivan-tymoshenko]

same