This is two and a half years old, but for the sake of future viewers:
Overall your coding style looks pretty good, especially for someone new to Clojure. I don't have any comments on the AI engine itself, but here are a few structural notes:
not-nil? already exists in clojure.core as some?.
Docstrings are usually phrased actively: e.g. "Returns the result ...
Here are my comments after a brief reading:
First, if you intend to make this library fit for general use, add documentation in comments for the provided functions. See the Racket style guide for examples
Also, if you intend to make this a more general library, don't include default arguments (e.g. for ircbot-connect) unless they make sense for all users. ...
What is not enough LISP-y in your function is the style used for the parentheses (they should not be left alone as last element of a line and they should be on the last line of the enclosed form). Another minor point is that you can use the shorter mapcar instead of map 'list.
Here is a version of your function (generalized for any function applied to the ...
It is a common convention in Lisp to avoid uppercase (or mixed lowercase and uppercase) letters in identifiers (symbols), so for instance use l instead of L, or fibo-nums instead of fiboNums, etc.
If you type your function in a Common Lisp interpreter/compiler, you will receive warnings undefined variable for A B and next. This is ...
You are not using tail recursion in my-sum, so the compiler cannot easily turn it into iteration.
It depends on the specific problem; in this case I would probably do it iteratively.
Sure, loop is useful and convenient.
Here is my attempt:
(defun multiple-of-3-or-5 (x)
(or (zerop (mod x 3))
(zerop (mod x 5))))
(defun my-sum (from to)
I'm the author of core.matrix, so hopefully I can give you some tips from that perspective.
If you want to improve performance, it's much better to use vectors in an optimised format throughout (vectorz-clj is a fine choice) rather than mixing in Clojure vectors everywhere. This saves the overhead of converting to/from Clojure vectors all the time, which is ...
Yes, it can be shortened by using defstruct. An example from the docs:
(cl-defstruct person name age sex)
(setq dave (make-person :name "Dave" :sex 'male))
⇒ [cl-struct-person "Dave" nil male]
(setq other (copy-person dave))
⇒ [cl-struct-person "Dave" nil male]
(eq dave other)
When I went through the WYAS48 I made the same decision of breaking the bits of eval off into smaller functions. I found this made testing easier since I could focus on just one function at a time.
I find that having a function be one massive bunch of logic is not helpful. In an imperative language the equivalent would be a single function with a ton of if/...
The IDE you linked uses CLISP, which is a bit lenient; when I evaluate
that definition, I immediately get two warnings from SBCL:
; in: DEFUN COUNT-UP-DOWN-CHARACTERS-WITH-DIFFERENCE
; (SETF COUNT-DOWN 0)
; (SETQ COUNT-DOWN 0)
; caught WARNING:
; undefined variable: COUNT-DOWN
; (SETF COUNT-UP 0)
; (SETQ COUNT-UP 0)
Lisp is well-known for it's REPL, so I'd expect to see at least the following 3 functions in any Lisp interpreter: read, evaluate and print.
read takes a string and returns a form (a number, string, symbol, list, and so on). For example: read("(+ 4 5)") should return a list that contains the symbol + and the numbers 4 and 5.
evaluate takes a ...
I agree with sds's post. In addition, you should note that eq does not guarantee to return true for two equal numbers (see, especially, the (let ((x 5)) (eq x x)) case). You should use zerop to test if a number is zero (and = for comparing numeric equality in general).
Do not recompute is-multiple repeatedly by either binding the value:
(defun num-action (i)
(let ((i3 (is-multiple i 3))
(i5 (is-multiple i 5)))
(cond ((and i3 i5) (print "FizzBuzz"))
(i3 (print "Fizz"))
(i5 (print "Buzz"))
(T (print i)))))
or by using if:
(defun num-action (i)
(if (is-multiple i 3)
Your find-node is actually (almost) assoc or, if you prefer, (find node graph :key #'first).
Use mapcar instead of map 'list because it makes the intent clearer (the difference is that mapcar takes only lists and map any sequence).
You need just one do in loop; you can even fold all your setfs into one (setf a b c d e f).
In loop, with should ...
Use 1- instead of (- ... 1).
Avoid very long lines (Emacs will indent for you).
Do not use cond when a single if without progn would do.
Use nconc instead of
append when possible to avoid
unnecessary consing (in your case, splice allocates a fresh list, so
its result can be passed to nconc).
Whenever you use nth, you are
The code achieves the most important goal. It runs and produces the correct answer. Using helper/auxiliary methods is a very useful practice and makes the code clearer.
As mentioned in the accepted answer, it is standard practice to use lower case for all names. The reason is that historically, the Lisp pretty printer prints in ALLCAPS. Using ...
Don't take all remarks as absolute. Some are just how I would do things. There are many ways to structure a program.
It's a great way to learn Lisp programming!
documented constant values with speaking names
names are usually speaking and self-documenting
use of CLOS
use of documentation strings
Suggestions for improvements:
DEFCLASS has a :...
There are several problems with your code.
is-prime is C/Java style. Lispers use primep or prime-number-p.
zerop is clearer than (= 0 ...).
Lispers use indentation, not paren counting, to read code. Your
code is thus virtually unreadable. Please use Emacs if you are
unsure how to format lisp properly.
is-prime is tail-recursive, so ...
Lisp is a multiparadigm language.
apply is just as lispy as recursion, and, in a way, much more so (think in HOFs)!
Please fix indentation.
Please write #'foo instead of (function foo).
The first (HOF) version can be much more efficiently rewritten in using mapcan (provided defscribe-path returns fresh lists):
The version with the count feature of loop in Common Lisp:
(defun count-up-down-characters-with-difference (input-string up-char down-char)
"Given a string of any length, iterate each character of the string looking
for up- and down-characters provided by the caller, and return the number of
each, as well as the difference between them."
(loop for ...
return isalpha(c) || c == '\'' || c == '?' || c == '+' || c == '-' || c == '*' || c == '/' || c == '^' || c == '=' || c == '>' || c == '<' || c == '~';
This could be rewritten much more readably as
return isalpha(c) || (c != '\0' && strchr("'?+-*/^=><~", c) != NULL);
A good compiler will produce the same code for both. (Neither GCC nor ...
A function's docstring must go before its argument vector. You've written your docstrings after your argument vectors, which prevents your :preconditions from being recognized and doesn't work when you try to retrieve the docstring with tools like doc:
Rather than ...
That's a great start! Looks like a lot fun.
I would do only a few things slightly different. Some of them a real improvements and some are style preferences of an old Lisper... ;-)
First function: PRINT-LINE
(defun print-line (&rest args)
:for type :in args :by #'cddr
:for num :in (cdr args) :by #'cddr
:do (loop :repeat num
A possible way is to use one of the bitwise logical operators on integers, that treat integers as binary numbers. For instance, by using the logxor operator, we could write:
(defun invert-bits2 (n)
(if (> n 0)
(logxor (1- (expt 2 (integer-length n))) n)
The function integer-length returns the number of bits of the binary representation ...
In general something like "if a then true else false" can be simplified as "a". So your first function could be simplified as:
(defun multiple-of-3-or-5p (n)
"predicate for multiple of 3 or 5"
(or (= 0 (rem n 3)) (= 0 (rem n 5))))
Cond with only two cases
A cond with only two cases is written preferibly as an if. ...
So either a is the minimum of the list in which case b and c or it's not and it's one of the numbers you want to keep. The other is the max of b and c.
(define (f a b c)
(if (or (> a b) (> a c))
(sum-of squares a (max b c))
(sum-of-squares b c)))
(define (square x) (* x x))
(define (sum-of-squares a b) (+ (square a) (square b)))
It looks okay for a recursion exercise, but it's naive code. Not usable in 'production'.
it conses like mad. It creates a lot of intermediate garbage. Splitting a string involves making lots of smaller strings.
for lists it is not efficient, too
CONCATENATE in loops or recursive functions is a code smell
the recursive implementation with the subroutine is ...
You're solving the problem inefficiently; in the same way that you can mentally figure out in a few seconds that the sum of every number from 1 to 100 inclusive is 5050, you can do the same here:
999/5 = 199 instances, divided by 2 is 99 (plus 500, the half-way mark)
5 + 995 = 1000 sum of opposite numbers
1000 sum * 99 instances = 99,000
999/3 = 333 ...
Is my use of recursion wrong?
It's not wrong, but it's unnecessarily complex. There's no need to create a list and recurse over it when you could simply iterate over the numbers.
Should I prefer iterative approach over recursion in Lisp too? (as in imperative languages)
Iteration is perfectly acceptable in functional languages! Functional programmers talk ...
There are lots of minor improvements possible to your code.
The biggest problems though is: unclear function interfaces.
(defun output-consequent-layer (anfis prev-output input)
I have no idea what anfis, prev-output or input actually is.
Either write comments for those, document the basic data structures somewhere or actually do it in Lisp code. Type ...
(defun skewed-average1 (list)
"calculate average by summing and dividing"
(let ((sum 0) (n 0))
(dolist (x list)
(if (>= x 0)
(if (> x 100) (setf x 100))
(setf sum (+ x sum))
(setf n (+ 1 n)))))
(/ sum n)))
IF ... PROGN is WHEN.
>= 0 is plusp.
(setf sum (+ ... is INCF.