Thanks Nicolas
for your valuable insight. I will apply your recommendations.
Le 26/08/2014 10:29, Nicolas Goaziou a écrit :
(orgtbl-ascii-plot): top-level function
(orgtbl-ascii-draw), (orgtbl-uc-draw-grid),
(orgtbl-uc-draw-cont): helper functions which go in table
If they are helper functions, I suggest to use "--" in their name, e.g.,
`orgbtl--ascii-draw'.
Well, all those functions have user-visibility. They appear in the
#+TBLFM: line. I should avoid the words "helper function" here.
+CHARACTERS is a string of characters that will compose the bar,
This is a minor issue, but a "string of characters" sounds strange:
aren't all strings constituted of characters? Maybe you really want
a list of characters.
Yes :) Just string.
+ (unless characters (setq characters " .:;c!lhVHW"))
+ (unless width (setq width 12))
I suggest let-binding variables instead:
(let ((characters (or characters " .:;c!lhvHW"))
(width (or width 12))))
I'll change to let-binding. What is the rational for that ? Better
byte-code ?
+ (if (stringp value)
+ (setq value (string-to-number value)))
Prefer `and' or `when' over one-armed `if'. Also, this may be dangerous
since `string-to-number' can return funny values. Why wouldn't you
simply forbid strings and limit VALUE to integers.
Can I limit VALUE to numbers ? VALUE comes from a cell in the
table. The formula line of the table looks like this:
#+TBLFM: $2='(orgtbl-ascii-draw $1 1 3 12)
VALUE is $1 in this formula.
If (string-to-number VALUE) fails, it will return zero, which is
not very harmful. But yes, you are right, it would be better to
check funny values, and explicitly do something sensible in this
case.
+ (orgtbl-ascii-draw value min max width " \u258F\u258E\u258D\u258C\u258B\u258A\u2589\u2588"))
+
+;;;###autoload
+(defun orgtbl-ascii-plot (&optional ask)
+ "Draws an ascii bars plot in a column, out of values found in another column.
+A numeric prefix may be given to override the default 12 characters wide plot.
+"
You must refer explicitly to ASK in your docstring. In particular, you
may want to detail the distinction between '(4) and 4.
Absolutely.
+ (interactive "P")
+ (let ((col (org-table-current-column))
+ (min 1e999)
`most-positive-fixnum'
Actually this should be `cl-most-positive-float', because
everything here works in floating point values. Thanks for the
clue.
+ (length 12)
+ (table (org-table-to-lisp)))
+ (cond ((consp ask)
+ (setq length
+ (or
+ (read-string "Length of column [12] " nil nil 12)
+ 12)))
+ ((numberp ask)
+ (setq length ask)))
(let ((length (cond ((consp ask)
(read-number "Length of column [12] " nil nil 12))
((numberp ask) ask)
(t 12)))))
Ok, shorter.
+ (mapc
Small nitpick: I suggest to use `dolist' instead of `mapc' (no funcall
overhead).
Sure, easier to read.
+ (lambda (x)
+ (when (consp x)
+ (setq x (nth (1- col) x))
+ (when (string-match
+ "^[-+]?\\([0-9]*[.]\\)?[0-9]*\\([eE][+-]?[0-9]+\\)?$"
+ x)
Would `org-table-number-regexp' make sense here instead of the
hard-coded regexp?
Before hard-coding this regexp, I took a look at what was already
there. org-table-number-regexp matches too much, for instance it
matches:
"<345"
"345()"
"345%45"
"0x45" // hexadecimal
"1101#2" // base two
Anyway, the string feeds (string-to-number x) which does not
accept all those variations. So I created a regexp exactly fitted
for (string-to-number).
+ (or (memq 'hline table) table)) ;; skip table header if any
This check is not sufficient in the following cases:
|-----------|
| no header |
|-----------|
and
|----------|
| header |
|----------|
| contents |
IOW, you need to eliminate the leading 'hline, if any,and skip until
the next 'hline if there is one and if there is something after it.
Ok. Not completely fool-proof, but better. On the other hand, this
loop searches for the min and the max of a column, ignoring
entries which are not numbers. So it could just iterate over the
full column, including any kind of headers.
| header | <-- ignored
|--------| <-- ignored
| 1 | <-- used (will set min=1)
| 2 | <-- used
| xx | <-- ignored
| 3 | <-- used (will set max=3)
|--------| <-- ignored