Language training program using Common Lisp and the ltk library

The idea is as follows:

We provide program with two files: one with English words and one with words in, let's say, Spanish. The files have one word (it could be a word, or word+some explanation etc) per line and words are ordered in files according to their translation: for example, line 5 of the first file contains the word "a stone" and line 5 of the second file contains the translation of the word "a stone".

The program asks you these questions:

• It decides whether to give your word in English and expect a translation or the other way around (pseudorandom 50% of each).

• It pseudorandomly choses 3 false answers, and together with the correct answer, gives these 4 possible options to you.

• You choose one of the 4 options and it reacts accordingly, keeping track of your win/all ration, win/lose streak, longest win streak, giving correct answers after each question and telling you whether you were right or wrong in your answer.

There are two entry points where you can write a path to files. There is also an option to choose one of two remembered pairs of paths (could be something like Spanish, Chinese or something - according to what language you want to train).

It does its job for me (it would save for one error that happens if you provide the wrong path but it's nothing terrible) but I would like to hear what I should improve on, what is bad style, what may give bad performance, etc.

I would also like to know how I can make an .exe while making sure a pseudorandom sequence will be different each time I run it (I managed to make an .exe, but the sequence was the same each time I turned it on). As you can see, I load the whole content of files into two arrays. Is it ok practice? How can I manage it in a better way?

```
(defparameter vocabulary-file "C:/sbcl/1.3.12/1.txt")
(defparameter translation-file "C:/sbcl/1.3.12/1.txt")
(defparameter wins 0)
(defparameter loses 0)
(defparameter win-streak t)
(defparameter streak-length 0)
(defparameter

Bad

• Really bad: uses macros for no reason. Use functions.

• basic operations like loading a vocabulary are unreadable.

• uses adjustable arrays. Much easier: read a list and convert it to a vector later.

• adjusting the array by one element in each loop iteration is extremely inefficient

• poorly emulates adjustable arrays with fill pointer and VECTOR-PUSH-EXTEND

• poor formatting and indentation

• lots of undocumented global variables

• no documentation in the code

• mixes logic and UI code

• hardcoded filenames

• each functionality should be ideally a function, which can be used and tested individually

• calling format to create a string, on a string which just has been created, is useless

• code is not structured

I'd say: a total rewrite is necessary.

Example

Your code with wrong indentation/formatting:

(defmacro load-up-vector (which from-where)
 `(with-open-file (stream ,from-where :direction :input :if-does-not-exist NIL :external-format :utf-8)
 (if (eq stream NIL) 
  (progn
   (do-msg "FILE NOT FOUND AT PROVIDED LOCATION")
   (setf *FILE-NOT-FOUND* t))
  (let ((line (read-line stream nil nil)))
  (progn 
   (loop 
    :until (eq line nil)
    :do (setf (aref (car ,which) (car (cdr ,which))) line)
        (incf (car (cdr ,which)))
        (adjust-array (car ,which) (list (1+ (car (cdr ,which)))))
        (setf line (read-line stream nil nil)))
    (decf (car(cdr ,which))))))))

Better formatting:

(defmacro load-up-vector (which from-where)
  `(with-open-file (stream ,from-where
                    :direction :input
                    :if-does-not-exist NIL
                    :external-format :utf-8)
     (if (eq stream NIL) 
         (progn
           (do-msg "FILE NOT FOUND AT PROVIDED LOCATION")
           (setf *FILE-NOT-FOUND* t))
       (let ((line (read-line stream nil nil)))
         (progn 
           (loop 
            :until (eq line nil)
            :do (setf (aref (car ,which) (car (cdr ,which))) line)
            (incf (car (cdr ,which)))
            (adjust-array (car ,which) (list (1+ (car (cdr ,which)))))
            (setf line (read-line stream nil nil)))
           (decf (car(cdr ,which))))))))

But the code is broken:

• it should not be a macro

• it should not adjust the array in each iteration, which is extremely costly

• the car/cdr stuff is just not needed

• there is no reason to count anything

• FILE-NOT-FOUND isn't used in the program and a poor way to communicate a missing file to the rest of the program

Better code

• a function

• shorter

• no side effects

• documentation string

• more efficient

• correctly formatted

• testable

Example:

(defun read-lines-from-file (file)
  "Reads a file line by line and returns the lines in a vector.
Returns nil if the file does not exist."
  (if (probe-file file)
      (coerce (with-open-file (stream file :external-format :utf-8)
                (loop for line = (read-line stream nil nil)
                      while line
                      collect line))
              'vector)
    (warn "File ~a not found" file)))

Ideally the logic to check for an existing file and warn should not be here. Left as an exercise...

Usage

Instead of an imperative load-up-vector

(load-up-vector *foo-vector* somewhere)

use something like

(let ((foo-vector (read-lines-from-file somewhere)))
   ...)

StackExchange Code Review Q#157412, answer score: 3