albertorestifo / node-dijkstra

A NodeJS implementation of Dijkstra's algorithm
MIT License
158 stars 37 forks source link

Can't use number as node name #11

Closed zhoudxnz closed 7 years ago

zhoudxnz commented 7 years ago

I did the following in my code and expected to get a path [1, 2]. But it returned null.

route.addNode(1, { 2: 10, 3: 20 }); 
console.log(route.path(1, 2));

Turned out it doesn't like me using numbers as the name of a node. It would be good if the library checks and raises an error, or make it work.

it('adds a node 2', () => {
      const route = new Graph();
      route.addNode(1, { 2: 10, 3: 20 }); 
      const node = route.graph.get(1);
      demand(node).be.instanceOf(Map);
      demand(node.get('2')).equal(10); //<--- this is fine
      demand(node.get(3)).equal(20); // <--- this fails
    }); 
albertorestifo commented 7 years ago

The library should be tested to work with numbers too, there is not reason for a note to not be a number.

albertorestifo commented 7 years ago

I had a deeper look at this and I decided I won't fix the issue as I don't think numbers should be used as object identifiers.

Here are the reasons why:

  1. In JSON, keys are always represented as strings
  2. Object.keys always returns an array of strings

To me is seems that, although numbers are accepted in the syntax as keys identifiers, the standard library is pushing towards strings.

I'll be happy to revisit this decisions if further interest in this arises.

zhoudxnz commented 7 years ago

Fair enough. It is easy to work around anyway. I just have to add a prefix to force every node name to a string.

kronick commented 7 years ago

I ran into the same issue and came here to report it. This is a great library and clearing up this ambiguity would make it even better!

My nodes don't have natural names so using numbers for the keys makes the most sense. Why not use a ES6 Map throughout and allow arbitrary types for all keys?

albertorestifo commented 7 years ago

@kronick Good point.

That's can be the sweet spot. Have the ability to pass Map to all the methods.