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.

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

Formatting #27

Merged
merged 9 commits into from
Jun 16, 2014
Merged

Formatting #27

merged 9 commits into from
Jun 16, 2014

Conversation

TiTi
Copy link
Contributor

@TiTi TiTi commented Jun 16, 2014

Code formatting.
Modifications are in demo.js.
Especially the Fix .cost vs .getCost().

TiTi added 9 commits June 16, 2014 23:01
- Only use one var when possible
- Define one var on each line (child1N)
- Avoid comments on 2 lines for a few chars
- Remove unecessary empty lines
No code change. Only replace tabulations. Thx Notepad2
Ability to put breakpoints into setTimeout functions
var sTime = new Date();
var result = astar.search(graph, start, end, options);
var eTime = new Date();
start = graph.grid[start[0]][start[1]];
Copy link
Owner

Choose a reason for hiding this comment

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

Missing var here?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh never mind - I see what you are doing with reusing the parameter now

@bgrins bgrins merged commit 71ecf8e into bgrins:master Jun 16, 2014
@bgrins
Copy link
Owner

bgrins commented Jun 16, 2014

Awesome! I've also updated the test harness for more strict checking in jshint

@bgrins
Copy link
Owner

bgrins commented Jun 17, 2014

Two notes:

  1. I'm wondering if we should expose astar.Graph instead of a global Graph object. Maybe we could consider changing the API even further for a v1 release.
  2. As there are no pending PRs, I'm thinking of globally switching from 4 spaces to 2 on the size of tabs. I have come to find this easier to read as it can lead to less columns per line - but if I did that we should change back to this pattern:
var foo = 1;
var bar = 1;

Instead of

var foo = 1,
  bar = 1;

Since it wouldn't line up nicely anymore

@TiTi
Copy link
Contributor Author

TiTi commented Jun 17, 2014

Hello. Thanks for the congrats and the merge. Regarding your notes:

  1. Hum I don't really have an opinion about this. At first I thought that it'll probably be a little more painful to create a graph.
    But now entries points are limited to:
  • new [astar.]Graph()
  • astar.search(graphObj, ...)

Moreover, either you use the default Graph, or completely bypass it with another one (ex: CityGraph).
Also: No need to access Graph directly, to update one method or the prototype (at least no evident need).
And this would still be possible if accessible through astar..

So it might be interesting not to pollute global scope with Graph.
I like the idea of only one variable exposed.
This will probably be better.

That's also breaking change. I'd just realized we've also made some other breaking changes recently (.getCost(), .weight, start & end nodes vs pos object, .neighbors(), ...).

Regarding API, yeah I got the feeling something better than Graph/CityGraph/CustomGraph is probably achievable but that good enough for a very specific lib. And I don't really see a better structure for now.

  1. I personnally prefer Allman style and tabs based indentation...
    It consume more space but visual brackets alignment is simply clearer, even with a modern IDE (brackets matching). This avoids mistakes and facilitate strcture comprehension.
    Anyhow I have no intention to start a troll, that's why I tried hard to stick to existing code style for astar.js ;)
    So I guess you're alone for this dilemma :p
    In case you decide to change actual style, please do that in a specific commit without code change.

@TiTi
Copy link
Contributor Author

TiTi commented Jun 17, 2014

One thing you might probably know already: https://github.com/rwaldron/idiomatic.js/
It gives key points regarding code conventions.

var foo = 1,
  bar = 1;

isn't lined up but that's all right

@bgrins
Copy link
Owner

bgrins commented Jun 18, 2014

I don't feel too strongly about the whitespace here, but I think the Graph object should definitely be moved out of the global scope. I've opened #29 if you are curious.

TiTi pushed a commit to TiTi/javascript-astar that referenced this pull request Jun 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants