4
\$\begingroup\$

This is a simple LISP program to read in three sides of a triangle and report what kind of triangle it is (or isn't). Any feedback would be much appreciated.

(defun read-side (side) (format t "Enter a value for side ~d: ~%" side) (read))

(defun is-a-triangle (a b c) (if (and (< a (+ b c)) (< b (+ a c)) (< c (+ a b))) t nil))

(defun is-equilateral (a b c) (if (and (= a b c)) t nil))

(defun is-isoceles (a b c) (if (or (= a b) (= b c)) t nil))

(defun is-right-triangle (a b c) 
  (let ((square-of-short-sides-sum 0) (short-sides ()) (large-side 0) (sides (list a b c))) 
    (loop for p in sides do 
      (if (> p large-side) 
        (progn (if (/= 0 large-side) (setq short-sides (append short-sides (list large-side)))) 
        (setq large-side p))
        (setq short-sides (append short-sides (list p)))))
    (format t "short-sides: ~a ~%" short-sides)
    (format t "large-side: ~a ~%" large-side)
    (loop for p in short-sides do 
      (setq square-of-short-sides-sum (+ square-of-short-sides-sum (expt p 2))))
    (if (= (expt large-side 2) square-of-short-sides-sum) t nil)))

(defun status-message (message a b c) (format t "The shape ~d,~d,~d ~a ~%" a b c message))

(let ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)))
  (status-message "is being checked..." a b c)
  (if (is-equilateral a b c) (status-message "is an equilateral triangle." a b c)
      (if (is-a-triangle a b c)
        (progn (if (is-isoceles a b c) (status-message "is an isoceles triangle." a b c)
            (if (is-right-triangle a b c) (status-message "is a right triangle." a b c)
                (status-message "is a non-isoceles, non-equilateral, non-right triangle." a b c))))
        (status-message "is not a triangle." a b c))))

Thanks to the previous commenters for your suggestions. I have since modified the code based on the feedback I have received and am posting the revised code below. If you have any further feedback on this new revision, it is welcome. Thank you!

(defun read-side (side) (format t "Enter a value for side ~d: ~%" side) (read))

(defun triangle-p (a b c) (and (< a (+ b c)) (< b (+ a c)) (< c (+ a b))))

(defun equilateral-p (a b c) (= a b c))

(defun isoceles-p (a b c) (or (= a b) (= b c)))

(defun right-triangle-p (a b c) 
  (destructuring-bind (long-side &rest short-sides) (sort (list a b c) #'>)
    (= (expt long-side 2) (+ (expt (first short-sides) 2) (expt (second short-sides) 2)))))

(defun status-message (message a b c) (format t "The shape ~d,~d,~d ~a ~%" a b c message))

(defun classify-triangle (a b c)
  (cond ((not (triangle-p a b c)))
    ((right-triangle-p a b c) 'right)
    ((equilateral-p a b c) 'equilateral)
    ((isoceles-p a b c) 'isoceles)
    (t 'triangle)))

(let (triangle-type (a (read-side 1)) (b (read-side 2)) (c (read-side 3)))
  (format t "classify triangle says ~a ~%" (classify-triangle a b c))
  (setq triangle-type (classify-triangle a b c))
  (cond ((eq 'right triangle-type) (status-message "is a right triangle." a b c))
    ((eq 'equilateral triangle-type) (status-message "is an equilateral triangle." a b c))
    ((eq 'isoceles triangle-type) (status-message "is an isoceles triangle." a b c))
    ((eq 'triangle triangle-type) (status-message "is a non-right, non-isoceles, non-equilateral triangle." a b c))
    (t (status-message "is not a triangle." a b c))))

Here is the latest revision:

(defun read-side (side) (format t "Enter a value for side ~d: ~%" side) (read))

(defun triangle-p (a b c) (and (< a (+ b c)) (< b (+ a c)) (< c (+ a b))))

(defun equilateral-p (a b c) (= a b c))

(defun isoceles-p (a b c) (or (= a b) (= b c)))

(defun right-triangle-p (a b c) 
  (destructuring-bind (long-side &rest short-sides) (sort (list a b c) #'>)
    (= (expt long-side 2) (+ (expt (first short-sides) 2) (expt (second short-sides) 2)))))

(defun status-message (message a b c) (format t "The shape ~d,~d,~d ~a ~%" a b c message))

(defun classify-triangle (a b c)
  (cond ((not (triangle-p a b c)))
    ((right-triangle-p a b c) 'right)
    ((equilateral-p a b c) 'equilateral)
    ((isoceles-p a b c) 'isoceles)
    (t 'triangle)))

(let* ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)) (triangle-type (classify-triangle a b c)))
  (format t "classify triangle says ~a ~%" triangle-type)
  (cond ((eq 'right triangle-type) (status-message "is a right triangle." a b c))
    ((eq 'equilateral triangle-type) (status-message "is an equilateral triangle." a b c))
    ((eq 'isoceles triangle-type) (status-message "is an isoceles triangle." a b c))
    ((eq 'triangle triangle-type) (status-message "is a non-right, non-isoceles, non-equilateral triangle." a b c))
    (t (status-message "is not a triangle." a b c))))

After reading through your suggestion on my sorting question and then reading up on 'format', I tried to write a format statement to simplify the logic at the bottom of the code. I couldn't figure out how to do a format conditional based on a symbol, so instead I defined the triangle types as global variables with 'defineparameter' (is there a better way to define constants?). I also used the format loop which you described in your response to the my-sort-3 question to shorten the expression. The new code is as follows:

(defparameter *right* 0)
(defparameter *equilateral* 1)
(defparameter *isoceles* 2)
(defparameter *triangle* 3)
(defparameter *non-triangle* 4)
(defun read-side (side) (format t "Enter a value for side ~d: ~%" side) (read))

(defun triangle-p (a b c) (and (< a (+ b c)) (< b (+ a c)) (< c (+ a b))))

(defun equilateral-p (a b c) (= a b c))

(defun isoceles-p (a b c) (or (= a b) (= b c) (= a c)))

(defun right-triangle-p (a b c) 
  (destructuring-bind (long-side &rest short-sides) (sort (list a b c) #'>)
    (= (expt long-side 2) (+ (expt (first short-sides) 2) (expt (second short-sides) 2)))))

(defun status-message (message a b c) (format t "The shape ~d,~d,~d ~a ~%" a b c message))

(defun classify-triangle (a b c)
  (cond ((not (triangle-p a b c)) *non-triangle*)
    ((right-triangle-p a b c) *right*)
    ((equilateral-p a b c) *equilateral*)
    ((isoceles-p a b c) *isoceles*)
    (t *triangle*)))

(let* ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)) (triangle-type (classify-triangle a b c)))
  (format t "classify triangle says ~a ~%" triangle-type)
  (format t "The shape (~{~d~^,  ~}) is ~[a right~;an equilateral~;an isoceles~;a non-right, non-isoceles, non-equilateral~;not a~] triangle." (list a b c) triangle-type))

Thanks, everyone, for your continued feedback. Here is the latest revision.

(defun read-side (side) (format t "Enter a value for side ~d: ~%" side) (read))

(defun triangle-p (a b c) (and (< a (+ b c)) (< b (+ a c)) (< c (+ a b))))

(defun equilateral-p (a b c) (= a b c))

(defun isoceles-p (a b c) (or (= a b) (= b c)))

(defun right-triangle-p (a b c) 
    (destructuring-bind (long-side short-side-1 short-side-2) (sort (list a b c) #'>)
          (= (expt long-side 2) (+ (expt short-side-1 2) (expt short-side-2 2)))))

(defun status-message (message a b c) (format t "The shape ~d,~d,~d ~a ~%" a b c message))

(defun classify-triangle (a b c)
    (cond ((not (triangle-p a b c)) nil)
          ((right-triangle-p a b c) 'right)
              ((equilateral-p a b c) 'equilateral)
              ((isoceles-p a b c) 'isoceles)
                  (t 'triangle)))

(let* ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)) 
    (triangle-type (classify-triangle a b c))
    (article (if (member triangle-type '(isoceles equilateral)) "an" "a"))
    (tri-name (if (eq triangle-type 'triangle) "non-right, non-isoceles, non-equilateral" triangle-type)))
    (format t "The shape (~d, ~d, ~d) is ~:[not a triangle~;~a ~(~a~) triangle~]." a b c triangle-type article tri-name))
Improve this question
\$\endgroup\$

5 Answers 5

Reset to default
3
\$\begingroup\$

Much better.

You seem to like assignment a bit more than I do, though. You can rewrite that last let as

(let* ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)) (triangle-type (classify-triangle a b c)))
  (format t "classify triangle says ~a ~%" triangle-type)
  (cond ((eq 'right triangle-type) (status-message "is a right triangle." a b c))
        ((eq 'equilateral triangle-type) (status-message "is an equilateral triangle." a b c))
        ((eq 'isoceles triangle-type) (status-message "is an isoceles triangle." a b c))
        ((eq 'triangle triangle-type) (status-message "is a non-right, non-isoceles, non-equilateral triangle." a b c))
        (t (status-message "is not a triangle." a b c))))

let* is basically let, except that its arguments are guaranteed to be evaluated in the given order (which means that later bindings can refer to previous ones from the same let* statement). It's good style to use let where you can, but let* is better than creating a temporary nil binding and setqing it later.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thank you! let* seems like a very useful function to know about. When you say "I like assignment," do you mean specifically using setq, or do you see another, better way to do this? \$\endgroup\$
    – jaresty
    Mar 9, 2011 at 2:27
  • \$\begingroup\$ @jaresty - I mean assignment in general (yes, your use of setq is what prompted the comment). I remember not minding back in the day, but it seems that you can do the vast majority of things more cleanly and easily without it. It's still useful to have, but IMO, mainly as a tool of last resort. \$\endgroup\$
    – Inaimathi
    Mar 9, 2011 at 2:44
2
\$\begingroup\$

Keep in mind that every form in lisp returns something. (and foo bar baz) returns nil if foo, bar or baz evaluates to nil, and t if all of them evaluate to non-nil. This means that anywhere you write (if (and ...) t nil), you can just write (and ...) with the same effect. The same applies to (if (or ...) t nil) and (or ...), as well as (if (= ...) t nil) and (= ...). Your is-equilateral is a particularly egregious violation of this; it can be written as simply 

(defun equilateral-p (a b c) (= a b c))

The most common convention for predicates in Common Lisp is ending the function name with p or -p, rather than prefixing it with is- (personally, I end predicate names with ?, in line with Scheme convention, but this is not the standard).

Your is-right-triangle function can be simplified a lot by removing the stateful operations and using a more functional approach. I would have written it like so:

(defun right-triangle-p (a b c)
  (destructuring-bind (long-side &rest short-sides) (sort (list a b c) #'>)
    (= (expt large-side 2) 
       (+ (expt (car short-sides) 2) (expt (cadr short-sides 2))))))

As a rule, whenever you use loop to assign values to intermediate variables through setq, double check that you need the loop at all. I've found that it's frequently (though not always) superfluous.

EDIT: If you take my advice about defining your other assignment as a function, you can instead write

...
(destructuring-bind (long-side &rest short-sides) (my-sort-3 a b c)
...

The final let structure can be simplified by using cond instead of nested ifs, and slightly re-arranging the clauses.

(let ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)))
  (cond ((not (triangle-p a b c)) (status-message "is not a triangle." a b c))
        ((equilateral-triangle-p a b c) (status-message "is an equilateral triangle." a b c))
        ((isoceles-triangle-p a b c) (status-message "is an isoceles triangle." a b c))
        ((right-triangle-p a b c) (status-message "is a right triangle." a b c))
        (t (status-message "is a non-isoceles, non-equilateral, non-right triangle." a b c))))

You might want to re-write that final let statement as a function that returns a type of triangle (so like 'isoceles instead of printing "The shape a,b,c is an isoceles triangle.\n") so that you can reuse it more easily. This is basically the same advice I gave you on your sorting function.

On a slightly more general note, you might want to be explicit about which Lisp you're using. It's unambiguous in this case, since I'm pretty sure loop is a CL construct, but keep in mind that "Lisp" is a fairly large family of languages,

Improve this answer
\$\endgroup\$
2
\$\begingroup\$

In response to the 4th edit, I was thinking something along the lines of

(let* ((a (read-side 1)) (b (read-side 2)) (c (read-side 3)) 
       (triangle-type (classify-triangle a b c))
       (article (if (member triangle-type '(isoceles equilateral)) "an" "a"))
       (tri-name (if (eq triangle-type 'triangle) "non-right, non-isoceles, non-equilateral" triangle-type)))
  (format t "The shape (~d, ~d, ~d) is ~:[not a triangle.~;~a ~(~a~) triangle~]." a b c triangle-type article tri-name))

Rather than using global parameters, but that works too (it's not as though you'd use either option in actual code).

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ 1. Why is it sufficient to write "(if (member triangle-type '(isoceles equilateral) ... )" rather than (if (member triangle-type '('isoceles 'equilateral) ... )"? \$\endgroup\$
    – jaresty
    Mar 10, 2011 at 0:58
  • \$\begingroup\$ and 2. What would you use in actual code? \$\endgroup\$
    – jaresty
    Mar 10, 2011 at 1:00
  • \$\begingroup\$ 1. ' is actually just shorthand for (quote ...). quote is a function that basically suppresses evaluation of its body (ie, it specifies that the things in it are to be treated as symbols rather than the values they would otherwise be reduced to). You do sometimes need to quote (and unquote) the components of a quoted form, but that's typically done in macros. As part of regular functions, it's mostly enough to have a single level of quoting because that first ' also suppresses a level of evaluation on the contents of the form its applied to. \$\endgroup\$
    – Inaimathi
    Mar 10, 2011 at 2:07
  • \$\begingroup\$ 2. The code above is a top-level let*, and contains a complex format directive with wordy messages. I'd this at the repl to test out a function, but it's not the sort of code I would check in. "Actual code" (by which I mean "production code") would be a named function (like test-triangle-utility), have brief comments explaining the tricky parts, wouldn't put format through its paces, have a set of defined test arguments instead of reading them from the user, and output a list of '(input result pass/fail) (as you can see by the article clause, English output complicates things). \$\endgroup\$
    – Inaimathi
    Mar 10, 2011 at 2:16
1
\$\begingroup\$

(In response to your 4th edit)

You can simplify right-triangle-p by doing more work in the destructuring:

(defun right-triangle-p (a b c)
  (destructuring-bind (x y z) (sort (list a b c) #'>)
    (= (expt z 2) (+ (expt y 2) (expt x 2)))))
Improve this answer
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Psst. Other way round (or sort using #'<). \$\endgroup\$
    – Inaimathi
    Mar 9, 2011 at 17:09
1
\$\begingroup\$

I think if you enter floating point values, your program might not work as expected. You shouldn't do comparison with =, if the numbers are float.

Improve this answer
\$\endgroup\$

Your Answer

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

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