Kickoff meeting

[Paul Gearon]

Thanks for the meetup guys. It can be hard to get these meetings going, so I appreciate the effort and I thought you did a good job last night.

The tnetstrings exercise was interesting on several levels... learning a new encoding, watching someone else approach the problem, interacting with others as suggestions were made... It certainly worked for me.

I see you've already pointed to it, but for the sake of anyone else, I blogged some comments on the meetup. It was written some time after midnight, so it may not be totally coherent.  :-)

http://gearon.blogspot.com/2012/08/clojure-dc.html

Looking forward to meeting #2.

Regards,

Paul

[Jonathan Mulieri]

Thanks for posting Paul! I noticed some forms that I haven't played around with yet, e.g. 'recur'. I've just recently started with Clojure and found your solution insightful. I finally cycled back around and worked on the parser this evening, here is my less concise stab at it... using the constructs I'm familiar with:) https://github.com/jmulieri/tnetstrings-clj/blob/master/src/tnetstrings/core.clj

Peace!

Jon

[Paul Gearon]

Hi Jon,

You've put it out there, so I'm guessing you'd like a critique? Of course, some of what I say is colored with my own preferences, but I should have a couple of useful things to say. If I'm lucky, someone may even critique my critique. :-)

I'll reference your code by line number, so I don't have to bring it all in. For convenience, it's at:

  https://github.com/jmulieri/tnetstrings-clj/blob/master/src/tnetstrings/core.clj

Starting with declares on lines 3-5, the declare macro is very similar to def. An equivalent would be to say:

(def parse)
(def parse-array)
(def parse-hash)

However, declare has the bonus that you can say it all in one line:

(declare parse parse-array parse-hash)

This would expand to:

  (do (def parse) (def parse-array) (def parse-hash))

So if you're using declare then I suggest collapsing it all into a single line (and if you want multi lines, then use def). That said, parse-array and parse-hash are only referenced in one place, so I would have moved then above the definition of parse-map thereby avoiding the need to pre-declare them.

When you define parse-map on line 7 you use the hash-map function. This function is useful to construct things programmatically, but when you're defining it statically like this then it is typical to use the literal form of {}.

Also, you've keyed parse-map on strings. While that is often useful, in this case tnetstrings is explicitly defined to use single characters, so you can take advantage of it here. Notice how on line 41 you need to calculate both the start and end of the string? If you're selecting a single character then you can just say nth to get the character at the given location, and you don't need to do the second addition (this is the same as .charAt, but takes advantage of java.lang.String being a seq).

Finally, in the map you've correctly wrapped java.lang parsing functions (which needed wrapping because they don't implement clojure.lang.IFn, Callable or Runnable), but you don't have to wrap any of the functions that are already in Clojure. Yes, it works, but it's completely redundant. So instead of:

"]" #(parse-array %)
"}" #(parse-hash %)

You can have:

"]" parse-array
"}" parse-hash

You could also say:

"," str

But this will just call str with a string, which returns an identical string. Instead, you can just have a function that returns the argument that it's given. This is already defined as identity so this makes much more sense to use instead of str.

parse-array on line 14 is mostly OK, though there are a few things. A minor point is that you can take advantage of strings being sequences by calling count instead of .length (my own code was using .substring instead of subs, so I can't talk). The main thing to comment on here is that you've wrapped the entire function in an if to provide termination of recursion. This looks so much like a loop/recur body that it just seems to be crying out for it (though that's probably because I've used loop/recur a number of times and tend to see it).

I should also complement you on using into, not because it's anything particularly special, but because I always forget to.  :-)  I tend to use a reduce/conj combo, which works but is not idiomatic. I think it betrays how I tend to think about certain types of operations.

Getting to parse-hash the first thing I noticed was that it lines 25-27 are identical to parse-array. That's almost always an indication that there's a more general operation going on. Lines 28-30 are practically identical as well: the only difference is that they include an offset. So now you've done the same operation 3 times, twice in the same function. You've already seen my approach to this, so I don't have to spell it out, but this is exactly the kind of thing you want to be on the lookout for.

Lines 31-33 have a couple of things going on. To start with, unless you have a specific reason to need a sorted-map, you shouldn't create one explicitly. It also seems strange that you chose to use one when you used a hash-map back on line 7. Again, it makes a lot more sense to create a map using the literal syntax {}. You correctly used literal syntax for a vector back on line 20, so even if it weren't more idiomatic it would make you more consistent.

The other strange thing is the use of conj to join a map to a map. The conj function takes a collection first, and an "item" second. In your case you've put a map in the item position, but an "item" for a map should be a key/value pair. ie. The following is a typical way to use conj on a map:

  (conj {"k1" "v1", "k2" "v2"} ["k3" "v3"])

That said.... I checked it out and your usage works. I was surprised to learn this. All the same, if I wanted to add a map to a map then I'd use into. If I didn't have into I've have used reduce/conj to insert the items one key/value at a time. That would be the more consistent approach.

I was curious about it, and so looked at the implementation of conj. This is just cons, with the arguments reversed (again, I wouldn't have thought it would work). So I looked at the implementation of cons in src/jvm/clojure/lang/APersistentMap.java (I was looking at the Clojure 1.4 sources). Sure enough, this checks to see if the item being cons'ed is a Map.Entry or a 2 element vector, and adds them as a key/value. But otherwise it treats the argument as a seq of Map.Entry, and inserts everything: which is the behavior you've shown. So I learnt something new.  :-)

Finally, we get to the parse function on line 35. Other than the comment I've already made about searching for a character instead of a string (as the key to parse-map on line 7) then there is nothing to comment on here.

One final question... if the string contains more data, then you'll just ignore it. This is safe, but perhaps it would be useful to see what wasn't parsed? If there was some reason to look at this data, then how do you think you might go about it? I ask this because my own code returned the unparsed data along with the parsed structure, and I noticed that the reference code on the tnetstrings site does the same. Returning both had the additional benefit of letting recursion do more of the work for me when parsing arrays and maps.

Regards,

Paul

[Jonathan Mulieri]

Hi Paul!

Thanks for taking the time to provide some great feedback! I finally got a chance to go through your suggestions:)

An interesting thing, when going through your feedback I initially didn't move the parse-map definition below the parse-array and parse-hash functions, but did change the #(parse-array %) to just parse-array, that triggered this exception: java.lang.IllegalStateException: Attempting to call unbound fn: #'tnetstrings.core/parse-array. So parse-array was an unbound variable and saving it off in the hash-map just yielded back exactly that when I referenced it. Wrapping in the anonymous function seems to have deferred the evaluation of the parse-array variable, which at execution time is correctly defined as a function. In the end, I just moved the hash definition below the functions and removed the forward declarations, as you suggested:)

I deliberately chose the sorted-map b/c I thought that the order of the parsed map should be the same as the order in the encoded data... the spec doesn't seem to say anything about order of dictionary k/v pairs, so I'm guessing it isn't really needed, that was just me trying to be precise:)

Thanks for pointing out the into vs. conj for combining the maps, that was my inexperience with the APIs and me just picking something that I found that worked. Also, using into makes the 2 functions look even more similar:)

I'll have to cycle back around and see what I can do with DRYing up the parse-XXX functions... also had that feeling of "yeah... I'm writing the same code I just wrote a minute ago...", so I agree, should probably factor something out.

For the final question: After some talk with a couple guys at the meetup, we agreed in conversation that what you parse must be 1 encoded entity, be that a string, boolean, dictionary, etc. And any sequential or nested structure must be done using the tnetstring constructs, ex. use an array as the 1 outer entity(like you would in json). That went against what I originally was planning on doing, which was to try to parse the entire string even if it contained more than 1 encoded entity, kind of like the scenario you bring into question. I was going to return a results vector which contained the parsed entities. But, I was persuaded otherwise and I think they were correct in their thinking, that you must interpret the string as 1 piece of data. After accepting that, it kind of implied that you don't really care if there was something else in the string. Furthermore, following the assumed logic, it would be an error of the sender if there were something else there. So, I think that makes sense...

Thanks again Paul! I really appreciate you sharing your insights. It reminds me of when I was first learning Ruby, I figured out ways to do things but they were often very different than the Ruby way of doing things. Having community feedback is great in getting around the curve a little faster! I also just got my copy of The Joy of Clojure in the mail, so that should help too:)

Take care!

Jon 

[Paul Gearon]

On Tuesday, September 4, 2012 12:43:58 AM UTC-4, Jonathan Mulieri wrote:

Hi Paul!

Thanks for taking the time to provide some great feedback! I finally got a chance to go through your suggestions:)

No problems. It was something interesting to do during some downtime.

 

I deliberately chose the sorted-map b/c I thought that the order of the parsed map should be the same as the order in the encoded data... the spec doesn't seem to say anything about order of dictionary k/v pairs, so I'm guessing it isn't really needed, that was just me trying to be precise:)

Maps (or dictionaries) explicitly have no defined sorting. That means that any implementation is free to provide ordering if it wants to, which is what sorted-map does. But ordering has no meaning in a dictionary unless you're iterating through it, which you're not. Hence, random ordering is fine, and the default map type provided by the {} syntax is the best approach.

Thanks for pointing out the into vs. conj for combining the maps, that was my inexperience with the APIs and me just picking something that I found that worked. Also, using into makes the 2 functions look even more similar:)

Yeah, well, as I said, I need to remember to use "into" more often myself. I always "see" these operations as a "reduce".  :-)

 

I'll have to cycle back around and see what I can do with DRYing up the parse-XXX functions... also had that feeling of "yeah... I'm writing the same code I just wrote a minute ago...", so I agree, should probably factor something out.

If you saw my blog post you'll see that I did the same thing.

 

For the final question: After some talk with a couple guys at the meetup, we agreed in conversation that what you parse must be 1 encoded entity, be that a string, boolean, dictionary, etc. And any sequential or nested structure must be done using the tnetstring constructs, ex. use an array as the 1 outer entity(like you would in json). That went against what I originally was planning on doing, which was to try to parse the entire string even if it contained more than 1 encoded entity, kind of like the scenario you bring into question. I was going to return a results vector which contained the parsed entities. But, I was persuaded otherwise and I think they were correct in their thinking, that you must interpret the string as 1 piece of data. After accepting that, it kind of implied that you don't really care if there was something else in the string. Furthermore, following the assumed logic, it would be an error of the sender if there were something else there. So, I think that makes sense...

I thought about this as well, but I had a good reason for going the way that I did.

First, by returning a pair of [parsed-data unparsed-string] I was able to recurse in order to parse arrays and maps. (This was the most compelling reason). Second, it provides more info for someone calling the parser. They can either choose to do something with the remaining string, or (more likely) report that it exists.

Following this, the parser that I wrote may benefit from a wrapper, like:

(defn parse

  [str]

  (let [[data r] (parse-t str)]

    (when (seq r) (throw (IllegalArgumentException. "Extra data found after tnetstring")))

    data))

You may want to include the "extra data" in your error message, or at least the start of that string.

 

Thanks again Paul! I really appreciate you sharing your insights. It reminds me of when I was first learning Ruby, I figured out ways to do things but they were often very different than the Ruby way of doing things. Having community feedback is great in getting around the curve a little faster! I also just got my copy of The Joy of Clojure in the mail, so that should help too:)

 

My steepest learning curve (and one that I'm still on) is learning idiomatic Clojure. I could write code that worked, but when I showed it to others they were always pointing out that I should probably have done it differently. I made the most progress in this regard by reading "The Joy of Clojure" (thanks @fogus!)

Regards,

Paul