[racket] Size matters

I have taken the liberty to rewrite the code according to the Style guide, since I am the author of this document: 

(define (fill-sack (items items) (volume-left 0.25) (weight-left 25) (sack null) (sack-value 0))
  (cond
    [(null? items) (values (list sack) sack-value)]
    [else 
     (define item (first items))
     (define weight (item-weight item))
     (define volume (item-volume item))
     (define value (item-value item))
     (define max-q-wgt (floor (/ weight-left weight)))
     (define max-q-vol (floor (/ volume-left volume)))
     (for/fold ((best-sacks (list sack)) (best-sack-value sack-value))
       ((qty (in-range 0 (add1 (min max-q-vol max-q-wgt)))))
       (define-values (best-sack* best-sack-value*)
         (fill-sack (rest items)
                    (- volume-left (* qty volume))
                    (- weight-left (* qty weight))
                    (cons (cons qty item) sack)
                    (+ sack-value (* qty value))))
       (cond
         [(> best-sack-value* best-sack-value)
          (values best-sack* best-sack-value*)]
         [(= best-sack-value* best-sack-value)
          (values (append best-sacks best-sack*) best-sack-value*)]
         [else (values best-sacks best-sack-value)]))]))

As you can tell, the Style eliminates a lot of right-ward drift by replacing if with cond and let-ies with defines. I was also honest and left the function header intact. I'd probably lift the initialization constants out to the top-level. 

Now I understand that the three lines of extracting the content of the first struct are painful. While I understand and agree with the match here, I have also experimented with a struct-open construct for my latest project, and I find this code as readable as the one with match: 

(define (fill-sack.v2 (items items) (volume-left 0.25) (weight-left 25) (sack null) (sack-value 0))
  (cond
    [(null? items) (values (list sack) sack-value)]
    [else 
     (define item1 (first items))
     (struct-open item item1 weight value volume)
     (for/fold ((best-sacks (list sack)) (best-sack-value sack-value))
       ((qty (in-range 0 (add1 (min (floor (/ weight-left weight)) (floor (/ volume-left volume)))))))
       (define-values (best-sack* best-sack-value*)
         (fill-sack (rest items)
                    (- volume-left (* qty volume))
                    (- weight-left (* qty weight))
                    (cons (cons qty item1) sack)
                    (+ sack-value (* qty value))))
       (cond
         [(> best-sack-value* best-sack-value)
          (values best-sack* best-sack-value*)]
         [(= best-sack-value* best-sack-value)
          (values (append best-sacks best-sack*) best-sack-value*)]
         [else (values best-sacks best-sack-value)]))]))

I am also less hesitant than you to go to 102 columns and, while this is a judgement call, I would probably not name the two floor expressions if they fit. So there, that's the code according to the Style. 






On Jun 8, 2013, at 3:49 PM, Eli Barzilay wrote:

> (Possible irrelevant rambling, but I generally am very aware of my
> code formatting, and this looked like a good point to highlight.  It's
> still probably picking on something that not many people care about as
> much as I do...)
> 
> The style guide has a section labeled "size matters" -- it also has
> another section on names, encouraging using readable names.  Both of
> these are perfectly reasonable goals, but they can pull you in
> different directions, and I ran into an example that demonstrates it
> in a very nice way.  There is also Yaron's quote about favoring
> readers, which is IMO more important than both of these -- since they
> are just sub-points aiming towards that goal.
> 
> I have recently experimented more with taking code compactness more
> seriously.  I still keep my own code under 80 characters, which I know
> many people don't like, but on the other hand, I always try to fill
> the lines as much as possible while maintaining readability.  The idea
> is that the principle of favoring readers means that it should be easy
> to read code -- and lines that are too long, or suffer from rightward
> drift, or names that are too long are all things that delay reading.
> I can also see the recent tendency to use `define' where possible as
> something that goes to this end too: prefer using the same kind of
> binding construct to make it easier to read code.
> 
> So to get to my point, there's the decision of what name to use for
> your identifiers some people (*ahem*) stick to an always-readable
> names, to the point of not using names like `i', `n', and `x'.  I'm on
> the complete other side of this -- I strongly prefer these names
> because they make code shorted and therefore easier to read.  The same
> goes for the style guide's recommendation to name intermediate values
> instead of nesting function calls -- it's obviously a line that the
> author should decide on (exaggerated examples on both sides are easy,
> and contribute nothing), and I tend to go further with nested calls
> than defining intermediates.  (This same point applies to naming
> functions vs using lambda expressions.)
> 
> My point here is that using longer names and binding intermediates can
> explain the code better, but it's easy to carry this over to hurting
> overall readability.  The example that made me post this is below (no
> need to look up who wrote the original code, it's irrelevant since it
> *is* following many of the style's guidelines).  The first chunk is
> the original code, the second is my rewrite.  There are some obvious
> things like using `match' to make the code more concise and more
> readable, but note that the `cond' expression in the loop is very hard
> to read in the first version -- the descriptive names are long enough
> that the overall structure of the loop is not obvious.
> 
> In my revision, the much shorter names are less readable, but on the
> flip side they allow laying out the code in a way that makes it much
> more obvious, and since I started this by just doing mechanical
> transformations, it was surprising to see that what the shrunk code is
> doing becomes very clear.  This clarity has the obvious advantages,
> which is why it's so important to "favor readers".
> 
> As a sidenote -- there are two named intermediates in this code that I
> didn't get rid of: on one hand losing them won't save space (and
> therefore I consider removing them as something that would only
> obfuscate things), and on the other hand the result would be nesting
> the verbose computation into a place where it is not relevant.  (And
> yes, there'd be an advantage for having an infix syntax here, IMO...)
> 
> (In any case, sorry for the noise.  I'll avoid further replies...)
> 
> -------------------------------------------------------------------------------
> 
> (define (fill-sack (items items) (volume-left 0.25) (weight-left 25) (sack null) (sack-value 0))
>  (if (null? items)
>      (values (list sack) sack-value)
>      (let* ((item (first items))
>             (item-wgt (item-weight item))
>             (max-q-wgt (floor (/ weight-left item-wgt)))
>             (item-vol (item-volume item))
>             (max-q-vol (floor (/ volume-left item-vol))))
>        (for/fold
>            ((best-sacks (list sack))
>             (best-sack-value sack-value))
>          ((qty (in-range 0 (add1 (min max-q-vol max-q-wgt)))))
>          (let-values (((inner-best-sacks inner-best-sack-value)
>                        (fill-sack (cdr items)
>                                   (- volume-left (* qty item-vol))
>                                   (- weight-left (* qty item-wgt))
>                                   (cons (cons qty item) sack)
>                                   (+ sack-value (* qty (item-value item))))))
>            (cond
>              [(> inner-best-sack-value best-sack-value)
>               (values inner-best-sacks inner-best-sack-value)]
>              [(= inner-best-sack-value best-sack-value)
>               (values (append best-sacks inner-best-sacks) inner-best-sack-value)]
>              [else (values best-sacks best-sack-value)]))))))
> 
> -------------------------------------------------------------------------------
> 
> (define (fill-sack items volume-left weight-left sack sack-value)
>  (match items
>    ['() (values (list sack) sack-value)]
>    [(cons (and (item _ _ item-val weight volume) item) items)
>     (define max-q-wgt (floor (/ weight-left weight)))
>     (define max-q-vol (floor (/ volume-left volume)))
>     (for/fold ([best (list sack)] [best-val sack-value])
>               ([n (exact-round (add1 (min max-q-vol max-q-wgt)))])
>       (define-values [best* best-val*]
>         (fill-sack items
>                    (- volume-left (* n volume))
>                    (- weight-left (* n weight))
>                    (cons (cons n item) sack)
>                    (+ sack-value (* n item-val))))
>       (cond [(> best-val* best-val) (values best* best-val*)]
>             [(= best-val* best-val) (values (append best best*) best-val*)]
>             [else                   (values best best-val)]))]))
> 
> -------------------------------------------------------------------------------
> 
> -- 
>          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
>                    http://barzilay.org/                   Maze is Life!
> ____________________
>  Racket Users list:
>  http://lists.racket-lang.org/users