Dictionary Data Object by stalgiag · Pull Request #1974 · processing/p5.js

stalgiag · 2017-05-30T21:05:54Z

Not ready to merge. I just wanted to put this here in progress to get feedback on my design. I also welcome any thoughts on methods that people think would be useful.

closes #422

lmccart

looks good so far, I made a few small notes

lmccart · 2017-05-30T23:04:34Z

+
+var p5 = require('../core/core');
+
+//TESTED


can these //TESTED notes turn into unit tests? ;)

lmccart · 2017-05-30T23:05:13Z

+
+
+//TESTED
+p5.TypedDict.prototype.create = function(key, value) {


with a variable number of params, our convention is to leave the parens empty

it this a public method? seems like it could be private in which case let's call it _create.

lmccart · 2017-05-30T23:08:21Z

+  this.count = 0;
+
+  if(arguments.length === 1) {
+    if(keys instanceof Object) {


hm is it keys or key? or are these two different things?

stalgiag · 2017-06-13T01:18:42Z

Okay this is getting close. All of the included unit tests work.

I feel like what it mainly needs at this point is the documentation and methods to write out to a file. I was thinking about adding the ability to write to json and csv. Any opinions on any of the above or anything else about the code?

lmccart · 2017-06-16T19:28:56Z

+    }
+  }
+  else if(arguments.length === 2) {
+    var key = arguments[0];


this code below looks a little familiar... could we just call this._addObj() here with the arguments obj?

hm I guess I am a bit confused, I was trying to handle if the user says stringDict.create('string1', 'string2' or they say stringDict.create( {'string1': 'string2', 'string3' : 'string4'} ). Sorry for my confusion!

right so the first part of the if takes care of your second case: stringDict.create( {'string1': 'string2', 'string3' : 'string4'} ).

the else if part could read like:

else if (arguments.length == 2) { this.addObj(arguments); }

to take care of the case:
stringDict.create('string1', 'string2');

lmccart · 2017-06-16T19:30:57Z

+//  */
+p5.StringDict = function(key, value) {
+  this.data = {};
+  this.count = 0;


I'm wondering about the need to keep track of count vs just calling Object.keys(this.data).length?

lmccart · 2017-06-16T19:33:27Z

+//  * @extends p5.TypedDict
+//  * @param
+//  */
+p5.NumberDict = function(key, value) {


are all these constructors generally the same? can we pass this work on to the p5.TypedDict() constructor instead to reduce redundancy?

In the latest commit, I used apply to set call the TypedDict constructor which works but creates a certain degree of messiness with the passing of arguments. I tried other approaches like Object.create and setting their constructor to the TypedDict constructor but they weren't retaining the variables made by the constructor like data. Not sure if there is a more elegant way.

lmccart · 2017-06-16T19:37:21Z

+};
+
+p5.TypedDict.prototype._validate = function(key, value) {
+  return true;


maybe the default method could make use of the type of object.. we already have number or string in the name, instead of having to write a custom validation method for each.

for example, on construction we could store a type:
this.type = m.constructor.name.replace('Dict', '').toLowerCase();

then the test would be
return (typeof value === this.type);

something like that?

I can't figure out how to do this now that both types are falling on the TypedDict constructor. I tried things like p5.StringDict.constructor = p5.StringDict to unexpected results.

lmccart · 2017-06-16T19:43:13Z

+  } else {
+    var result = Object.keys(this.data)[0];
+    for(var i=1; i>this.data.count; i++) {
+      if(Object.keys(this.data)[i] < result) {


I think this should be >

lmccart · 2017-06-16T19:45:11Z

+ *
+ *
+ */
+p5.NumberDict.prototype.minValue = function() {


i have the feeling these four blocks could be reduced into one function, since only one symbol or line changes between them. for example, if it's just a < that flips.. you can't pass operators as arguments but you could write a function that takes 1 or -1, and multiply both sides of the comparison by that operator. multiplying by -1 would have the effect of flipping the < to a >.

lmccart · 2017-06-16T19:46:02Z

looking good! I left a few comments. I love the idea of being able to export to json and csv.

as per lmccart’s suggestions

stalgiag · 2017-06-20T21:07:01Z

I made a save function but the way it currently works is that the user needs to pass in either 'csv' or 'json' as the first argument. Making people write it as a string might be a bit confusing, not sure. Other options I can think of are either making a constant or making separate saveJSON & saveCSV.

lmccart · 2017-06-20T21:09:23Z

we actually have saveJSON() and saveTable() methods in p5 already, so it may make sense to duplicate this syntax.

stalgiag · 2017-06-20T21:14:45Z

Don't know how I overlooked those, I should probably duplicate that syntax but I should also just use them to do the work. slaps head

Didn’t use p5.prototype.saveTable in TypedDict.saveTable because the data object would have to be converted to a p5.Table first which seemed like overkill

lmccart · 2017-06-26T14:31:57Z

@mlarghydracept is this ready for final review?

stalgiag · 2017-06-26T17:31:06Z

@lmccart Thanks for checking in! I am going to do another once over, write comments/examples, and a few more unit tests. I will do that this week and let you know when it is finished

lmccart · 2017-06-30T16:37:48Z

@mlarghydracept great, I'm going to close this until it's ready for review. just reopen whenever it is. this way I'll get the email notification to review.

stalgiag · 2017-07-03T02:44:40Z

I think this is ready for review!

A few notes -- I said this elsewhere in you comments on my code but I couldn't put all of the construction work in the TypedDict constructor because of the way the initializing data (the first arguments) were being retained. I tried a bunch of different approaches but maybe I am missing something.

Also this is my first time writing a new file with new functions so I wasn't completely sure what I was doing with the commenting and documentation work. I tried to follow examples from other files.

lmccart · 2017-07-03T16:21:22Z

+ * @return {Number|String} the value stored at that key
+ *
+ * @example
+ * <div><code>


if the examples aren't drawing anything, let's add class="norender" so we don't have blank gray squares in the ref.
https://github.com/processing/p5.js/wiki/Inline-documentation#adding-example-code

lmccart · 2017-07-03T16:23:22Z

+ *
+ */
+
+p5.TypedDict.prototype.clear = function(){


this could be simplified as
this.data = [];
i think?

lmccart · 2017-07-03T17:01:53Z

@mlarghydracept looks good! thanks for this big contribution. I'm going to go ahead and merge. if you can make a later pull request with the two small notes I made, that'd be great.

will you make a note of that question about constructors and we can look at it again when we're in the same place? I couldn't quite remember what I was thinking so maybe my point didn't even make sense, but I want to take one last look together at some point just in case there was some further refinement we could make.

mlarghydracept and others added 10 commits May 19, 2017 18:51

Merge remote-tracking branch 'processing/master' into p5.NumberDict

6791903

starting

a95a767

Merge remote-tracking branch 'processing/master' into p5.NumberDict

4e620dc

second

ffcc0c4

third

0fe1f27

continue getting rid of array approach

13eb714

getting there with inheritance

bea13d4

adding typed validation

3dc3695

Merge remote-tracking branch 'processing/master' into p5.NumberDict

b44cf34

starting to add type specific functions

4f1da80

lmccart reviewed May 30, 2017

View reviewed changes

mlarghydracept added 3 commits June 1, 2017 16:53

beginning commenting/fixing typos

1c61857

setting up to actually build

c4a8889

starting to add unit tests

9fa05a5

shiffman mentioned this pull request Jun 6, 2017

Data objects #422

Closed

mlarghydracept added 2 commits June 12, 2017 13:09

Merge remote-tracking branch 'processing/master' into p5.NumberDict

8c0058a

adding new NumberDict methods and getting tests working

4d2a599

lmccart reviewed Jun 16, 2017

View reviewed changes

mlarghydracept added 2 commits June 19, 2017 17:08

changes to constructor inheritance, count, and minmax functions

1512d07

as per lmccart’s suggestions

add save for json and cvs

6e3ae35

making separate save functions

6e3b124

Didn’t use p5.prototype.saveTable in TypedDict.saveTable because the data object would have to be converted to a p5.Table first which seemed like overkill

lmccart closed this Jun 30, 2017

mlarghydracept added 3 commits July 1, 2017 15:37

Merge remote-tracking branch 'processing/master' into p5.NumberDict

a04513d

adding to comments/documentation

f60ab43

more cleaning up

41f4d66

stalgiag reopened this Jul 3, 2017

lmccart reviewed Jul 3, 2017

View reviewed changes

lmccart merged commit 14b14fd into processing:master Jul 3, 2017



		//TESTED
		p5.TypedDict.prototype.create = function(key, value) {

Uh oh!

Uh oh!

Conversation

stalgiag commented May 30, 2017

Uh oh!

lmccart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stalgiag commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stalgiag Jun 16, 2017 • edited by lmccart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmccart Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmccart Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmccart commented Jun 16, 2017

Uh oh!

stalgiag commented Jun 20, 2017

Uh oh!

lmccart commented Jun 20, 2017

Uh oh!

stalgiag commented Jun 20, 2017

Uh oh!

lmccart commented Jun 26, 2017

Uh oh!

stalgiag commented Jun 26, 2017

Uh oh!

lmccart commented Jun 30, 2017

Uh oh!

stalgiag commented Jul 3, 2017

Uh oh!

lmccart Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmccart commented Jul 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stalgiag Jun 16, 2017 •

edited by lmccart

Loading

lmccart Jun 16, 2017 •

edited

Loading

lmccart Jun 16, 2017 •

edited

Loading

lmccart Jul 3, 2017 •

edited

Loading