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

Issue 4 of cosmoz-treenode-navigator #1

Merged
merged 28 commits into from
Aug 17, 2017
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
92c303e
Refactor search(), demo added
JaySunSyn Jul 21, 2017
9dca38a
Cleanup & Demo via iron-component-page
JaySunSyn Jul 21, 2017
b19a36f
Refactor to fit cosmoz-treenode-navigator
JaySunSyn Jul 22, 2017
b9fb2ff
added cosmoz-tree-behavior
JaySunSyn Jul 23, 2017
f7d2fdb
added hook for tree class; added notify tree changes
JaySunSyn Jul 23, 2017
dea0b9a
add properties to afterFound Hook
JaySunSyn Jul 23, 2017
760b426
refaktor to make it renderLevel sensitive
JaySunSyn Jul 23, 2017
1c88582
Defaults to ES5
JaySunSyn Jul 23, 2017
cd64e21
fixes of PR comments
JaySunSyn Jul 23, 2017
5a2064e
PR comments (without rename)
JaySunSyn Jul 26, 2017
f8b75df
PR comments (back to old naming)
JaySunSyn Jul 26, 2017
5af285e
Removed & Exposed Methods, fixed issues
JaySunSyn Jul 29, 2017
d8e9ffd
Rename getPath() => getPathNodes()
JaySunSyn Jul 29, 2017
e878ec1
re-added getNodeByProperty() getPathStringByProperty()
JaySunSyn Jul 31, 2017
00bb3de
getChildren() refactor
JaySunSyn Jul 31, 2017
44ca085
Added comments and some (default) properties
JaySunSyn Jul 31, 2017
13f837a
PR comments: minor fixes, added childProperty as class attribute
JaySunSyn Aug 1, 2017
4174ae1
PR comments: search() refactor; findNode() added
JaySunSyn Aug 1, 2017
47dfbf0
Fix getPathNodes(); add hasChildren(); utilise _searchNodes() in getN…
JaySunSyn Aug 1, 2017
16dcf27
added Init options of DefaultTree
JaySunSyn Aug 2, 2017
90122a3
fixed consistent return of search()
JaySunSyn Aug 2, 2017
fa33008
Refactor getPathNodes()
JaySunSyn Aug 3, 2017
e465ec6
'basic' Test added; & adjustments
JaySunSyn Aug 3, 2017
9e5078d
'multiRoot' Test fix
JaySunSyn Aug 3, 2017
3c5318f
'missingAncestor' Test getPathNodes() in Progress
JaySunSyn Aug 3, 2017
23ab530
adjusted getPathNodes()
JaySunSyn Aug 4, 2017
5308aae
adjusted getPathNodes() Tests
JaySunSyn Aug 4, 2017
20a3aad
getPathNodes() => map(); added some tests
JaySunSyn Aug 7, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
bower_components/
node_modules/
debug.log
.DS_Store
263 changes: 166 additions & 97 deletions cosmoz-default-tree.html
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
<link rel="import" href="cosmoz-tree.html">

<!--
Navigator through object with treelike datastructure and default settings.

@demo demo/index.html
-->
<script>
'use strict';

Polymer({ is: 'cosmoz-default-tree' });
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we actually need to make this a Polymer element.
Did you see a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason was for the iron-component-page to not crash. (demo & docs)
Maybe there is another way but I didn't explore it yet..

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok well then leave it but maybe add a comment about it, too.


window.Cosmoz = window.Cosmoz || {};

function _objectValues(obj) {
if (!obj) {
return;
}
return Object.keys(obj).map(function (key) {
return obj[key];
});
Expand All @@ -23,134 +34,192 @@

Cosmoz.DefaultTree.prototype = Object.create(Cosmoz.Tree.prototype);

Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As in comment, needs to stay.

/**
* Returns the first node found.
* @return {object} or null if there was an empty response.
* @param {string} propertyName (The name of the property the match should be based on. e.g. "name")
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {boolean} exact (If the search should be executed exact or flaw. true wouldn't match "Pet")
* @param {object} nodeObj (The object the search should be based on.) or on default this._treeData
*/
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue, exact, nodeObj) {
if (propertyName === undefined || propertyValue === undefined) {
return;
}
// Defaults
exact = exact !== undefined ? exact : true;

if (propertyName === 'pathLocator') {
return this._getNodeByPathLocator(propertyValue);
return this.getNodeByPathLocator(propertyValue, nodeObj);
}

return this._searchAllRoots(propertyName, propertyValue);
var results = this.searchNodes(propertyName, propertyValue, exact, nodeObj);
return results && results.length > 0 ? results[0] : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use pop() in the other method, we should use shift() here to be consistent.
return results && results.shift() || null;

But do we need to make a difference between undefined and null here?

};

Cosmoz.DefaultTree.prototype.getPath = function (node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think we can drop getPath

if (node === undefined) {
return;
}
return this._getPathByPathLocator(node.pathLocator);
/**
* Searches a (multi root) node and matches nodes based on a property and a value.
* @return {array}
* @param {string} propertyName (The name of the property the match should be based on. e.g. "name")
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {boolean} exact (If the search should be executed exact or flaw. true wouldn't match "Pet")
* @param {object} nodeObj (The object the search should be based on.) or on default this._treeData
* @param {string} childProperty (The string which describes the child property of a node.) Default: "children"
*/
Cosmoz.DefaultTree.prototype.searchNodes = function (propertyName, propertyValue, exact, nodeObj, childProperty) {
var results = [];

// Defaults
exact = exact !== undefined ? exact : true;
var nodes = nodeObj ? _objectValues(nodeObj) : this._roots;

nodes.forEach(function (node) {
results = results.concat(this.search(node, propertyName, propertyValue, exact, childProperty));
}, this);

return results;
};

Cosmoz.DefaultTree.prototype.getPathByProperty = function (propertyName, propertyValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't drop getPathByProperty

Copy link
Contributor

Choose a reason for hiding this comment

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

As in comment, needs to stay.

if (propertyName === undefined || propertyValue === undefined) {
return;
/**
* Returns the node of a given path.
* @return {object}
* @param {string} pathLocator (The string which describes the path. e.g. "1.2.9")
* @param {object} nodeObj (The object the search should be based on.) Default: this._treeData
* @param {string} separatorSign (The string which separates the path. e.g ".") Default: "."
* @param {string} childProperty (The string which describes the child property of a node.) Default: "children"
*/
Cosmoz.DefaultTree.prototype.getNodeByPathLocator = function (pathLocator, nodeObj, separatorSign, childProperty) {
if (!pathLocator) {
return this._treeData;
}

if (propertyName === 'pathLocator') {
return this._getPathByPathLocator(propertyValue);
}
// Defaults
nodeObj = nodeObj ? nodeObj : this._treeData;
separatorSign = separatorSign ? separatorSign : '.';
childProperty = childProperty ? childProperty : 'children';

var node = this._searchAllRoots(propertyName, propertyValue);
if (node !== undefined) {
return this._getPathByPathLocator(node.pathLocator);
}
var pathNodes = this.getPathNodes(pathLocator, nodeObj, separatorSign, childProperty),
lastNodeOnPath = pathNodes.pop();
return lastNodeOnPath !== undefined ? lastNodeOnPath : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

return pathNodes.pop() || null;

But do we need to differentiate between undefined and null ?

};

Cosmoz.DefaultTree.prototype.getChildren = function (node) {
if (node === undefined) {
return;
/**
* Returns the nodes on a given path.
* @return {array} or null if there was an empty response.
* @param {string} pathLocator (The string which describes the path. e.g. "1.2.9")
* @param {object} nodeObj (The object the search should be based on.) Default: this._treeData
* @param {string} separatorSign (The string which separates the path. e.g ".") Default: "."
* @param {string} childProperty (The string which describes the child property of a node.) Default: "children"
*/
Cosmoz.DefaultTree.prototype.getPathNodes = function (pathLocator, nodeObj, separatorSign, childProperty) {
if (!pathLocator) {
return this._roots;
}
return _objectValues(node.children);
};

Cosmoz.DefaultTree.prototype.getProperty = function (node, propertyName) {
if (node === undefined || propertyName === undefined) {
return;
}
// Defaults
separatorSign = separatorSign ? separatorSign : '.';
childProperty = childProperty ? childProperty : 'children';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think since Cosmoz.DefaultTree is an implementation, it should own the (default) contract of the tree and childProperty should actually be a property of the implementation - this.childProperty - optionally configurable by the caller.


var path = pathLocator.split(separatorSign),
pathSegment = nodeObj ? nodeObj : this._treeData,
pathIsInvalid,
nodes;

// Get the nodes on the path.
nodes = path.map(function (nodeKey) {
var node = pathSegment[nodeKey],
children = node[childProperty];
if (node && children !== undefined && Object.keys(children).length > 0) {
pathSegment = children;
}
return node;
}, this);

return node[propertyName];
};
// Check if all nodes on the path are valid.
pathIsInvalid = nodes.some(function (n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When have you found the path to be invalid?
Do we need to drop the whole path?
(Also just do return node === undefined; if we need this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found it to be invalid in the current implementation.
Though, If someone would use this element with their "own" implementation of a navigator or similar it should return an "expected" (not sure if expected is the right word here) behavior.

But you are right, it actually might should be implemented in getNodeByPathLocator rather than in getPathNodes.

Imagine those nodes: 1 - 9 - 5 - 7

getPathNodes(1.9.33.24) //would return [1,9, undefined, undefined] => which could be ok

getNodeByPathLocator(1.9.33.24) //would return 9 in the current implementation => which would not be acceptable

I'll move the check to getNodeByPathLocator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I think the most plausible scenario is getPathNodes('18.1.9.5.7') which I think I would want to return [1, 9, 5, 7] due to only part of the tree being accessible in the structure.

Copy link
Contributor Author

@JaySunSyn JaySunSyn Jul 31, 2017

Choose a reason for hiding this comment

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

But I wouldn't have an easy way to figure out which path part was missing plus it could be a complete different path I intended to get (as well with undefined in the path).

Imagine a branch:

     21 - 1 - 9 - 5 -7
19 - 20 - 1 - 9 - 5 -7

have to check that use case..

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic doesn't start traversing a path until without finding a hit, so you wouldn't ever be able to just "skip" node 21 and find the path beneath it. (Also this is not a relevant scenario in our app.)
But, thinking about it, using a 1.9.20.5.7 path will put undefined in the middle which is horrible..
And even though the current implementation of getNodeByPathLocator() will get undefined at the end of the array for a bad path, causing getNodeByPathLocator() to return null, we should probably have a clearer error handling of this.

But still, any undefined in the beginning of a path is probably ok, so we might want to filter those out before doing this check:

while (nodes.length > 0 && nodes[0] === undefined) {
  nodes.shift();
}

Unless we have a reason to keep them, then we might want to make the path validator function more intelligent instead.

Also, to avoid logic like "!pathIsInvalid" which hurts readability (and the mind) we should probably invert this to something like

pathIsValid = nodes.every(function (node) {
  return node !== undefined;
}

(This also matches your code comment closer.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the rest, thanks, much nicer! (pathIsValid)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I see your point.
Again, I'm thinking from our app perspective, where that scenario would be impossible (all numbers are globally unique).
Otherwise, we would need some other method that explicitly will drop that security aspect to allow only returning the end of the path. Maybe by repeatedly calling getPathNodes() with less and less of the pathLocator until it returns a non-null value?

Copy link
Contributor Author

@JaySunSyn JaySunSyn Aug 1, 2017

Choose a reason for hiding this comment

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

Btw I need to change the node gathering of getPathNodes() to:

path.some(function (nodeKey) {
	var node = pathSegment[nodeKey],
	children = node ? node[this.childProperty] : undefined;
			
	if (node) {
		nodes.push(node);
	}

	if (node && children !== undefined && Object.keys(children).length > 0) {
		pathSegment = children;
	} else {
		return true;
	}
}, this);

Reason:
If only 100.9.7 would be valid, tree.getPathNodes('100.9.7.7') would return twice the 7 [100, 9, 7, 7] instead of [100, 9, 7] or [100, 9, 7, undefined]

With above implementation, no "undefined" values would be added. And an empty array would be returned if the start of the path would be already invalid.

Maybe thats better after all and solves the problem in both functions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I see.. how about:

	nodes = path.map(function (nodeKey) {
			if (!pathSegment || !pathSegment[nodeKey]) {
				return;
			}

			var node = pathSegment[nodeKey];
			pathSegment = node[childProperty];
			return node;
		}, this);

Then, actually, after filtering out the initial undefined, we could actually do:

pathIsValid = nodes.indexOf(undefined) === -1;

right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I guess I have the worst timing when pushing..
Could you please check if the current implementation is ok for you?

if (n === undefined) {
return true;
}
});

Cosmoz.DefaultTree.prototype._searchAllRoots = function (propertyName, propertyValue) {
var i,
node;
return pathIsInvalid ? null : nodes;
},

for (i = 0; i < this._roots.length; i+=1) {
node = this.search(this._roots[i], propertyName, propertyValue);
if (node !== undefined) {
return node;
}
}
/**
* Returns a string which describes the path of a node (found by its path locator).
* @return {string} e.g. home/computer/desktop
* @param {string} pathLocator (The string which describes the path. e.g. "1.2.9")
* @param {string} pathProperty (The property of a node on which the path should be build on. e.g "location" if node = {"location": "home"})
* @param {string} pathSeparator (The string the path should get separated with.) Default: "/"
* @param {string} separatorSign (The string which separates the path. e.g ".") Default: "."
* @param {string} childProperty (The string which describes the child property of a node.) Default: "children"
*/
Cosmoz.DefaultTree.prototype.getPathString = function (pathLocator, pathProperty, pathSeparator, separatorSign, childProperty) {
var pathNodes = this.getPathNodes(pathLocator, this._treeData);

// Defaults
pathSeparator = pathSeparator ? pathSeparator : '/';
separatorSign = separatorSign ? separatorSign : '.';
childProperty = childProperty ? childProperty : 'children';

return pathNodes.map(function (node) {
return node[pathProperty];
}).join(pathSeparator);
};

Cosmoz.DefaultTree.prototype._getNodeByPathLocator = function (pathLocator) {
var node,
path,
i;
/**
* Returns a string which describes the path of a node (found by a node's property and value).
* @return {string} e.g. home/computer/desktop
* @param {string} propertyName (The name of the property the match should be based on. e.g. "name")
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {string} pathProperty (The property of a node on which the path should be build on. e.g "location" if node = {"location": "home"})
* @param {string} pathSeparator (The string the path should get separated with.) Default: "/"
* @param {string} separatorSign (The string which separates the path. e.g ".") Default: "."
* @param {string} childProperty (The string which describes the child property of a node.) Default: "children"
*/
Cosmoz.DefaultTree.prototype.getPathStringByProperty = function (propertyName, propertyValue, pathProperty, pathSeparator, separatorSign, childProperty) {
if (propertyName === undefined || propertyValue === undefined) {
return;
}

for (i = 0; i < this._roots.length; i+=1) {
node = this._roots[i];
if (node.pathLocator === pathLocator) {
return node;
}
// Defaults
pathProperty = pathProperty ? pathProperty : 'name';
separatorSign = separatorSign ? separatorSign : '.';
childProperty = childProperty ? childProperty : 'children';

if (pathLocator.indexOf(node.pathLocator + '.') === 0) {
path = pathLocator.slice(node.pathLocator.length + 1).split('.');
for (i = 0; i < path.length; i+=1) {
if (node.children) {
node = node.children[path[i]];
} else {
node = null;
}
if (node === undefined || node === null) {
break;
}
}

if (node !== undefined && node !== null) {
return node;
}
}
if (propertyName === 'pathLocator') {
return this.getPathString(propertyValue, pathProperty);
}

var node = this.getNodeByProperty(propertyName, propertyValue);

if (node) {
return this.getPathString(node.pathLocator || node.path, pathProperty);
}
};

Cosmoz.DefaultTree.prototype._getPathByPathLocator = function (pathLocator) {
var node,
path,
subPath,
i;
/**
* Returns an Object or an Array representing the children of a node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, multiple return types.
Any code doing this.getChildren(node) should be able to do node.children instead so drop the asList argument and consider it always true (revert to old return statement)

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs needs updating

*/
Cosmoz.DefaultTree.prototype.getChildren = function (node, childProperty) {
if (!node) {
return;
}

for (i = 0; i < this._roots.length; i+=1) {
node = this._roots[i];
path = [node];
// Defaults
childProperty = childProperty ? childProperty : 'children';

if (node.pathLocator === pathLocator) {
return path;
}
return _objectValues(node[childProperty]);
};

if (pathLocator.indexOf(node.pathLocator + '.') === 0) {
subPath = pathLocator.slice(node.pathLocator.length + 1).split('.');
for (i = 0; i < subPath.length; i+=1) {
if (node.children) {
node = node.children[subPath[i]];
} else {
node = null;
}
if (node === undefined || node === null) {
path = null;
break;
} else {
path.push(node);
}
}

if (path !== null) {
return path;
}
}
/**
* Returns the property of a Node based on a given property name.
*/
Cosmoz.DefaultTree.prototype.getProperty = function (node, propertyName) {
Copy link
Contributor Author

@JaySunSyn JaySunSyn Jul 30, 2017

Choose a reason for hiding this comment

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

@nomego actually similar discussion as with getChildren. I don't get the purpose... Can we drop it? (or put it in a separate helper behavior)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it's pretty weird.
Not sure why we have it and it only seems to be used in cosmoz-treenode.
If we drop it from cosmoz-treenode I think we can drop it here, too.

if (!node || !propertyName) {
return;
}
return node[propertyName];
};


</script>
</script>
Loading