Topic: Advancing my routine from "just works" to proficient

[cgeorg13]

This is simply a post for learning purposes, I'm just looking for better methods of completing the tasks within this routine.

I know that the way I wrote it just works and "just works" is not good enough for me.

I want it to run faster and more efficient and I'm sure this is the place to get all the tips and tricks and advancements for this routine that I need.

This could be a great thread for not only me but for anyone else who is in the somewhat of a beginner phase of writing routines. My routine is very readable for newcomers because that's what I am, so showing how to better my routine can produce clean connections from one way of doing a task, to a more advanced way.

I appreciate any input/advice that the community provides!

Code: [Select]

(defun C:UtilitiesCOH()
  (setvar "cmdecho" 0)
  (setvar "expert" 3)
  (setq planVar 0)
  (while (= planVar 0)
    (setq plan (substr (strcase (getstring "Include Plan Numbers? ((Y)es/(N)o):")) 1 1))
    (cond
      ((= plan "Y") (setq planVar 1))
      ((= plan "N") (setq planVar 2))
      (T (prompt "Invalid Entry\n"))
      )
    )
  (setq tmpVar planVar)

  ;;Provide used layers
  (if (null (tblsearch "layer" "GasUtilitiesCombo"))
    (command "-layer" "make" "GasUtilitiesCombo" "color" "yellow" "" ""))
  (if (null (tblsearch "layer" "GasUtilitiesStorm"))
    (command "-layer" "make" "GasUtilitiesStorm" "color" "64" "" ""))
  (if (null (tblsearch "layer" "GasUtilitiesSanitary"))
    (command "-layer" "make" "GasUtilitiesSanitary" "color" "26" "" ""))
  (if (null (tblsearch "layer" "GasUtilitiesWater"))
    (command "-layer" "make" "GasUtilitiesWater" "color" "171" "" ""))

  (setq sel (ssget '((0 . "LINE,*POLYLINE"))))
  (repeat (setq i (sslength sel))
    (setq ent (ssname sel (setq i (1- i))))
    (setq startpoint (vlax-curve-getstartpoint ent))
    (setq endpoint (vlax-curve-getendpoint ent))
    (if (null (ade_odgetfield ent "SewerMain" "SYSTEMTYPE" 0))
      (progn
(if (null (ade_odgetfield ent "TBL_WATER_MAIN" "DIAMETER" 0))
  (setq od "TBL_SERVICE")
  (setq od "TBL_WATER_MAIN")
  )
(setq sys "Water")
        (setq dia (ade_odgetfield ent od "DIAMETER" 0))
(setq planNum (strcat "(" (ade_odgetfield ent od "PLANNUMBER" 0) ")"))
)
      (progn
(setq sys (ade_odgetfield ent "SewerMain" "SYSTEMTYPE" 0))
        (setq dia (ade_odgetfield ent "SewerMain" "DIAMETER" 0))
        (setq planNum (strcat "(" (ade_odgetfield ent "SewerMain" "MAPREF" 0) ")"))
)
      )
    (if (= planNum "()") (setq planNum ""))
 
    ;;get variables to usable string
    (varConv)
 
    ;;set LT Definition
    (setq LTdef (strcat "____" LTdia  "'' " LTsys "____" LTdia  "'' " LTsys "___\n"))
 
    (if (null (tblsearch "ltype" lt))(createLT))
    (changeProps)
    (if (= planVar 1)
      (progn
(tmaskon)
(if (/= tmpVar planVar)
  (setq planVar tmpVar)
  )
)
      )
    (if (= i 0) (setvar "expert" 0))
    )
  (setq text (ssget "_X" '((0 . "MTEXT"))))
  (command "select" text "")
  )
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;END MAIN;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;BEGIN CALLOUTS;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;;;;;;;VARIABLE CONVERSION;;;;;;;;

(defun varConv()
  (setq tester (/ dia 10))
  (cond
    ((= sys "STORM") (setq LTsys "STM"))
    ((= sys "SANITARY") (setq LTsys "SAN"))
    ((= sys "COMBINED") (setq LTsys "CMB"))
    ((= sys "Water") (setq LTsys "W"))
    (T (prompt "unknown system type"))
    )
  (cond
    ((< tester 1) (setq LTdia (substr (rtos dia) 1 1)))
    ((>= tester 10) (setq LTdia (substr (rtos dia) 1 3)))
    (T (setq LTdia (substr (rtos dia) 1 2)))
    )
  (cond
    ((= LTdia "0") (setq lt LTsys))
    ((= sys "Water") (setq lt (strcat sys LTdia "''")))
    (T (setq lt (strcat LTsys LTdia "''")))
  )
)

;;;;;;;LINETYPE;;;;;;;;

(defun createLT ()
  (setq diaLength (strlen LTdia))
  (setq sysLength (strlen LTsys))
  (setq total (+ dialength syslength))
  (setq bspace (atof ".1"))
  (setq x (float (* (atof ".008") (+ total 3))))
  (setq espace (float (* (atof ".05") (+ total 3))))
  (setq LTdef (strcat "____" LTdia  "'' " LTsys "____" LTdia  "'' " LTsys "___\n"))
  (setq LTspacing (strcat "A,4,-"(rtos bspace)",[""\"" LTdia  "'' " LTsys  "\",LT_FONT,X=-"(rtos x)",y=-.03,S=.08],-"(rtos espace)"\n"))
 
  ;;Filepath/name
  (setq fname "C:\\Users\\Public\\TeamFishel\\Autodesk\\Google Drive\\R19.1\\Linetypes\\Custom.lin")
  ;;Appends written text to Custom.lin file
  (setq linFile (open fname "a"))
  (write-line (strcat "*" lt ", " LTdef LTspacing) linFile)
  (close linFile)
  (command "-linetype" "load" lt "C:\\Users\\Public\\TeamFishel\\Autodesk\\Google Drive\\R19.1\\Linetypes\\Custom" "")
  )

;;;;;;;CHANGE PROPERTIES;;;;;;;;

(defun changeProps()
  (command "chprop" ent "")
  (cond
    ((= LTsys "CMB")(command "layer" "GasUtilitiesCombo" "color" "bylayer"))
    ((= LTsys "SAN")(command "layer" "GasUtilitiesSanitary" "color" "bylayer"))
    ((= LTsys "STM")(command "layer" "GasUtilitiesStorm" "color" "bylayer"))
    (T (command "layer" "GasUtilitiesWater" "color" "bylayer"))
    )
  (cond
    ((>= dia 24)
     (progn
       (command "")
       (command "laymcur" ent)
       (pipeOffset)
       )
     )
    ((> dia 0)
     (progn
       (command "ltype" lt "" )
       (command "laymcur" ent)
       (if (= planVar 1)
(command "mtext" "m2p" startpoint endpoint "j" "ml" "s" "ROMANS" "h" "1.3" "l" "" "2.1667"
  "r" endpoint "w" "0" planNum "")
)
       )
     )
    (T
     (progn
       (command "ltype" LTsys "" )
       (command "laymcur" ent)
       (if (= planVar 1)
(command "mtext" "m2p" startpoint endpoint "j" "ml" "s" "ROMANS" "h" "1.3" "l" "" "2.1667"
  "r" endpoint "w" "0" planNum "")
         )
       )
     )
    )
  )

;;;;;;;LINE OFFSET;;;;;;;;

(defun pipeOffset()
 
  (setq ofd (float (/ (/ dia 12.00) 2.00)))
  (if (setq ss (ssget "p"))
    (repeat (sslength ss)
      (setq ent (vlax-ename->vla-object (ssname ss 0)))
      (if (vlax-method-applicable-p ent 'Offset)
(mapcar '(lambda (o)
   (vl-catch-all-error-p
     (vl-catch-all-apply
       'vla-offset (list ent o))))(list ofd (- ofd)))
)
      (ssdel (ssname ss 0) ss)
     
      )
    )
  (princ)
  (command "chprop" "p" "" "ltype" lt "layer" "Defpoints" "")
  (if (= planVar 1)
    (command "mtext" "m2p" startpoint endpoint "j" "ml" "s" "ROMANS" "h" "1.3" "l" "" "2.1667"
     "r" endpoint "w" "0" (strcat LTdia  "'' " LTsys " " planNum) "")
    (command "mtext" "m2p" startpoint endpoint "j" "ml" "s" "ROMANS" "h" "1.3" "l" "" "2.1667"
     "r" endpoint "w" "0" (strcat LTdia  "'' " LTsys) "")
    )
 
  (setq planVar 1)
  )

(defun TMaskOn ()
  (vl-load-com)
  (setq s (ssget "_L" '((0 . "MTEXT"))))
  (if s
    (mapcar '(lambda (x)
       (setq ob1 (vlax-ename->vla-object x))
       (if (= (vla-get-backgroundfill ob1) :vlax-false)
(vla-put-backgroundfill ob1 :vlax-true)
)
       )
    (vl-remove-if 'listp (mapcar 'cadr (ssnamex s)))
    )
    )
  (prin1)
  )

[ronjonp]

Just a quick glance, but you should localize your variables for all your functions & IMO try to reduce command calls as much as possible. Below are some additional comments.

Code - Auto/Visual Lisp: [Select]

1. (defun c:utilitiescoh ;; RJP - Always localize variables in your functions!
2.             (/       ce      dia     endpoint          ent     ex      i       lt
3.              ltdef   ltdia   ltsys   od      plan    plannum planvar sel     startpoint
4.              sys     text    tmpvar
5.             )
6. ;;;  (setq planvar 0)
7. ;;;  (while (= planvar 0)
8. ;;;    (setq plan (substr (strcase (getstring "Include Plan Numbers? ((Y)es/(N)o):")) 1 1))
9. ;;;    (cond ((= plan "Y") (setq planvar 1))
10. ;;;     ((= plan "N") (setq planvar 2))
11. ;;;     (t (prompt "Invalid Entry\n"))
12. ;;;    )
13. ;;;  )
14.   ;; RJP - consider using initget with getkword like so:
15.   (initget 0 "Yes No")
16.   (setq   planvar   (cond ((getkword (strcat "\nInclude Plan Numbers [Yes/No] <Yes>: ")))
17.             ("Yes")
18.       )
19.   )
20.   ;; RJP - Why are you doing this?
21.   (setq tmpvar planvar)
22.   ;; RJP - added check for selection otherwise error on (sslength nil)
23.   (if (setq sel (ssget '((0 . "LINE,*POLYLINE"))))
24.     (progn ;;Provide used layers
25.       ;; RJP - consider using entmake for layers .. then you won't change the current layer
26.       ;; Also when you make changes to the drawing consider only making them if the user
27.       ;; successfully picks items ( this is why it's located within the 'if' statement.
28.       (if (null (tblsearch "layer" "GasUtilitiesCombo"))
29.         (command "-layer" "make" "GasUtilitiesCombo" "color" "yellow" "" "")
30.       )
31.       (if (null (tblsearch "layer" "GasUtilitiesStorm"))
32.         (command "-layer" "make" "GasUtilitiesStorm" "color" "64" "" "")
33.       )
34.       (if (null (tblsearch "layer" "GasUtilitiesSanitary"))
35.         (command "-layer" "make" "GasUtilitiesSanitary" "color" "26" "" "")
36.       )
37.       (if (null (tblsearch "layer" "GasUtilitiesWater"))
38.         (command "-layer" "make" "GasUtilitiesWater" "color" "171" "" "")
39.       )
40.       ;; RJP - consider storing current variable value then restoring at end of routine ( good practice )
41.       (setq ce (getvar 'cmdecho))
42.       (setq ex (getvar 'expert))
43.       (setvar 'cmdecho 0)
44.       (setvar 'expert 3)
45.       (repeat (setq i (sslength sel))
46.         (setq ent (ssname sel (setq i (1- i))))
47.         (setq startpoint (vlax-curve-getstartpoint ent))
48.         (setq endpoint (vlax-curve-getendpoint ent))
49.         ;; RJP - What does 'ade_odgetfield' do?
50.         (if (null (ade_odgetfield ent "SewerMain" "SYSTEMTYPE" 0))
51.           (progn (if (null (ade_odgetfield ent "TBL_WATER_MAIN" "DIAMETER" 0))
52.          (setq od "TBL_SERVICE")
53.          (setq od "TBL_WATER_MAIN")
54.             )
55.             (setq sys "Water")
56.             (setq dia (ade_odgetfield ent od "DIAMETER" 0))
57.             (setq plannum (strcat "(" (ade_odgetfield ent od "PLANNUMBER" 0) ")"))
58.           )
59.           (progn (setq sys (ade_odgetfield ent "SewerMain" "SYSTEMTYPE" 0))
60.             (setq dia (ade_odgetfield ent "SewerMain" "DIAMETER" 0))
61.             (setq plannum (strcat "(" (ade_odgetfield ent "SewerMain" "MAPREF" 0) ")"))
62.           )
63.         )
64.         (if (= plannum "()")
65.           (setq plannum "")
66.         )
67.         ;;get variables to usable string
68.         (varconv)
69.         ;;set LT Definition
70.         (setq ltdef (strcat "____" ltdia "'' " ltsys "____" ltdia "'' " ltsys "___\n"))
71.         (if (null (tblsearch "ltype" lt))
72.           (createlt)
73.         )
74.         (changeprops)
75.         ;; RJP - change to check for "Yes" rather than 1
76.         (if (= planvar "Yes")
77.           (progn (tmaskon)
78.             ;; RJP - This will always be true...
79.             (if (/= tmpvar planvar)
80.          (setq planvar tmpvar)
81.             )
82.           )
83.         )
84.         ;; RJP - restore variables ... look into error handling routines in case your program does not exit cleanly
85.         (setvar 'cmdecho ce)
86.         (setvar 'expert ex)
87.       )
88.       (setq text (ssget "_X" '((0 . "MTEXT"))))
89.       ;; RJP - consider sssetfirst
90.       (sssetfirst nil text)
91.       ;;(command "select" text "")
92.     )
93.   )
94.   ;; RJP - added (princ) to exit routine cleanly
95.   (princ)
96. )

[dgorsman]

• Localise variable names
• Reduce hard-coded data like path names - use searching external data stores, or provide as an argument
• Error handling
• Always provide the leading zero ie. 0.008, not .008
• Don't convert values unnecessarily ie. 0.008, not (atof "0.008")
• Have functions consume arguments and return something meaningful

[cgeorg13]

Thank you for your gracious response Ronjon.

Code: [Select]

        ;; RJP - What does 'ade_odgetfield' do?
        (if (null (ade_odgetfield ent "SewerMain" "SYSTEMTYPE" 0))


ade_odgetfield was basically the start of what made me write this routine. We use the command MapImport to bring in shapefiles and attach ObjectData 'od' to the lines/block/points/etc and we store every bit of info into this. I found out about searching into these object data tables from the following site.
https://map3d.wordpress.com/2010/06/17/greek-or-nerd-using-links-in-od-tables/

I definitely have some questions to come about some of your other comments.

But for now what I remember is to ask what the importance of localizing the variables is?
As in, whats the difference from not calling them and calling them in the opening (defun cmdName(Variables) ?


Also,

Code: [Select]

(if (= planvar "Yes")
          (progn (tmaskon)
            ;; RJP - This will always be true...
            (if (/= tmpvar planvar)
         (setq planvar tmpvar)
            )
          )
        )


I change the planVar in the pipeOffset section so that it will run into the text mask function even if i dont include the plan numbers because we have to include mtext with the diameter and type of pipe when we offset it instead of having a linetype with that information. I didnt want to end up calling the text mask function twice by throwing it into the pipeoffset section if that makes sense.

[cgeorg13]

• Localise variable names
• Reduce hard-coded data like path names - use searching external data stores, or provide as an argument
• Error handling
• Always provide the leading zero ie. 0.008, not .008
• Don't convert values unnecessarily ie. 0.008, not (atof "0.008")
• Have functions consume arguments and return something meaningful

First off, Thank for a quick list of stuff to look into!

The only reason I have hard-coded path names is because we have all our information linked to a shared google drive that is accessable to every CAD operator and includes any crucial information in the same location for everyone.

As for error handling, I hope I'm not alone on this one but that stuff is insanely hard for a newbie to understand! Any simple examples to get the idea of the process across?

Leading zeros- this is actually the parts of my routine that I am least happy with. The conversion and checking for how many trailing decimal places to keep and keeping a leading zero. I know there has to be an easier method of keeping precision than what I have done here because its not even that precise! Any help with this is MOST appreciated!

Have functions consume arguments and return something meaningful:
This I believe is a newbie mistake for thinking that I am organizing my routine by making functions outside of the main function that is merely to do calculations rather than returning "something meaningful", I'll be sure to work on this.

Thank you so much again for your input dgorsman.

[ronjonp]

Thank you for your gracious response Ronjon.

Code: [Select]

        ;; RJP - What does 'ade_odgetfield' do?
        (if (null (ade_odgetfield ent "SewerMain" "SYSTEMTYPE" 0))


ade_odgetfield was basically the start of what made me write this routine. We use the command MapImport to bring in shapefiles and attach ObjectData 'od' to the lines/block/points/etc and we store every bit of info into this. I found out about searching into these object data tables from the following site.
https://map3d.wordpress.com/2010/06/17/greek-or-nerd-using-links-in-od-tables/

I definitely have some questions to come about some of your other comments.

But for now what I remember is to ask what the importance of localizing the variables is?
As in, whats the difference from not calling them and calling them in the opening (defun cmdName(Variables) ?


Also,

Code: [Select]

(if (= planvar "Yes")
          (progn (tmaskon)
            ;; RJP - This will always be true...
            (if (/= tmpvar planvar)
         (setq planvar tmpvar)
            )
          )
        )


I change the planVar in the pipeOffset section so that it will run into the text mask function even if i dont include the plan numbers because we have to include mtext with the diameter and type of pipe when we offset it instead of having a linetype with that information. I didnt want to end up calling the text mask function twice by throwing it into the pipeoffset section if that makes sense.

Lee has an excellent write up on locali(z)(s)ing variables HERE. 

[cgeorg13]

Code: [Select]

(initget 0 "Yes No")
(setq   planvar   (cond ((getkword (strcat "\nInclude Plan Numbers [Yes/No] <Yes>: ")))
            ("Yes")
      )
  )

I'm having a problem getting the initget and the getkword to work correctly with the yes/no options and storing the string as the planVar. It is prompting correctly but will not take anything but pressing enter. Is this because of the "0" in the initget? I tried switching it around and its still not acting correctly.

If you could help me understand a little more with what the cond statement is checking for and maybe I could figure it out.

[ronjonp]

You can actually take the 0 out (copy\paste from some other code)

Code - Auto/Visual Lisp: [Select]

1. (initget "Yes No")
2. (setq planvar (cond ((getkword "\nInclude Plan Numbers [Yes/No] <Yes>: "))
3.           ("Yes")
4.          )
5. )

If I were you, I'd read up on the functions being used. Each one is handily linked (blue) to a help file. 

[Cathy]

From a user point of view, I hate it when lisp programs change my system variables.

(setvar "cmdecho" 0)
  (setvar "expert" 3)

[David Bethel]

You may want to consider combining all LAyer calls vs calling (command "_.LAYER") multiple times :

Code - Auto/Visual Lisp: [Select]

1. ;;Provide used layers
2. (command "_.LAYER")
3. (foreach l (list (list "GasUtilitiesCombo"     "2" "CONTINUOUS")
4.                  (list "GasUtilitiesStorm"    "64" "CONTINUOUS")
5.                  (list "GasUtilitiesSanitary" "26" "CONTINUOUS")
6.                  (list "GasUtilitiesWater"   "171" "CONTINUOUS"))
7.   (if (not (tblsearch "LAYER" (nth 0 l)))
8.       (command "_Make"  (nth 0 l)
9.                "_Color" (nth 1 l) (nth 0 l)
10.                "_LType" (nth 2 l) (nth 0 l))))
11. (command "")
12.  

Editing / maintaining a master list works better for me

-David

[dgorsman]

From a user point of view, I hate it when lisp programs change my system variables.

(setvar "cmdecho" 0)
  (setvar "expert" 3)

Sometimes its necessary.  But to be polite, anything it changes should be set back and an error handler used to do the same in the event of a cancel or hard error.

[cgeorg13]

This is my new conversions of the diameter of pipe from a real number to a precise, and usable string for generating linetypes...

I'm still not happy with it but it works, and as the headline says, thats not what I want! haha

Could someone please give me any advice on how to improve this more.

Code - Auto/Visual Lisp: [Select]

1. ;;Diameter Conversion
2.     (cond
3.       ((<= dia 0) (setq LTdia "0"))
4.       ((< dia 10)
5.        (progn
6.          (setq str 2)
7.          (while (/= test "0")
8.            (setq str (+ str 1))
9.            (setq test (substr (rtos dia) str 1))
10.          )
11.          (setq LTdia (substr (rtos dia) 1 (- str 1)))
12.        )
13.       )
14.       ((< dia 100)
15.        (progn
16.          (setq str 3)
17.          (while (/= test "0")
18.            (setq str (+ str 1))
19.            (setq test (substr (rtos dia) str 1))
20.          )
21.          (setq LTdia (substr (rtos dia) 1 (- str 1)))
22.        )
23.       )
24.       ((>= dia 100)
25.        (progn
26.          (setq str 4)
27.          (while (/= test "0")
28.            (setq str (+ str 1))
29.            (setq test (substr (rtos dia) str 1))
30.          )
31.          (setq LTdia (substr (rtos dia) 1 (- str 1)))
32.        )
33.       )
34.     )
35.  
36.     ;;Remove trailing decimal point if no decimal places recorded
37.     (if (= (substr LTdia (strlen LTdia) 1) ".")
38.       (setq LTdia (substr LTdia 1 (- (strlen LTdia) 1)))
39.     )
40.  

I then use it to build the Linetype definition which is why I'm converting into strings if anyone's wondering

[cgeorg13]

You may want to consider combining all LAyer calls vs calling (command "_.LAYER") multiple times :

Code - Auto/Visual Lisp: [Select]

1. ;;Provide used layers
2. (command "_.LAYER")
3. (foreach l (list (list "GasUtilitiesCombo"     "2" "CONTINUOUS")
4.                  (list "GasUtilitiesStorm"    "64" "CONTINUOUS")
5.                  (list "GasUtilitiesSanitary" "26" "CONTINUOUS")
6.                  (list "GasUtilitiesWater"   "171" "CONTINUOUS"))
7.   (if (not (tblsearch "LAYER" (nth 0 l)))
8.       (command "_Make"  (nth 0 l)
9.                "_Color" (nth 1 l) (nth 0 l)
10.                "_LType" (nth 2 l) (nth 0 l))))
11. (command "")
12.  

Editing / maintaining a master list works better for me

-David

David,

This looks very clean and smooth, I did not have a chance to try it out yet but I plan on trying to implement it into the routine tomorrow!
Thanks a lot for the suggestion!

[Lee Mac]

This is my new conversions of the diameter of pipe from a real number to a precise, and usable string for generating linetypes... I'm still not happy with it but it works

Code - Auto/Visual Lisp: [Select]

1. _$ (getvar 'luprec)
2. 4
3. _$ (setq dia 12.3456)

We could be here for some time 

[Lee Mac]

For the string conversion, I would suggest something along the lines of:

Code - Auto/Visual Lisp: [Select]

1. ;; Number to String  -  Lee Mac
2. ;; Converts a supplied numerical argument to a string
3.  
4. (defun LM:num->str ( x / dim rtn )
5.     (if (equal x (atoi (rtos x 2 0)) 1e-8)
6.         (rtos x 2 0)
7.         (progn
8.             (setq dim (getvar 'dimzin))
9.             (setvar 'dimzin 8)
10.             (setq rtn (vl-catch-all-apply 'rtos (list x 2 15)))
11.             (setvar 'dimzin dim)
12.             (if (not (vl-catch-all-error-p rtn)) rtn)
13.         )
14.     )
15. )

Examples:

Code - Auto/Visual Lisp: [Select]

1. _$ (LM:num->str 1)
2. "1"
3. _$ (LM:num->str 1.2)
4. "1.2"
5. _$ (LM:num->str 1.234)
6. "1.234"

To account for the negative condition:

Code - Auto/Visual Lisp: [Select]

1. (defun diaconversion ( x )
2.     (if (< 0 x) (LM:num->str x) "0")
3. )