Closed GoogleCodeExporter closed 9 years ago
Initial commit made. If anyone has time I would appreciate a code review on
this
stuff. I'm relatively new to javascript and any suggestions regarding style or
a
better wayto do something would be great.
Also, I seem to be having an issue with the Family mutator in common.js. It
doesn't
seem to work properly in an extended class. When I set it in Jx.Store which
extends
Jx.Object and run the spec test the $type() method returns Jx.Object instead of
Jx.Store. Anyone have an idea as to why that would happen?
Original comment by jbomgard@gmail.com
on 18 May 2009 at 4:48
I've done a code review of the new stuff - first let me say wow, that's a ton
of great code! Now the comments
... hopefully these format okay
* overall, I'm really happy with the way this is coming together, I think it is
going to provide a lot of great
functionality and an excellent framework for developing new code
* we need to make sure the header comments are standardized with the others and
that you add a copyright
for yourself in code that is predominantly yours.
* stylistic questions (which we should probably discuss on the list):
- should we use underscore to prefix attribute and method names that are intended to be private in scope?
- should we use camelCase or lowercase for event names?
* Jx.Store.Sortable
- refers to this._resolveCol but that is not declared in this class, it is defined in Jx.Store. This means
Jx.Store.Sortable mixin can only be mixed into a Jx.Store or another class that
defines _resolveCol. This
doesn't really feel right (having a mixin class require an API of the class
being mixed into). I don't have an
alternate proposal right now.
- the use of _determineComparator and _determineSorter could be done a little more elegantly I think. We
could define a sorters attribute with the name as the key and a reference to
the class as the value:
sorters: {
quick: Jx.Sort.Quicksort,
merge: Jx.Sort.Mergesort,
heap: Jx.Sort.Heapsort
}
this._sorter = new this.sorters[sort](data, col, fn);
We could also just define the methods inside the sort function so they are out
of the class scope but still
accessible
sort: function(col, sort, data, ret) {
var determineComparator = (function() {}).bind(this);
var determineSorter = function(data, sort, col, fn) {};
...
}
* Jx.Sort
- seems like _swap could be made a global $swap function ala other mootools functions or Jx.swap at least.
- I would prefer that method names, especially API methods, be spelled out in full in most cases, so setCol
would become setColumn
Original comment by pagameba
on 21 May 2009 at 12:59
Thanks for the review! I also really like the way things are coming together as
well.
I'll look at incorporating your comments. Here area few of my immediate
thoughts in
response:
* style question - I'll post something to the list to begin a discussion. The
underscore thing was something I carried over from PHP but I'm by no means
stuck on it.
* What header information should be considered standard?
* I like your first suggestion regarding the _determine methods. I'll implement
this
and a similar one for comparator. As for the _resolveCol method... I'll need to
give
that some thought as this was originally an extended class not a mixin and I
just
recently changed it and forgot I used that method. If you think of some way to
fix it
let me know.
* For the _swap method in Jx.Sort - do you think that it could have a more
global
use? If so, then I kinda like the Jx.swap version to keep it in the namespace.
* I'll go through and check the method names to make sure they are spelled out.
Original comment by jbomgard@gmail.com
on 21 May 2009 at 3:42
I also reviewed your code and would suggest to use the native Array.sort
function
instead of javascript sorters for speed reasons.
I implemented an alternative store (just a fast hack but functional).
It handels data reading, sorting and json requests.
Usage:
window.addEvent('load',function(){
var store = new Jx.Store({
columns: {
'id':{
'type':'int',
'display':'Id'
},
'name':{
'type':'string',
'display':'Name'
}
},
data: [
[1,'test1'],
[2,'test2'],
[3,'test3'],
[4,'test4']
]
});
console.log('unsorted',store.count());
if (store.hasNext()){
do {
console.log(store.getValue(0),store.getValue(1));
}
while(store.next());
}
store.sort('id',true);
store.rewind();
console.log('sorted by id desc')
if (store.hasNext()){
do {
console.log(store.getValue('id'),store.getValue('name'));
}
while(store.next());
}
store.sort('name',true);
console.log('sorted by name desc')
store.rewind();
if (store.hasNext()){
do {
console.log(store.getValue('id'),store.getValue('name'));
}
while(store.next());
}
store.addEvent('loadsuccess',function(){
console.log('load success')
store.sort('name');
console.log(store.count(), 'sorted by name')
if (store.hasNext()){
do {
console.log(store.getValue('id'),store.getValue('name'));
}
while(store.next());
}
});
store.addEvent('loadfailed',function(obj){console.log('failed',obj)});
store.loadData({url:'/index/json'});
/* returns
{data:[
[1,'sdfgdsgf'],
[2,'gdfhf'],
[3,'dfghfgh'],
[4,'xcbvvvvvv'],
[5,'sdfgsdfdsdsgf'],
[6,'asdafsfd'],
[7,'rzurztu'],
[8,'gklkjlkh']
]
}
*/
})
Maybe we could merge both implementations
KASI
Original comment by k...@arielgrafik.de
on 21 May 2009 at 9:41
KASI - can you post the actual implementation or send it to me soI can have a
look
at it as it doesn't seem to be here? I'm always open to different ways to do
things.
One thing I noticed from what you posted here was that this implementation
seems to
work on an array or arrays. Mine works on an array of Hashes. I'm not sure if
the
difference is a big deal or not.
Anyway, send yours along and I'll give it a look with an eye towards using the
native function instead.
Thanks,
Jon
Original comment by jbomgard@gmail.com
on 21 May 2009 at 9:58
Hi Jon,
it is in trunk/src/Source/data/store2.js
KASI
Original comment by k...@arielgrafik.de
on 22 May 2009 at 11:42
So I've done a bit more work on the data store.
1) changed Jx.Store.Sortable to determine comparator and sorting algorithm in a
more
elegant fashion. I'm learning alot with this so you can expect the internals of
alot
of the other classes to change later.
2) changed public API to spell out method names (i.e. setCol became setColumn)
3) implemented a sorter based on the native Array.sort and added it to the
collection of sorters. This one was interesting because in informal testing the
mergesort algorithm is actually faster than the native array sorting. To see
the
timings, run the spec tests in firefox and look in the console. You'll see some
interesting results. The native sort is comparable in speed to the heapsort
implementation, and of all things the "quick" sort is the slowest - though that
might just be my inefficient implementation. So, I think for now I'll leave all
of
the sorting options in. If I get some time I'll try them with a much larger
data set
which might (and probably will) change the results.
Still Todo:
1) add comments and copyright files
2) look at the _resolveCol part of the sorter. I don't think there's a way
around it
as these sorters as specifically designed to work with the current store
implementation (using an array of hashes). If anyone has a suggestion on that
(see
Paul's comments above) I'd be open to it.
3) add some more events for other objects to hook into. I'll probably add a
dataChanged event but am looking for suggestions for other events that might be
handy to have available.
Original comment by jbomgard@gmail.com
on 22 May 2009 at 6:47
Here are the timings I came up with. These are in Firefox 3.0.10 on Linux.
mergesort - numeric : 4ms
mergesort - alphanumeric : 3ms
mergesort - date : 20ms
quicksort - numeric : 24ms
quicksort - alphanumeric : 8ms
quicksort - date: 70ms
heapsort - numeric : 11ms
heapsort - alphanumeric : 7ms
heapsort - date : 64ms
nativesort - numeric : 11ms
nativesort - alphanumeric : 6ms
nativesort - date : 63ms
Original comment by jbomgard@gmail.com
on 22 May 2009 at 7:17
I consolidated groupable and sortable into the main Jx.Store. Also moved
Jx.Sort.swap() to Array.swap() in common.js. Added a few new events for
flexibility
and fixed tests. Give it a whirl and let me know what you think.
Original comment by jbomgard@gmail.com
on 7 Jun 2009 at 3:29
Original comment by pagameba
on 2 Nov 2009 at 4:19
Original comment by fred.war...@gmail.com
on 2 Nov 2009 at 4:31
Original comment by fred.war...@gmail.com
on 2 Nov 2009 at 4:51
For anyone who need to know, I'll be making some major changes in how the store
works
based on discussions in the google group. Specifically:
http://groups.google.com/group/jxlib/browse_thread/thread/b079b038dd89f39
see the thread for the details. Once that's done and tested I think we can
consider
this issue completed.
Original comment by jbomgard@gmail.com
on 8 Nov 2009 at 6:44
Major rewrite to Jx.Store has been completed and committed in r637. There are
still a
few pieces I think we should do here:
- Jx.Store.Protocol.DOM
- Jx.Store.Protocol.JSONP (for cross domain json APIs though I'd like some
feedback
on this one)
- Jx.Store.Parser.HTML
- Jx.Store.Parser.XML (I'm not really familiar with working with XML in JS so
I'd
kinda need to rely on someone else to take this one)
Original comment by jbomgard@gmail.com
on 21 Nov 2009 at 7:23
Jon, I think we could leave some of these as open enhancements for version 3,
the architecture is in place to
support them which is the most important thing. If there is no immediate need
that anyone has expressed for
these then we should defer them and focus on getting a release out the door
with all this new, cool stuff!
Original comment by pagameba
on 30 Nov 2009 at 1:23
As the basic architecture is together and working I'm going to close this as
fixed
and move the other pieces into a different issue for a future milestone.
Original comment by jbomgard@gmail.com
on 30 Nov 2009 at 10:28
Original issue reported on code.google.com by
jbomgard@gmail.com
on 7 May 2009 at 5:33Attachments: