Re: code improvement

shlomiohana · Posted 10-15-2024 08:30 AM

Hello, I wrote the following code, I would appreciate it if you could improve it for a shorter and more elegant writing.

proc sql noprint;
create table AAA as
select t1.*
from CM_EMAIL_SEND t1
left join CM_TEST_CUSTOMERS t2
on lowcase(t1.email)=lowcase(t2.email)
where t2.email is null;
select count(*) into :row_count from AAA;
quit;

%put the number: &row_count;

%MACRO test;
%IF &row_count. gt 0 %THEN
%DO;
data CM_EMAIL_CHASOM;
set CM_EMAIL_CHASOM AAA(in=new);
if new then
SINUN= 'CM';
run;

proc sql;
delete from CM_EMAIL_SEND
where email in (select email from AAA);
quit;
%END;
%MEND test;
%test;

Tom · Posted 10-15-2024 09:10 AM

It would help if you explained the purpose of the code.

But let's go through it.

Why use a LEFT JOIN instead of just use IN operator of SQL with a subquery? The result should be the same unless your intent is to generate multiple copies of the observations from CM_EMAIL_SEND when the same email address appear in multiple observations from the CM_TEST_CUSTOMERS dataset.

But is seems you only want the observations with missing value of email (is that a mistake in your code?). So perhaps what you really meant was NOT in ?

create table AAA as
select t1.*
  from CM_EMAIL_SEND t1
  where lowcase(t1.email) not in 
   (select lowcase(t2.email) from CM_TEST_CUSTOMERS
    where t2.email is not null)
;

Second there is no need to run another query to know how many observations were written to AAA because PROC SQL already wrote that number into the SQLOBS macro variable.

%let row_count=&sqlobs;

Also there is no need to use the %IF code since when AAA has zero observations appending it to CM_EMAIL_CHASOM will not add any observations. If you are worried that it will take too long to recopy all of the existing observation in CM_EMAIL_CHASOM then use some other method to append the records, such as PROC APPEND or a data step with a MODIFY statement.

Which implies why not just include the variable SINUN when generating the dataset AAA?

create table AAA as
select t1.*, 'CM' as SINUN
  from CM_EMAIL_SEND t1 ...

And if there is no other need for the AAA dataset then why not just use PROC SQL INSERT statement to add the observations to CM_EMAIL_CHASOM?

insert into CM_EMAIL_CHASOM
select t1.*, 'CM' as SINUN
  from CM_EMAIL_SEND t1
  where lowcase(t1.email) not in 
   (select lowcase(t2.email) from CM_TEST_CUSTOMERS where
    t2.email is not null)
;

And finally if the datasets are already sorted by EMAIL (and the values of EMAIL are already converted to lowercase) then forget the SQL and just use data steps.

data aaa;
  merge CM_EMAIL_SEND(in=in1) CM_TEST_CUSTOMERS(in=in2 keep=email);
  by email;
  if in1 and not in2;
run;

shlomiohana · Posted 10-15-2024 09:38 AM

Hello,
The goal of the code is to find all the records in the CM_EMAIL_SEND table whose email is not in the CM_TEST_CUSTOMERS table.
If records are found, they should be added from the CM_EMAIL_SEND table to the CM_EMAIL_CHASOM table, delete the records that were found from the CM_EMAIL_SEND table, SINUN field value should be changed to CM for records passed to the table CM_EMAIL_CHASOM.
email will be sent saying that records have been transferred between the 2 tables.
Thanks.

Astounding · Posted 10-15-2024 09:28 AM

Seriously? You're subsetting on:

where t2.email is null;

But then you want to also check:

 lowcase(t1.email)=lowcase(t2.email

That's really the same as saying:

where t1.email is null and t2.email is null;

This looks like code that was written to confuse someone rather than a real attempt at writing a program. Can you explain in words what you are trying to make happen?

code improvement