Code Feedback

Accepted Solution Solved
Reply
Occasional Contributor
Posts: 9
Accepted Solution

Code Feedback

Hi,

 

The company I work for just got SAS about a year ago and I never used SAS before that. I've been trying to pick it up and use SAS exclusively the past few months. I created my first loop yesterday and I was looking for any feedback I could get. Since SAS is new for the whole company, not many people here can help. I've tried to comment out my program so anybody willing to provide feedback could follow along but I'll be on next week and can answer any questions if you're confused at what I was trying to do.

 

Thanks!

P

 


Accepted Solutions
Solution
3 weeks ago
Super User
Super User
Posts: 8,075

Re: Code Feedback

It would be easier to review if you just pasted the code into a code box.

A couple of quick things I see.

The main one is don't use exact value tests to end a do loop. 

 

%LET DATE=201803;
	/*Start Loop*/
	%DO %UNTIL (&DATE=201312);

What if the value increments right past the value?   You could end up with a infinite loop.

 

 

It kind of looks like you are looping over months?  If so then use a normal DO loop and the INTNX() function. 

 

%do month=0 to %sysfunc(intck(month,'01MAR2018'd,'01DEC2013'd);
   %LET DATE=%sysfunc(intnx(month,'01MAR2018'd,&month),yymmn6);

Also there is no need for CAT() functions when working in macro code.

Replace

 

%LET VMF= %SYSFUNC(CAT(SaAcPlVf.VMFHEALTHBKRSLTCIT_
						,&DATE
						,_));
	%LET VMF= &VMF.&VMF_LENGTH;

with

 

 

%LET VMF= SaAcPlVf.VMFHEALTHBKRSLTCIT_&date._&VMF_LENGTH;

 

 

View solution in original post


All Replies
Solution
3 weeks ago
Super User
Super User
Posts: 8,075

Re: Code Feedback

It would be easier to review if you just pasted the code into a code box.

A couple of quick things I see.

The main one is don't use exact value tests to end a do loop. 

 

%LET DATE=201803;
	/*Start Loop*/
	%DO %UNTIL (&DATE=201312);

What if the value increments right past the value?   You could end up with a infinite loop.

 

 

It kind of looks like you are looping over months?  If so then use a normal DO loop and the INTNX() function. 

 

%do month=0 to %sysfunc(intck(month,'01MAR2018'd,'01DEC2013'd);
   %LET DATE=%sysfunc(intnx(month,'01MAR2018'd,&month),yymmn6);

Also there is no need for CAT() functions when working in macro code.

Replace

 

%LET VMF= %SYSFUNC(CAT(SaAcPlVf.VMFHEALTHBKRSLTCIT_
						,&DATE
						,_));
	%LET VMF= &VMF.&VMF_LENGTH;

with

 

 

%LET VMF= SaAcPlVf.VMFHEALTHBKRSLTCIT_&date._&VMF_LENGTH;

 

 

Occasional Contributor
Posts: 9

Re: Code Feedback

Hey Tom,

 

Appreciate the feedback.

 

I agree that I should change %UNTIL (&DATE<=201312) is what I should have used. I built this for an ad hoc project and to practice using a Loop.

 

As for concatenating in macro language, does the "." only concatenate when it is in between two macro variables? How would you concatenate two macro variables and also want the "."? For instance, what if one macro variable was the library and the second was the table name. How would you concatenate that? I find myself sometimes using the . and sometimes use the CAT function. I think this is mainly because I'm unfamiliar with concatenating in macro language and know how to do it using the CAT function (which I also find easier to read).

 

I was not always looping by month. Our data sets go as follows:

201312

201412

201512

201603

201606

201609

...

201706

201707

201708

...

Super User
Super User
Posts: 8,075

Re: Code Feedback

SAS uses the period to indicate the end of the macro variable name.  This makes it possible tell the where the end of the macro variable name is. So if you want to include a literal period you just need have two. One for SAS to use to mark the end of the macro variable name and one to be part of the generated text.

 

To loop across an arbitrary list of values in macro code I normally using a delimited list.

%let list=201312 201412 201512 201603 ;
%do i=1 %to %sysfunc(countw(&list));
  %let next=%scan(&list,&i);
  ...
Super User
Posts: 13,508

Re: Code Feedback

"Magic numbers" instead of parameters are something to look out for. Magic number is a term for hardcoded values that do not have an obvious reason why it is that explicit value.

 

The general idea with macro coding is that is flexible and so you can pass parameters such that the code adjusts without having to go through the code you have and change "201707" to "201708" for instance as a comparison.

Better instead of the %let date=201803; at the start of the code to set that value as a parameter:

 

%macro loop (date=);

 

And then when you use the code

%loop(date=201803); to run for exact value.

or

%loop(date=201804); when you need to run similar code for the next value.

It appears that the magic numbers I referenced could well be calculated from that value. Note that when dealing with date related items, as @Tom said it is really a good idea to think in terms of SAS date values and the various date functions.

 

A comment on general code. This line made me very uncomfortable:

	/*Overwrite &DATE local macro variable with the result of the &prior global macro variable*/
	%LET DATE= &prior;

Reason I am uncomfortable is because you are using DATE as a loop control and then have another macro %create_Priors that might alter the value of &prior inside the loop. And since you also delete the global variable that assignment in the %create_priors seems very likely.   A small error or possibly just an odd data value somewhere might kick this macro into an infinite loop because &date never gets to 201312.

 

 

Replacing &date and &prior both a different times might make diagnostics involving those two variables when the loop misbehaves a bit difficult.

Occasional Contributor
Posts: 9

Re: Code Feedback

Maybe I should have put "update" instead of "overwrite". 

 

I'm confused at your comment about &prior (which was the global I passed to %Create_Priors) that I then use to assign to &date. I then delete &prior in each iteration because I don't like having globals laying around. While I code, I try to get the data I need from the global, assign it to a local macro variable (in this case &date), and then delete the global.

Regular Contributor
Posts: 209

Re: Code Feedback

I recommend adding %local-statement for each macro-variable to avoid really annoying debug-session when someday someone calls the macro in another macro using variables with the same names you used.

Occasional Contributor
Posts: 9

Re: Code Feedback

Posted in reply to error_prone

I thought a %LET creates a local? Is that not the case or are you saying I should use %local instead of %let?

Super User
Super User
Posts: 8,075

Re: Code Feedback

[ Edited ]

@pchappus wrote:

I thought a %LET creates a local? Is that not the case or are you saying I should use %local instead of %let?


A %LOCAL statement is used to create a local macro variable. 

A %LET is used to assign a value to macro variable. 

 

If the variable already exists (in ANY scope) then its value is replaced. But if it does not already exist then a new macro variable is created in the most local symbol table. 

 

Making the macro variable as local will prevent you from overwriting values of a similarly named macro variable in a calling environment's symbol table.

☑ This topic is solved.

Need further help from the community? Please ask a new question.

Discussion stats
  • 8 replies
  • 193 views
  • 4 likes
  • 4 in conversation