2
\$\begingroup\$

This is a problem from Exercism here, the goal is to clean up badly formatted phone numbers. The rules are:

  • If the phone number is less than 10 digits assume that it is a bad number
  • If the phone number is 10 digits assume that it is good
  • If the phone number is 11 digits and the first number is 1, trim the 1 and use the first 10 digits
  • If the phone number is 11 digits and the first number is not 1, then it is a bad number
  • If the phone number is more than 11 digits assume that it is a bad number

A function should pretty-print the numbers in the form (123) 456-7890.

Here is my solution. I'm very new to Clojure and am sure it's not idiomatic in parts. For example, should I have stored the different parts of the number as separate variables?

I would also be interested in tips on how to format the code to make it easier to read, I'm only aware of lispy-multiline which I find a bit too aggressive.

All feedback much appreciated.

(ns phone-number)

(defn digit-not-one-or-zero [digit]
  (and (not (= \0 digit))
       (not (= \1 digit))))

(defn clean-number [num-string]
  ;; strip all non-digit characters from a string
  (filter #(Character/isDigit %) num-string))

(defn check-valid [num-string]
  ;; number is valid only if:
  ;; a) length is between 10-11 chars
  ;; b) if 11 chars, first digit must be 1
  ;; c) (after stripping first digit if 11), first and fourth can't be 0 or 1
  (let [clean-string (clean-number num-string)
        len (count clean-string)]
    (if (or (< len 10) (> len 11))
      false
      (if (= len 11)
        (if (= \1 (first clean-string))
          (check-valid (rest clean-string))
          false)
        (if (and (digit-not-one-or-zero (first clean-string))
                 (digit-not-one-or-zero (nth clean-string 3)))
          true
          false)))))

(defn number [num-string]
  (if (check-valid num-string)
    (let [clean-string (clean-number num-string)]
      (if (= (count clean-string) 10)
        (apply str (clean-number clean-string)) (apply str (rest (clean-number clean-string)))))
    "0000000000"))

(defn area-code [num-string]
  ;; get the first 3 chars of the cleaned up number
  (apply str (take 3 (number num-string))))

(defn pretty-print [num-string]
  (let [formatted-number (number num-string)]
    (apply str (concat
                "(" (area-code num-string) ")" " " (subs formatted-number 3 6) "-" (subs formatted-number 6)))))
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I suggest you to look at Clojure Cheatsheet and get familiar with the functions dedicated for working with strings. There is also the library clojure.string, containing a few more. With these functions, you won't have to convert your string into sequence and then back repeatedly.

Take for example your pretty-print- subs returns string, concat returns sequence, apply str returns string again. Pick the correct data structure at the beginning, use dedicated functions and convert as little as possible.

Use :require in ns to include clojure.string library:

(ns phone-number
  (:require [clojure.string :as s])
  (:gen-class))

I will start with the area-code. This function could be improved with ->> (thread-last macro) and clojure.string/join like this:

(defn area-code [num-string]
  (->> (number num-string)
       (take 3)
       s/join))

But you can also use subs:

(defn area-code [num-string]
  (subs (number num-string) 0 3))

Next, pretty-print. You can use format here:

(defn pretty-print [num-string]
  (let [validated (number num-string)]
    (format "(%s) %s-%s" (area-code validated)
            (subs validated 3 6)
            (subs validated 6))))

Next, digit-not-one-or-zero. You usually create a set with these elements and call it on your argument, something like:

(defn digit-not-one-or-zero [digit]
  (not (#{\0 \1} digit)))

Or even shorter:

(def digit-not-one-or-zero (complement #{\0 \1}))

Your clean-number is ok (I would just add s/join at the end to ensure it will return a string, as any other function here), unless you want to use some regex:

(defn clean-number [num-string]
  (s/replace num-string #"\D" ""))

Next, number and check-valid.

  • Don't use nested ifs- learn about cond, condp and case and use these. When can be also useful here.
  • Don't use (if ... true false). In Clojure, all values are logically true or false. The only "false" values are false and nil - all other values are logically true. So, take this example:
(if (and (digit-not-one-or-zero (first clean-string))
         (digit-not-one-or-zero (nth clean-string 3)))
  true
  false)

You can just write:

(and (digit-not-one-or-zero (first clean-string))
     (digit-not-one-or-zero (nth clean-string 3)))
  • check-valid is too long- you should break it into smaller functions.

So, here is my version you can study. Note how using short functions, idioms and proper flow-control macros increased readability:

(ns phone-number
  (:require [clojure.string :as s])
  (:gen-class))

(def allowed-digits (complement #{\0 \1}))

(defn clean-number [num-string]
  (s/replace num-string #"\D" ""))
(defn validate10 [s]
  (and (allowed-digits (.charAt s 0))
       (allowed-digits (.charAt s 3))
       s))

(defn validate11 [s]
  (when (= \1 (.charAt s 0))
    (validate10 (subs s 1))))
(defn validate [num-string]
  (condp = (count num-string)
    11 (validate11 num-string)
    10 (validate10 num-string)
    false))

(defn number [num-string]
  (if-let [valid (validate (clean-number num-string))]
    valid
    "0000000000"))

(defn area-code [num-string]
  (subs (number num-string) 0 3))

(defn pretty-print [num-string]
  (let [validated (number num-string)]
    (format "(%s) %s-%s" (area-code validated)
            (subs validated 3 6)
            (subs validated 6))))

EDIT: Alternative solution with regexes:

(defn number [num-string]
  (if-let [valid (->> (s/replace num-string #"\D" "")
                      (re-matches #"1?([^01]..[^01].{6})")
                      second)]
    valid
    "0000000000"))

(defn area-code [num-string]
  (subs (number num-string) 0 3))

(defn pretty-print [num-string]
  (s/replace (number num-string)
             #"(\d{3})(\d{3})(\d{4})"
             "($1) $2-$3"))
\$\endgroup\$
1
  • \$\begingroup\$ Thanks so much, this is all really helpful. Do you have any suggestions for rules for formatting? \$\endgroup\$ Dec 18, 2022 at 19:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.