Skip to main content
added 19 characters in body
Source Link
Dave Yarwood
  • 1.1k
  • 6
  • 13

I noticed that you have some def and defn statements within the function definition of process-match. It's generally considered un-idiomatic to use def/defn anywhere except for at the top level of the namespace you're in. It looks likeUsually you would use bufferlet is only used within theinstead if what you're trying to process-matchdefine isn't used outside of the function, sobut it doesn't look like that's the case here -- I think you should useare wanting letbuffer instead. Yourand echo function seems to to be used elsewhereat the top level, so I would pull thatthem out and define it as a private function at the top level; now, the echo function depends on buffer being defined, so what you'll need to do is add a buffer argument forof the echoprocess-match function, and then pass in the appropriate StringBuffer object whenever you need to use it. I would consider rearranging your code to lookdo something like this:

(ns closeddoor.core
  (:require clojure.string)
  (:gen-class :main true))

(def tag-start "<\\?clj")  ; The long/normal start tag.
(def tag-end "(\\?>|\\Z)") ; The long/normal end tag.

(def echo-start "<%") ; The short/echo start tag.
(def echo-end "%>")   ; The short/echo end tag.

; the compiled regex patterns for each of the above
(def tag-pattern (re-pattern (str tag-start "((?s:.+?))" tag-end)))
(def echo-pattern (re-pattern (str echo-start "((?s:.+?))" echo-end)))

(def buffer (StringBuilder.))

(defn- echo [string buffer][string]
  "A small helper function for echoing things to a buffer."
  (.append buffer string))

(defn- process-match [[match group]]
  "Processes the given match, and returns the output of the given match."
  (let [buffer (StringBuilder.)]
    (load-string (str "(use 'closeddoor.core)" group))
    (clojure.string/replace (.toString buffer) "$" "\\$")))

(defn- process-match-echo-wrapped [[match group]]
  "Process the given match, but wraps it first in the echo function."
  (process-match [match (str "(echo " group " buffer)")]))

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (clojure.string/replace
    (clojure.string/replace input tag-pattern process-match)
    echo-pattern
    process-match-echo-wrapped))

(defn process [source out]
  "Processes the given source, which means that it reads everything from
   the source with slurp, runs it through parse and spits it out into out.
   So (process input *out*) is basically shorthand for (spit *out* (parse input))."
  (spit out (parse (slurp source))))

(defn -main [& args]
  "The main function which does everything."
  (if (empty? args)
    (process *in* *out*)
    (doseq [arg args]
      (if (= arg "-")
        (process *in* *out*)
        (process arg *out*)))))

If I can offer one last criticism: your methods that deal with modifying buffer (which is initially defined as (StringBuilder.) are destructive actions, so you might consider adding ! onto the end of your function names (echo!, process-match!, etc.)

I noticed that you have some def and defn statements within the function definition of process-match. It's generally considered un-idiomatic to use def/defn anywhere except for at the top level of the namespace you're in. It looks like buffer is only used within the process-match function, so you should use let instead. Your echo function seems to be used elsewhere, so I would pull that out and define it as a private function at the top level; now, the echo function depends on buffer being defined, so what you'll need to do is add a buffer argument for the echo function, and then pass in the appropriate StringBuffer object whenever you need to use it. I would consider rearranging your code to look something like this:

(ns closeddoor.core
  (:require clojure.string)
  (:gen-class :main true))

(def tag-start "<\\?clj")  ; The long/normal start tag.
(def tag-end "(\\?>|\\Z)") ; The long/normal end tag.

(def echo-start "<%") ; The short/echo start tag.
(def echo-end "%>")   ; The short/echo end tag.

; the compiled regex patterns for each of the above
(def tag-pattern (re-pattern (str tag-start "((?s:.+?))" tag-end)))
(def echo-pattern (re-pattern (str echo-start "((?s:.+?))" echo-end)))

(defn- echo [string buffer]
  "A small helper function for echoing things to a buffer."
  (.append buffer string))

(defn- process-match [[match group]]
  "Processes the given match, and returns the output of the given match."
  (let [buffer (StringBuilder.)]
    (load-string (str "(use 'closeddoor.core)" group))
    (clojure.string/replace (.toString buffer) "$" "\\$")))

(defn- process-match-echo-wrapped [[match group]]
  "Process the given match, but wraps it first in the echo function."
  (process-match [match (str "(echo " group " buffer)")]))

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (clojure.string/replace
    (clojure.string/replace input tag-pattern process-match)
    echo-pattern
    process-match-echo-wrapped))

(defn process [source out]
  "Processes the given source, which means that it reads everything from
   the source with slurp, runs it through parse and spits it out into out.
   So (process input *out*) is basically shorthand for (spit *out* (parse input))."
  (spit out (parse (slurp source))))

(defn -main [& args]
  "The main function which does everything."
  (if (empty? args)
    (process *in* *out*)
    (doseq [arg args]
      (if (= arg "-")
        (process *in* *out*)
        (process arg *out*)))))

I noticed that you have some def and defn statements within the function definition of process-match. It's generally considered un-idiomatic to use def/defn anywhere except for at the top level of the namespace you're in. Usually you would use let instead if what you're trying to define isn't used outside of the function, but it doesn't look like that's the case here -- I think you are wanting buffer and echo to be used at the top level, so I would pull them out of the process-match function and do something like this:

(ns closeddoor.core
  (:require clojure.string)
  (:gen-class :main true))

(def tag-start "<\\?clj")  ; The long/normal start tag.
(def tag-end "(\\?>|\\Z)") ; The long/normal end tag.

(def echo-start "<%") ; The short/echo start tag.
(def echo-end "%>")   ; The short/echo end tag.

; the compiled regex patterns for each of the above
(def tag-pattern (re-pattern (str tag-start "((?s:.+?))" tag-end)))
(def echo-pattern (re-pattern (str echo-start "((?s:.+?))" echo-end)))

(def buffer (StringBuilder.))

(defn- echo [string]
  "A small helper function for echoing things."
  (.append buffer string))

(defn- process-match [[match group]]
  "Processes the given match, and returns the output of the given match."
  (load-string (str "(use 'closeddoor.core)" group))
  (clojure.string/replace (.toString buffer) "$" "\\$"))

(defn- process-match-echo-wrapped [[match group]]
  "Process the given match, but wraps it first in the echo function."
  (process-match [match (str "(echo " group ")")]))

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (clojure.string/replace
    (clojure.string/replace input tag-pattern process-match)
    echo-pattern
    process-match-echo-wrapped))

(defn process [source out]
  "Processes the given source, which means that it reads everything from
   the source with slurp, runs it through parse and spits it out into out.
   So (process input *out*) is basically shorthand for (spit *out* (parse input))."
  (spit out (parse (slurp source))))

(defn -main [& args]
  "The main function which does everything."
  (if (empty? args)
    (process *in* *out*)
    (doseq [arg args]
      (if (= arg "-")
        (process *in* *out*)
        (process arg *out*)))))

If I can offer one last criticism: your methods that deal with modifying buffer (which is initially defined as (StringBuilder.) are destructive actions, so you might consider adding ! onto the end of your function names (echo!, process-match!, etc.)

Source Link
Dave Yarwood
  • 1.1k
  • 6
  • 13

Cool idea! I could definitely see this coming in handy in a variety of domains.

I noticed that you have some def and defn statements within the function definition of process-match. It's generally considered un-idiomatic to use def/defn anywhere except for at the top level of the namespace you're in. It looks like buffer is only used within the process-match function, so you should use let instead. Your echo function seems to be used elsewhere, so I would pull that out and define it as a private function at the top level; now, the echo function depends on buffer being defined, so what you'll need to do is add a buffer argument for the echo function, and then pass in the appropriate StringBuffer object whenever you need to use it. I would consider rearranging your code to look something like this:

(ns closeddoor.core
  (:require clojure.string)
  (:gen-class :main true))

(def tag-start "<\\?clj")  ; The long/normal start tag.
(def tag-end "(\\?>|\\Z)") ; The long/normal end tag.

(def echo-start "<%") ; The short/echo start tag.
(def echo-end "%>")   ; The short/echo end tag.

; the compiled regex patterns for each of the above
(def tag-pattern (re-pattern (str tag-start "((?s:.+?))" tag-end)))
(def echo-pattern (re-pattern (str echo-start "((?s:.+?))" echo-end)))

(defn- echo [string buffer]
  "A small helper function for echoing things to a buffer."
  (.append buffer string))

(defn- process-match [[match group]]
  "Processes the given match, and returns the output of the given match."
  (let [buffer (StringBuilder.)]
    (load-string (str "(use 'closeddoor.core)" group))
    (clojure.string/replace (.toString buffer) "$" "\\$")))

(defn- process-match-echo-wrapped [[match group]]
  "Process the given match, but wraps it first in the echo function."
  (process-match [match (str "(echo " group " buffer)")]))

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (clojure.string/replace
    (clojure.string/replace input tag-pattern process-match)
    echo-pattern
    process-match-echo-wrapped))

(defn process [source out]
  "Processes the given source, which means that it reads everything from
   the source with slurp, runs it through parse and spits it out into out.
   So (process input *out*) is basically shorthand for (spit *out* (parse input))."
  (spit out (parse (slurp source))))

(defn -main [& args]
  "The main function which does everything."
  (if (empty? args)
    (process *in* *out*)
    (doseq [arg args]
      (if (= arg "-")
        (process *in* *out*)
        (process arg *out*)))))

BTW, I also changed your spacing from 4 spaces to 2 which is the Clojure standard. I guess there's nothing wrong with using 4 spaces, but 2 is predominantly what you see.

I've generally rearranged the spacing and line breaks to suit my own aesthetic preference, but it isn't necessarily "the right way," just my opinion on what formatting I find more readable. Just my 2 cents!

I also moved the (require 'clojure.string) into the namespace definition, which is idiomatic and more concise.

I'm a bit confused about your parse function... did you mean for the last argument of each call to clojure.string/replace function to be a function (process-match and process-match-echo-wrapped, respectively)? I believe you need to have strings there, indicating what to replace each match with. Maybe you meant (process-match match group), where match and group are replaced with the actual match and group? Maybe I'm missing something.

By the way, if you wanted to simplify the nested calls to clojure.string/replace in that function, you could do something like this:

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (reduce (fn [s [pat rep]] 
            (clojure.string/replace s pat rep))
          input
          [[tag-pattern process-match]
           [echo-pattern pattern-match-echo-wrapped]]))

Hope this helps!