maybe switch the cond to a case?

Hi again,

I couldn't resist answering more :)

...

In particular I'm unhappy with the way the message arguments are accessed as (car arguments) rather than by explicit name. I can see that being a potential cause of bugs if the number of arguments grows and they vary with each message.

You'll probably enjoy optional parameters, then, even though my code sample is not really portable extensible:

(lambda (message #!optional arg1) (case message ((:put-name!) (push (make-blob-name arg1) unused-name-list) (randomise-list)) ...))

By the way, why sort the list? Why not also let the constructor of your data type assign the random value? I barely use DEFINE-TYPE, but I guess there's a way to make the constructor call a function to generate the random value or something like that. Unfortunately, (random init: (random-integer 42)) doesn't work.

maybe switch the cond to a case?

Indeed, that too.

P!

Hi Marjin,

Thanks for your effort here, it's much appreciated and I prefer your solution to mine in every meaningful way.

I've converted your solution to work with Gambit. There were only four small changes, but I've had to lose the steroid powered match-lambda* and replace it with a simple cond.

I've got Meroon built into this app, and I'll write the class based equivalent to see what it looks like. However I suspect that it will be more clumsy for something this straightforwards and probably isn't the best solution unless you need to make use of a feature like inheritance.

Roger.

On Thu, Jan 19, 2012 at 3:17 PM, Marijn hkBst@gentoo.org wrote:

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

On 19-01-12 01:16, Roger Wilson wrote:

...

Hi,

Hi Roger

...

I'm still using Gambit Scheme from time to time and having a lot of fun with it. I've a newbie question however, I'm trying to figure out the best way to structure code. With that in mind are there any comments on the following code?

This is often a good question, so since you asked I've spent some time tearing your code apart, but also preparing an alternative answer, so bear with me.

...

I'm trying to create an 'object' that holds a list of names, can return a random one on request that can later be returned to it.

You mention a list here, but really that is not the right data structure for random access, so don't take it literally.

...

What's the proper Scheme way of doing this kind of thing? Here's my stab, it returns a function that then accepts 'messages' as parameters....

My solution will follow you here although an alternative is to use an object/class system depending on your scheme implementation so you don't have to wire public methods manually.

...

(define (make-name-list seed-names)

;; The type the list is built from (define-type blob-name

Really dislike the name `blob-name' you chose here and it leads to horrendously named accessors. Better just use `blob', `entry' or `elt'. Also there is no reason this data structure need only contain names; really any scheme value can be stored in it, so you should choose more generic names based on that.

...

name (random init: 0)) ; Used for sorting

Such as `item' and `key'.

...

(let ((unused-name-list '()))

Why would you want to have an ``unused list'' in your data structure? Surely it would be better if the data structure members are actually put to good use? ;P

...

(define (randomise-list) ; Assign each name a random number and then sort them (map (lambda (x) (blob-name-random-set! x (random-integer 65536))) unused-name-list)

...

(sort! unused-name-list (lambda (x y) (if (< (blob-name-random x) (blob-name-random y)) #t #f))))

;; Put all the seed names in the list (map (lambda (x) (push (make-blob-name x) unused-name-list)) seed-names)

Try to limit the line-width you use (for email consumption).

...

(lambda (message . arguments) (cond ((eq? message ':get-name!) (if (null? unused-name-list) "No names available" (begin (let ((name (blob-name-name (car unused-name-list)))) (set! unused-name-list (cdr unused-name-list)) name))))

; Add a name to the list, used to return names once they're no longer needed

What do you mean no longer needed? These names WILL come back if you ':get-item! long enough.

...

((eq? message ':put-name!) (push (make-blob-name (car arguments)) unused-name-list) (randomise-list))

...

; Debug, print the internal display ((eq? message ':display) (display unused-name-list))

(else (error 'name-list "unrecognized message"))))))

;; Test drive code....... (define name-list (make-name-list (list "Albert" "Bruce" "Clive"))) (display (name-list ':get-name!)) (name-list ':put-name! "David") (name-list ':put-name! "Edgar") (name-list ':put-name! "Frederick")

(display (name-list ':display))

Excellent, we have test code!

...

Some points:

In particular I'm unhappy with the way the message arguments are accessed as (car arguments) rather than by explicit name. I can see that being a potential cause of bugs if the number of arguments grows and they vary with each message.

Really what you would like here is to be able to match the arguments to a number of patterns which correspond to the different ways this procedure can be validly called. Thinks of `match' as `case' on steroids. Really BIG steroids. Steroids so big you can hide several dozen cases of `case's in their shadow. `match' swallows these steroids whole so you get the idea... A quick search seems to indicate `no', but: Does gambit have `match'?

...

I realise that the randomise-list function is probably called too often and maintaining a 'dirty bit' would allow it to be only called when necessary when getting a name.

It is an option, but you can do even better if you use a vector to store your items.

...

I've years of C/C++ programming behind me, which is probably readily apparent.

No comment? ;P Seriously, you made that too easy. More seriously, it can go either way and if you're aware of the potential for bad influence (like you seem to be) then it should be possible to use that experience to your advantage. For example when estimating the cost of certain constructs.

...

How can I improve the code?

I have prepared an alternative solution to your specification here. In racket since that is what I am most familiar with atm. Really the only racket-specific stuff seems to be the match-lambda*, although keyword arguments probably work differently in gambit and I'm not sure gambit has a vector->values which isn't essential to anything. And I don't know about unit testing for gambit. Some expert gambitteer(?) should be able to fill in the details of those differences.

#lang racket

(require rackunit)

(define (make-random-store #:capacity (capacity 99) . init-entries) (define size 0) (define store (make-vector capacity))

(define (add-entries entries) (for-each (lambda (e) (add-entry e)) entries))

(define (add-entry entry) (define (_add-entry) (vector-set! store size entry) (set! size (+ size 1)))

(define capacity (vector-length store))

(if (< size capacity) (_add-entry) (let ((new-store (make-vector (* 2 capacity)))) (vector-copy! new-store 0 store) (set! store new-store) (_add-entry))) )

(define (remove-entry n) (cond ((< -1 n size) (set! size (- size 1)) (define ret (vector-ref store n)) (vector-set! store n (vector-ref store size)) ret) (else (error "index out of range")) ) )

(define (entries) (vector->values store 0 size))

(add-entries init-entries)

(match-lambda* ((list 'put! entries ...) (add-entries entries)) ((list 'get!) (remove-entry (random size))) ((list 'size) size) ((list 'entries) (entries)) ) )

;; Test drive code....... (define name-list (make-random-store "Albert" "Bruce" "Clive")) (check-true (set=? (call-with-values (lambda () (name-list 'entries)) set) (set "Albert" "Bruce" "Clive"))) (define name1 (name-list 'get!)) (check-not-false (member name1 '("Albert" "Bruce" "Clive"))) (name-list 'put! "David" "Paul") (name-list 'put! "Edgar") (name-list 'put! "Frederick") (define name2 (name-list 'get!)) (check-true (set=? (set-union (set name1 name2) (call-with-values (lambda () (name-list 'entries)) set)) (set "Albert" "Bruce" "Clive" "David" "Paul" "Edgar" "Frederick") ))

I hope that was helpful,

Marijn -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8YM/cACgkQp/VmCx0OL2w66ACfQXj/PeGIf0Ibq3AdebTmVKiD cwQAoJf/Uz9hRyK1ekPcjJfVbTGm09jS =XdPT -----END PGP SIGNATURE-----