views:

1634

answers:

3

From googling around, I found that using while loops or using variables is discouraged.

Now I implemented a very simple algorithm that will read characters from an inputstream and parse accordingly: if input is 10:abcdefghej it will parse out 10 then read next 10 bytes after the colon.

The thing I am kinda lost with is how I can refactor this so it does not rely on variables.


(defn decode-string [input-stream indicator]

  (with-local-vars [length (str (char indicator) )
         delimiter (.read input-stream ) 
         string (str "")
         counter 0 ]

    (while (not(= (var-get delimiter) 58 ))
       (var-set length (str (var-get length) (char (var-get delimiter)) ))
       (var-set delimiter (.read input-stream )))

    (var-set length (new BigInteger (var-get length)) )
    (var-set counter (var-get length))

    (while (not(zero? (var-get counter) ))
       (var-set string (str (var-get string) (char (.read input-stream ))  ))
       (var-set counter (dec (var-get counter))))
    (var-get string)))

Also, I understand that the only way to declare variables is using the with-local-vars keyword. Isn't it kind of unpractical to define all the variables in one block at the beginning, or am I missing some crucial point?

+4  A: 

I'm learning Clojure myself, so take this not as guru advice but as a fellow students advice.

Clojure is a functional programming language. Functional programming means no loops, no variables, and no side effects. If you ever deviate from those three rules you need very good reasons for doing so and valid reasons are pretty rare.

You are obviously a very proficient programmer so take a look at this information and you should hopefully get more of an idea of how Functional design is different than Object Oriented design.

http://en.wikibooks.org/wiki/Clojure_Programming/Concepts#Sequence_Functions

Also, I would recomend looking at some clojure code, here is a sample program hosted on github.com which was written to be part of a clojure screencast-tutorial.

http://github.com/technomancy/mire/tree/master

The screencast-tutorial which the code was meant for can be found here but it is not free:

http://peepcode.com/products/functional-programming-with-clojure

(I'm not affiliated with peepcode.com in anyway).

Good luck with Clojure!

AlexCombas
+11  A: 

What you are writing is C code with lisp-like syntax (no offense intended). Defining a style by what you don't do is very defining, but it is not very helpful if you don't know "well, then how else?"

By the way, I don't know what indicator is supposed to do.

This is how I would approach this problem:

  1. The problem has two parts: find the number of characters to read, then read that many characters. Therefore, I would write two functions: read-count and read-item, the latter using the former.

    (defn read-count [stream]
      ;; todo
      )
    
    
    (defn read-item [stream]
      ;; todo
      )
    
  2. read-item first needs to determine the number of characters to read. For that, it uses the convenient function read-count that we will also define.

    (defn read-item [stream]
      (let [count (read-count stream)]
        ;; todo
        ))
    
  3. Looping is in Clojure generally best handled by using loop and recur. loop also binds variables, like let. acc is meant to accumulate the read items, but note that it is not modified in place but re-bound each iteration.

    (defn read-item [stream]
      (loop [count (read-count stream)
             acc ""]
        ;; todo
        (recur (dec count)        ; new value for count
               (str acc c)))))    ; new value for acc
    
  4. Now we need to do something in that loop: bind c to the next character, but return acc when count is 0. (zero? count) is the same as (= count 0). I annotated the if form a bit for those unfamiliar with it.

    (defn read-item [stream]
      (loop [count (read-count stream)
             acc ""]
        (if (zero? count)                  ; condition
            acc                            ; then
            (let [c (.read stream)]        ; \
              (recur (dec count)           ;  > else
                     (str acc c)))))))     ; /
    
  5. Now all we need is the read-count function. It uses a similar loop.

    (defn read-count [stream]
      (loop [count 0]
        (let [c (.read stream)]
          (if (= c ":")
              count
              (recur (+ (* count 10)
                        (Integer/parseInt c)))))))
    
  6. Test it on the REPL, debug, refactor. Does .read really return characters? Is there a better way to parse an integer?

I have not tested this, and I am a bit hampered by not having any experience nor deep knowledge of Clojure (I use Common Lisp mostly), but I think that it shows how to approach this kind of problem in a "lispy" way. Note how I don't think about declaring or modifying variables.

Svante
if i understand your advice correctly i should attack a problem by trying to compose it as much in to separate functions as possible am i right?
Hamza Yerlikaya
I wouldn't say as much as possible, rather as much as sensible---you find out what concepts you can name, then encapsulate them behind that name. However, this is just one little aspect of the above.
Svante
+3  A: 

Idomatic Clojure really lends it self to working with sequences. In C i tend to think in terms of variables or changing the state of a variable a bunch of times. In Clojure I think in terms of sequences. In this case I would break the problem into three layers of abstraction:

  • turn the stream into a sequence of bytes.
  • turn the sequence of bytes into a sequence of characters
  • translate the sequence of characters into a sequence of strings.

stream to bytes:

defn byte-seq [rdr]  
  "create a lazy seq of bytes in a file and close the file at the end"  
  (let [result (. rdr read)]  
    (if (= result -1)  
      (do (. rdr close) nil)  
      (lazy-seq (cons result (byte-seq rdr))))))  

bytes to chars

(defn bytes-to-chars [bytes]
  (map char bytes))

chars-to-strings [chars]

(defn chars-to-strings [chars]
   (let [length-str (take-wile (#{1234567890} %) chars)
         length (Integer/parseInt length-str)
         length-of-lengh (inc (count length-str)) 
         str-seq (drop length-of-length chars)]
        (lazy-seq 
          (cons
            (take length str-seq)
            (recur (drop (+ length-of-length length) chars))))))

This is evaluated lazily so every time the next string is needed it will be pulled from the input stream and constructed. you could use this on a network stream for instance with out having to buffer the whole stream first or worry about having the code reading from this stream worry about how it is constructed.

ps: im not at my REPL at the moment so please edit to fix any erros :)

Arthur Ulfeldt