Ttykuu

This semester a classmate and I visited a course on functional programming in our university. For grading, we've to submit a small project which should be written in Clojure and make use of functional programming.
However, the topic of the course itself was mostly about Clojure and its syntax and not so much about functional programming itself. That's why we're not quite sure if our code complies with functional programming paradigms.

It would be highly appreciated if someone takes a look at our code to point out flaws and possible violations of functional programming.
If you see anything that could be written simpler or seems confusing to you, please point it out, since we're new to Clojure and not sure if we've done everything correctly or in the most efficient way (probably not).

Following below is the main part of our code. The entire project can be found in our Gitlab repository.
Since it's a web app we've used hiccup, ring and compojure.
The main functionalities are a landing page at /, which lets you either put in a RSS Feed url or select one RSS Feed from categories provided by us.
Once you submit you're routed to /feed. Here we render the separate items of the current feed.
You've the possibility to filter the feed by typing something, like 'Trump' and as a result we only show the feed items that include the word 'Trump' either in the title or the description. Additionally those "matches" are afterwards highlighted like so:

Furthermore, there's the possibility to filter by author. All authors of the current feed are available as an option in a select box.

Code
feed.clj

 (ns rss-feeds-webapp.feed
 (:require
 [compojure.core :refer :all]
 [ring.middleware.defaults :refer [wrap-defaults site-defaults]]
 [clj-http.client :as client]
 [clojure.java.io :as io]
 [clojure.xml :as xml]
 [clojure.string :as string])
 (:import [java.io ByteArrayInputStream])
 (:use
 [rss-feeds-webapp.helper :refer :all]
 ))

;; gets the feed from the url, then parses it to an byte array and then to an xml sequenze
(defn get-feed [link]
 (xml-seq (xml/parse (io/input-stream (ByteArrayInputStream. (.getBytes (:body (client/get link)) "UTF-8"))))) )

;; returns the content of the xml tag
;; @param [item] xml-seq the post item which contains the title, description and link7
;; @param [tag] key the tag, that contains the information you want, e.g. title
(defn get-content [item tag]
 (for [x item
 :when (= tag (:tag x))]
 (first (:content x))))

;; creates an hash-map for the feed item, containing the title, description, link, creator and pubDate (if it exists)
;; param [item] xml-seq the post item,
(defn get-item [item]
 :title (get-content item :title )
 :description (get-content item :description)
 :link (get-content item :link)
 :pubDate (get-content item :pubDate)
 :image (get-content item :image)
 :creator (get-content item :dc:creator)
 
 )

;; creates an hash-map for the feed item, containing the creator
;; param [item] xml-seq the post item,
(defn get-author [item]
 :creator (get-content item :dc:creator)
 )

;; finds all feed items and calls the get-item function for each one
;; @param [item] xml-seq the post item which contains the title, description, creator and link
(defn get-items [ return-items link]
 (for [x (get-feed link)
 :when (= :item (:tag x))]
 (conj return-items(get-item (:content x)))))

;; finds all feed items and calls the get-author function for each one
;; @param [item] xml-seq the post item which contains the creator
(defn get-authors [ return-authors link]
 (for [x (get-feed link)
 :when (= :item (:tag x))]
 (conj return-authors(get-author (:content x)))))

;; we define a default Value to filterTerm in case it's null
;; then we filter the items so that either the title or the description of the feed contain the filter term and the author equals the authorTerm
(defn filter-items-by-term [term author]
 (let [term (or term "")]
 (let [author (or author "")]
 (fn [[item]]
 (and (or
 (string/includes?
 (simplify-string (apply str (get item :title)))
 (simplify-string term))
 (string/includes?
 (simplify-string (apply str (get item :description)))
 (simplify-string term))
 )
 (string/includes?
 (simplify-string (apply str (get item :creator)))
 (simplify-string author))
 )))))


;; calls the get-items function, in the end it returns an array with all the items
;; items get filtered by filter term
(defn get-feed-data [link & [filterTerm authorTerm]]
 (filter (filter-items-by-term filterTerm authorTerm) (get-items [] link))
 )

;; calls the get-authors function, in the end it returns an array with all authors
(defn get-feed-authors [link]
 (distinct (get-authors [] link)))

;; gets the feed title
(defn get-feed-title [link]
 (first (for [x (get-feed link)
 :when (= :title (:tag x))]
 (first (:content x)))))

helper.clj

(ns rss-feeds-webapp.helper
 (:require
 [clojure.string :as string]))

;; parses a date string into our desired output format
(defn get-time [date-string]
 (.format (java.text.SimpleDateFormat. "dd/MM/yyyy - HH:mm") (new java.util.Date date-string))
 )

;; removes all trailing whitespaces and converts the string to lowercase
;; --> needed for accurate comparison
(defn simplify-string [str]
 (let [str (or str "")]
 (string/lower-case (string/trim str))))

;; takes one string, analyzes it for regex matches of the filterTerm and returns a list with html spans
;; apply list is needed so the lazy sequence gets converted to a list for sequential access
(defn get-highlighted-text-by-key [item filterTerm key]
 (let [title []
 match (re-matches (re-pattern (str "(?i)(.*)(" filterTerm ")(.*)"))(apply str (get item key)))]
 (if match
 ;; drops the first element of the match vector, since that's the whole string
 (let [nested-title (for [substr (drop 1 match)]
 (if (= (simplify-string substr) (simplify-string filterTerm))
 [:span :class "highlight" substr]
 [:span substr]
 )
 )]
 (apply list (conj title (apply list nested-title) )))
 (apply list (conj title [:span (get item key "No title specified")])))))

;; checks if the option value matches the link parameter and returns true
(defn get-selected-option [value filterTerm]
 (if (= value filterTerm) true false))

templating.clj

(ns rss-feeds-webapp.templating
 (:use
 [hiccup.page :only (html5 include-css include-js)]
 [hiccup.form :refer :all]
 [rss-feeds-webapp.helper :refer :all]
 [rss-feeds-webapp.feed :refer :all]
 [ring.util.anti-forgery :as util]
 )
 )

(defn gen-page-head
 [title]
 [:head
 [:title (str title)]
 (include-css "https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0/css/bootstrap.min.css")
 (include-css "/css/styles.css")])



(defn render-item [item filterTerm]
 [:div :class "card"
 ;;[:img :class "card-img-left" :src "..." :alt "Card image cap"]
 [:div :class "card-body"
 [:h5 (get-highlighted-text-by-key item filterTerm :title)]
 [:p :class "card-text" (get item :creator "No author specified")]
 [:p :class "card-text" (get-highlighted-text-by-key item filterTerm :description)]
 [:p :class "card-date" (get-time (apply str(get item :pubDate "PubDate"))) ]
 [:a :class "btn btn-outline-info" :href (apply str(get item :link)) :target "_blank" "Read full article"]
 ]]
 )

(defn render-author-options [item filterTerm]
 [:option :value (apply str(get item :creator "No title specified")) :selected (get-selected-option (apply str(get item :creator )) filterTerm )
 (get item :creator "No title specified")]
 )

(defn render-authors [content filterTerm]
 (for [entry content]
 (render-author-options (first entry) filterTerm)))

(defn render-feed [content filterTerm]
 (for [entry content]
 (render-item (first entry) filterTerm)))

(defn render-category-selection []
 [:div :class "container margin-top-25 no-left-padding"
 [:div :class "col-md-12 no-left-padding"
 [:h3 "Or select a category"]
 [:form :method "get" :action "/feed" :class "category-form"
 (util/anti-forgery-field)
 [:div :class "col-lg-8 col-sm-12 no-left-padding no-right-padding"
 [:div :class "input-group"
 [:div :class "select-wrapper"
 [:select :class "custom-select form-control no-right-border-radius" :name (str "link")
 [:option :value "https://www.nytimes.com/services/xml/rss/nyt/World.xml" "World News"]
 [:option :value "http://feeds.bbci.co.uk/news/world/europe/rss.xml" "Europe"]
 [:option :value "http://feeds.nytimes.com/nyt/rss/Business" "Business"]
 [:option :value "http://feeds.bbci.co.uk/news/health/rss.xml" "Health"]
 [:option :value "http://feeds.nytimes.com/nyt/rss/Technology" "Technology"]
 [:option :value "http://rss.nytimes.com/services/xml/rss/nyt/Travel.xml" "Travel"]
 [:option :value "http://feeds.bbci.co.uk/news/education/rss.xml" "Education & Family"]
 [:option :value "https://www.nytimes.com/services/xml/rss/nyt/Sports.xml" "Sports"]
 [:option :value "http://feeds.feedburner.com/vagubunt" "Linux & Webdevelopment"]
 [:option :value "https://www.sitepoint.com/feed/" "Programming"]
 [:option :value "https://css-tricks.com/feed" "CSS-Tricks"]
 [:option :value "https://clojureverse.org/posts.rss" "Clojure"]
 ]
 ]
 [:span :class "input-group-btn"
 [:input.action :type "submit" :class "btn btn-outline-secondary no-left-border-radius" :value "Load selected category"]]]]]]]
 )

(defn render-possible-feeds []
 [:div :class "container no-left-padding" :style "margin-top: 50px"
 [:div :class "col-md-12 no-left-padding"
 [:h5 "Possible Feeds"]
 [:div :class "col-md-12 no-left-padding"
 [:p "http://feeds.feedburner.com/vagubunt"]
 [:p "http://www.spiegel.de/schlagzeilen/tops/index.rss"]
 [:p "http://rss.cnn.com/rss/cnn_topstories.rss"]
 [:p "http://feeds.bbci.co.uk/news/world/europe/rss.xml"]
 [:p "https://www.nytimes.com/services/xml/rss/nyt/HomePage.xml"]
 [:p "https://www.nytimes.com/services/xml/rss/nyt/World.xml"]
 [:p "http://feeds.nytimes.com/nyt/rss/Technology"]
 [:p "http://www.tagesschau.de/xml/rss2"]
 [:p "https://spectrum.ieee.org/rss/fulltext"]
 ]]]
 )

(defn body [title & content]
 (html5
 (gen-page-head title)
 [:body
 [:div :class "container" :style "position: relative"
 [:a :href "/" :title "Go back home"
 [:img :class "site-logo" :src "http://tiny.cc/rdun3y" :alt "Card image cap"]]
 [:h1 :class "margin-vertical-20" title]
 content
 ]]))

;; renders an input group with an input field and an action button to the right
(defn render-input-group [inputPlaceholder, actionText, inputName, inputType]
 [:div :class "input-group"
 [:input :type inputType :class "form-control" :name inputName :placeholder inputPlaceholder]
 [:span :class "input-group-btn"
 [:input.action :type "submit" :class "btn btn-primary no-left-border-radius" :value actionText]]]
 )

;; HTML Page for /
(defn view-input []
 (body
 "RSS-Feed Viewer"
 [:div
 [:div :class "container margin-top-25 no-left-padding"
 [:div :class "col-md-12 no-left-padding"
 [:h3 "Enter a rss-feed url:"]
 [:form :method "get" :action "/feed"
 (util/anti-forgery-field)
 [:div :class "col-lg-6 no-left-padding no-right-padding"
 (render-input-group "Enter your RSS Url..." "load feed" "link" "url")
 ]
 ]
 ]]
 (render-category-selection)
 (render-possible-feeds)]
 ))

;; HTML Page for /view-error
(defn view-error []
 (body
 "RSS-Feed Viewer"
 [:div
 [:div :class "container margin-top-25 no-left-padding"
 [:div :class "col-md-12 no-left-padding"
 [:h3 :class "error" "Ups, something went wrong..."]

 ]]]
 ))

;; HTML Page for /feed
(defn view-feed [link, filter, author]
 (body
 (get-feed-title link)
 [:div :class "container no-left-padding no-right-padding"
 [:form :method "get" :action "/feed"
 (util/anti-forgery-field)
 [:div :class "col-lg-6 no-left-padding no-right-padding"
 [:div :class "input-group hidden"
 [:input :type "url" :class "form-control" :name "link" :placeholder "..." :value link]
 [:span :class "input-group-btn"
 [:input.action :type "submit" :class "btn btn-primary no-left-border-radius" :value "Go"]]]
 [:div :class "input-group"
 [:input :type "text" :class "form-control" :autofocus true :name "filter" :placeholder "Filter RSS Feeds..." :value filter]
 [:span :class "input-group-btn"
 [:input.action :type "submit" :class "btn btn-primary no-left-border-radius" :value "Filter feeds"]]]
 ]
 [:div :class "col-lg-8 col-sm-12 no-left-padding no-right-padding"
 [:div :class "input-group margin-top-25"
 [:div :class "select-wrapper"
 [:select :class "custom-select form-control no-right-border-radius" :name (str "author")
 [:option :value "" :selected (get-selected-option "" author ) "--- choose author ---" ]
 (render-authors (get-feed-authors link) author)]
 ]
 [:span :class "input-group-btn"
 [:input.action :type "submit" :class "btn btn-outline-secondary no-left-border-radius" :value "Filter authors"]]]]]
 ]
 (render-feed (get-feed-data link filter author) filter)
 ))

Thanks a lot for the thorough explanation and analysis!

I tried your suggestion for updating the 'get-highlighted-text-by-key' method, unfortunately, it didn't work right away though. I managed to make it work like so:

(defn get-highlighted-text-by-key [item filterTerm key]
 (if-let [match (re-matches (re-pattern (str "(?i)(.*)(" filterTerm ")(.*)"))
 (apply str (get item key)))]
 ;; drops the first element of the match vector, since that's the whole string
 (let [nested-title (for [substr (drop 1 match)]
 (if (= (simplify-string substr) (simplify-string filterTerm))
 [:span :class "highlight" substr]
 [:span substr]
 )
 )]
 nested-title)
 [:span (get item key "No title specified")]))

This is also way simpler and makes more sense. When I wrote the method I had the feeling that something is off, but I couldn't quite wrap my head around it. So thanks for clearing that up!

This code is quite good for someone learning the language. You haven't made any of the common errors such as using def inside of function instead of let. I don't see anything really outright wrong with the code, so I'll just be commenting on style and things that I think can be improved. I'll just go top to bottom after addressing some things you mentioned.

. . . we're not quite sure if our code complies with functional programming paradigms.

Yes, this code is functional. You aren't needlessly mutating anything by abusing atoms or relying on side-effects. Really, those are major deciding factors.

(:use [rss-feeds-webapp.helper :refer :all])

Shouldn't be there. That should be tucked inside the :require with everything else. Use of :use is discouraged, as is :refer :all in many cases. Blanket unqualified imports of namespaces aren't ideal. Say you come back to this project a year from now. Are you going to remember what functions come from what modules? Unless the names provide enough context, you may have trouble. Bulk unqualified imports also increase the chance of having a name conflict. Always try to use :as, or :refer [...] when importing. That way you can easily see what code is coming from where, and avoid polluting your namespace.

(.getBytes (:body (client/get link)) "UTF-8")

That would benefit from using a type hint.

; I'm assuming :body is returning a String
(ByteArrayInputStream. ^String (.getBytes (:body (client/get link)) "UTF-8"))

Not only does that help a smart IDE like IntelliJ give you better autocomplete suggestions, it also makes your code faster by avoiding reflection. If you run lein check, your call to .getBytes will cause the following warning:

Reflection warning, your-file-here.clj - reference to field getBytes can't be resolved.

Avoiding reflection isn't a big deal in this case, but it's a good thing to keep in mind.

get-content and similar functions make good use of for. I'll just point out though that they can also be written in terms of map and filter as well:

(defn get-content [item tag]
 (->> item
 (filter #(= tag (:tag %)))
 (map #(first (:content %)))))

; Or

(defn get-content [item tag]
 (->> item
 (filter #(= tag (:tag %)))
 (map (comp first :content))))

If you already have a ->> "pipe" going, it may prove to be cleaner to use map/filter here instead of for. This is purely a matter of taste though.

(defn filter-items-by-term [term author]
 (let [term (or term "")]
 (let [author (or author "")]

contains needless nesting. It can simply be

(defn filter-items-by-term [term author]
 (let [term (or term "")
 author (or author "")]

A lot of your code (like in the example immediately above), you're doing something like

(or some-parameter a-default-value)

Now, this is a good way of dealing with possibly-nil values. The placement of the check is weird though. Take a look at filter-items-by-term. Why are term and author possibly nil? Because get-feed-data takes optional parameters, and passes the data, unchecked, to filter-items-by-term. This means that part of the implementation of filter-items-by-term (checking for nil values) is needed due to the implementation of get-feed-data (passing potentially nil values). What if you change how one of the functions work and forget to change the other? It also seems needlessly complicated that many of your functions are trying to "protect themselves" by assuming that bad data may be handed in. It would be much cleaner to expect that the caller, when possible, checks the data prior to calling. All your functions should expect valid data. If the caller has bad data, it's up to them to fix it. I'd make the following changes:

(defn filter-items-by-term [term author]
 ; Assumes valid data is being passed!
 (fn [[item]]

(defn get-feed-data [link & [filterTerm authorTerm]]
 ; It's now this function's duty to verify its own data
 (filter (filter-items-by-term (or filterTerm "")
 (or authorTerm ""))
 (get-items [] link)))

And similarly for the other cases like this.

I also cleaned up get-feed-data. It had two things that I didn't like:

• Trailing ) on a new line, like you would have a } in Java. This isn't necessary if you're using a good IDE and consistent indentation. It seems like one of you is using this style while the other isn't, since it's inconsistent. Even worse than using an unidiomatic style is applying the style inconsistently. Your teacher will mark you down for inconsistency alone if they're paying attention.




• Trying to shove a bunch on one line. It's much better to break up long lines for readability.

Also in that same function, you have the long anonymous function being returned. In it, you repeatedly access item using keys. It might be cleaner to deconstruct item in the parameter list:

(fn [[:keys [title description creator]]]
 (and (or
 (string/includes?
 (simplify-string (apply str title))
 (simplify-string term))
 (string/includes?
 (simplify-string (apply str description))
 (simplify-string term)))

 (string/includes?
 (simplify-string (apply str creator))
 (simplify-string author))))

Also note, (get item :title) can be written simply as (:title item). Keywords implement IFn:

(ifn? :hello)
=> true

They are usable as functions that return the corresponding element when applied to a map.

get-feed-data could also make use of partial. You're having the function return another function so you can supply some data ahead of time, then have filter call it with the last bit of data. This is very common, and is the situation that partial was made for. Id make the following changes:

; Note how "item" was simply added as a third parameter
(defn filter-items-by-term [term author :keys [title description creator]]
 ; And now a function isn't returned
 (and (or
 (string/includes?
 (simplify-string (apply str title))
 (simplify-string term))
 (string/includes?
 (simplify-string (apply str description))
 (simplify-string term)))

 (string/includes?
 (simplify-string (apply str creator))
 (simplify-string author))))

And now:

(defn get-feed-data [link & [filterTerm authorTerm]]
 ; Partially apply "filterTerm" and "autherTerm" to "filter-items-by-term"
 (filter (partial filter-items-by-term filterTerm authorTerm) 
 (get-items [] link)))

or, just manually wrap it in another function (#() here):

(defn get-feed-data [link & [filterTerm authorTerm]]
 (filter #(filter-items-by-term filterTerm authorTerm %)
 (get-items [] link)))

partial may seem complicated at first, but this example should make it clearer:

(let [add-five (partial + 5)]
 (println (add-five 5) (add-five 20)))

10 25

It returns a function that expects the rest of the arguments. This is useful when you have some data right now, and want to call the function with some other data later.

Why would I make this change? Why should filter-items-by-term care about how it's being used? Why should it need to know that the caller needs to supply some of the data later? It shouldn't.

Speaking of long lines, I'd break up the get-time body:

(defn get-time [date-string]
 (.format (java.text.SimpleDateFormat. "dd/MM/yyyy - HH:mm") 
 (new java.util.Date date-string)))

Align everything properly based on indentation (like you would in Python), and Par-infer (Parenthesis Inference) can automatically handle closing braces for you. I never manually add )s when writing Clojure. IntelliJ's Cursive plugin (both free) includes Par-Infer which infers where they should be and adds them for me. I highly recommend this set-up if you plan on writing Clojure.

get-selected-option is redundant and has a confusing name. It isn't actually "getting" anything, it's a predicate. It also doesn't make sense to write (if pred? true false). The predicate already returns true or false (or at least a truthy/falsey value), so the if here isn't needed. I'd change it to:

(defn is-filter-term? [value filterTerm]
 (= value filterTerm))

Although it could be argued that this function is so simple that it should just be inlined. It's pretty clear what a simple call to = is checking.

For render-authors and the function below it, I'd just use map here. Especially since you don't need any filtering or use of :let, map would likely be cleaner:

(defn render-authors [content filterTerm]
 (map #(render-author-options (first %) filterTerm) 
 content))

But again, this a matter of taste. I will say though, if you have a iterable collection like content, it's a good idea to match Clojure convention and have it as the last parameter of the function. That way, you can do something like:

(->> content
 (map some-transformation)
 (render-authors some-filter-term) ; Wouldn't be possible with your current order
 (filter some-predicate))

Threading macros (-> and ->> mainly) are pretty common, and writing functions that work well with them will make your life easier. You didn't use and threading macros in your code, but I also don't see any good opportunities to use them either. I'd practice using them if you intend to keep writing Clojure, as they're exceedingly helpful.

In get-highlighted-text-by-key, there's a few notable things.

One, I'm not sure title is necessary here. It's needed in two places, and neither of those places seem to even need it. Unless my brain is just fried, (conj title (apply list nested-title)) is just conjoining (apply list nested-title) to an empty vector, which will just result in a single element vector. It would make more sense to just write:

(vector (apply list nested-title))

I also don't understand why you then wrap that in a (apply list, which just converts it to a list. You convert nested-title to a list with the first (apply list, then wrap it in a vector with (conj [], then convert the vector to a list with a second (apply list I'm not sure what you're trying to do here. It looks like that entire line could be reduced to just:

(list (apply list nested-title))

Although, I'll note that that plain lists aren't used very often unless you're writing macros. Lists are simple linked lists, so they can't do O(1) lookups by index. Use vectors in the vast majority of cases unless you have good reason to use something else (like you're writing a macro). Simply returning the lazy list that for evaluates to would likely be fine here. The caller can force the result if they want to.

Second,

(let [match (re-matches (re-pattern (str "(?i)(.*)(" filterTerm ")(.*)"))(apply str (get item key)))]
 (if match

(Besides being way too long of a line) is a perfect case to make use of if-let:

(if-let [match (re-matches (re-pattern (str "(?i)(.*)(" filterTerm ")(.*)"))(apply str (get item key)))]
 (... If truthy, do this, using "match" ...)
 (... If falsey, do this, without having access to "match")

if-let checks if the bound variable is truthy. If it is, it executes the first body with the truthy binding in scope. If it's falsey, it executes the second body, without the binding in scope.

I think that's all the major things I can find. Good luck. Hopefully this was helpful.