Sample AutoLISP code - what would you do different?

[sinc]

I've just started messing around with LISP programming, and came up with the following routine. I put this code together by browsing through the online developer documentation, and it illustrates several questions that arose along the way. The command works exactly like the built-in AutoCAD offset command, except that the resulting objects reside on the current layer, instead of the layer of the source object.

I decided that the easiest way to implement the function would be to have it use AutoCAD's default offset command, rather than by creating entities directly via entmake or vla commands, so that I didn't have to worry about how to handle offsetting different types of entities.

The code works, but a bunch of what I did seems really clunky to me, mostly because I seemed to be having typing issues. Having the getdist function return either a real or a string seemed to cause issues. It seemed like there should be an easier way of going about things. (Is there a sprintf sort of functionality in AutoLISP?) I used a global variable to remember the offset distance between successive executions of the function, but is that how I should have done it? The routine also does no error-checking or handling. How should it handle a failure of the offset command (for example, if the selected item cannot be offset)? What else should I have done different?

(defun C:OFS (/ prev prm dist e p edata clayer)
  (setvar "cmdecho" 0)
  (setq clayer (getvar "clayer"))
  (princ "\nOffset Object\n")
  (if (= 'STR (type *ZYZ_OFF*))
    (setq prev *ZYZ_OFF*)
    (if	(= 'REAL (type *ZYZ_OFF*))
      (setq prev (rtos *ZYZ_OFF*))
      (setq prev "")
    )
  )
  (setq prm (strcat "Distance to offset or Through <" prev ">:"))
  (initget 68 "Through")
  (setq dist (getdist prm))
  (if (/= dist nil)
    (setq *ZYZ_OFF* dist)
  )
  (if (/= *ZYZ_OFF* nil)
    (progn
      (if (= 'REAL (type *ZYZ_OFF*))
	(setq prm "\nSelect side to offset:")
	(setq prm "\nSelect through point:")
      )
      (while (setq e (entsel "\nSelect entity to offset:"))
	(redraw (car e) 3)
	(if (setq p (getpoint prm))
	  (progn
	    (command "_.offset" *ZYZ_OFF* e p "")
	    (setq edata (entget (entlast)))
	    (setq edata
		   (subst (cons 8 clayer) (assoc 8 edata) edata)
	    )
	    (entmod edata)
	  )
	)
      )
    )
  )
  (princ)
)

[Coolmo]

Here's what I use.

 
(defun C:os ()
(setvar "cmdecho" 0) 
(setq cl (getvar "clayer"))
(setq osetent (entsel "\nSelect Object To Offset:"))
(setq osetpnt (getpoint "\nPick Side To Offset To:"))
(command "offset" osetdist osetent osetpnt "" "CHANGE" "L" "" "P" "LA" CL "")
(c:os) ;; This will repeat the program until the user hits ESC
)

[Coolmo]

oops. I just noticed I needed to put my CODE in a CODE window obviously. How do I do that?

[BrenBren]

If you are replying, click on the "go advanced" button. This will bring up options for more customization of your message. It should look like the attached image. The button that looks like a number sign, if you hover your mouse over it, says wrap [code] text. Select your code, and click this button. It will do what you are looking for

[sinc]

So then, is osetdist a global variable you set somehow?

Your code is shorter, but I think I prefer my version - it highlights objects as you pick them, remembers the last distance used, handles the "Through" case, and doesn't force the user to hit "Esc" to end the command (something I like to avoid if the command functionality allows it). Add that functionality, and they'd be pretty much the same. The only difference would then be the use of the "change" command in place of my entmod stuff - a half-line in place of three lines. Is there any functional difference between the two? I'd suspect the entmod version would be faster (not that that is a concern in this case).

[Coolmo]

If all you're trying to do is offset an entity to the current layer then the code I posted works good. You're right, it doesn't use the through method but it could be changed easily to offset "through" your pickpoint when asked "Pick side to offset to". Since it uses the AutoCAD command "OFFSET" it automatically highlights the selected object you're offsetting. As far as global variables, the SETQ method in AutoLISP will hold it's set value until it's changed or until you switch drawings. In other words, if (SETQ X 1) is used, X will equal 1 until you either quit AutoCAD or open up a new drawing.

[RobertB]

I've just started messing around with LISP programming, and came up with the following routine. I put this code together by browsing through the online developer documentation, and it illustrates several questions that arose along the way...
The code works, but a bunch of what I did seems really clunky to me, mostly because I seemed to be having typing issues. Having the getdist function return either a real or a string seemed to cause issues. It seemed like there should be an easier way of going about things...
I used a global variable to remember the offset distance between successive executions of the function, but is that how I should have done it? The routine also does no error-checking or handling. How should it handle a failure of the offset command (for example, if the selected item cannot be offset)? What else should I have done different?

Wow. I'm pretty impressed by this effort. You obviously have some programming background.

A global variable is easiest, so that is ok for this type of function. Alternatives would include Dictionaries/XRecords, one of the User?? system variables, or (a poor choice IMHO) the registry.
There isn't much error handling required in this case. The conditional branching took care of most of it, and AutoCAD's command pipeline takes care of the selected object cannot be offset error.
As you surmised, most of the improvements can be made in handling the default and input data types.

Here is my take on the function:

(defun C:OFS  (/ prev prm ePick ptPick eNew)
 (setvar "cmdecho" 0)
;;; there was no need to use a variable to store the CLayer

;;; initialize variable on 1 of 3 conditions:
;;; #1 if global is a string, it must be "Through"
;;; #2 if it is bound (but wasn't a string) it must be a number
;;; #3 if it wasn't bound at all, initialize global to "Through"
 (setq prev (cond ((= 'STR (type *ZYZ_OFF*)) *ZYZ_OFF*)
                  (*ZYZ_OFF* (rtos *ZYZ_OFF*))
                  ((setq *ZYZ_OFF* "Through"))))

;;; I prefer to explicitly show the bits used in (initget)
;;; Also, you may have forgot about 0 (why permit an offset of 0?)
 (initget (+ 2 4 64) "Through")

;;; Might as well set the global to the user's input
;;; I also prefer to use (cond) since it allows for future conditions
;;; as opposed to nesting ifs 
 (setq *ZYZ_OFF* (cond ((getdist (strcat "Distance to offset or [Through] <" prev ">: ")))
                       (*ZYZ_OFF*))
       prm       (cond ((= 'REAL (type *ZYZ_OFF*)) "\nSelect side to offset: ")
                       ("\nSelect through point: ")))

;;; Use clearer variable names; it costs nothing, and improves readabilty
 (while (setq ePick (entsel "\nSelect entity to offset:"))
  (redraw (car ePick) 3)
  (cond ((setq ptPick (getpoint prm))
         (command "_.offset" *ZYZ_OFF* ePick ptPick "")
         (setq eNew (entget (entlast)))

;;; This is all you need to change the layer
         (entmod (subst (cons 8 (getvar "CLayer")) (assoc 8 eNew) eNew))))

;;; Turn off highlight in cases where no ePick point is selected
    (redraw (car ePick) 4))
 (princ))

[sinc]

Thanks, Robert; that answers all my questions (on this particular subject, anyway ). I overlooked the (cond) function in the documentation; that function should prove useful.

So then, I take it people regularly use all that clunky (strcat) and (rtos) sort of stuff in Lisp to accomplish the equivalent of a sprintf?

[dbroad]

Your original code looked fine. Robert also made some great improvements. Here is a completely different approach:

;;create a reactor to handle layering
;;reactor stored in global variable: layer-it
;;D. C. Broad, Jr. 7/8/04
(if (vlr-added-p layer-it) (vlr-remove layer-it))
(setq layer-it (vlr-command-reactor  nil
		 '((:vlr-commandended . layeradjust)
		   (:vlr-commandcancelled . layeradjust)))))

(vlr-remove layer-it)  ;;deactivate until needed

;;callback relayers the work
(defun layeradjust (r cl / e lay)
  (vlr-remove layer-it) ;;deactivate the reactor
  (setq e (vlr-data layer-it)) ;;get the last entity before the offset
  (setq lay (getvar "clayer")) ;;save the current layer
  (while (setq e (entnext e))
    (vla-put-layer (vlax-ename->vla-object e) lay))
)

;;custom offset command
(defun c:ofs ()
  (vlr-add layer-it) ;;activate the reactor
  (vlr-data-set layer-it (entlast)) ;;store the last entity
  (command "offset")) ;;perform offset as usual.

[sinc]

I thought of trying to use a reactor, but decided I wanted each new object to appear on the current layer as it was created; this seemed more user-friendly than changing all objects to the current layer at the end. Your code serves as a good example of how to implement a reactor, though, clearer than the garden path example in the documentation (which involves multiple reactors chained together). Thanks.