Declaring a new variable in a loop or outside of it?

[Solarion]

Which one is actually the correct way. This

foreach ( i in dirList) {
	object invoiceNo = i.Split("\\").Last().Split(".").First.Split(" ").Last;

	MailMSG mail_message = new MailMSG();
	mail_message.Message = "Please see attached document...";
	mail_message.To = emailTo;
	mail_message.EMailType = EmailType.Invoice;
	mail_message.Attachments = i;

	SendEmail(mail_message, invoiceNo);

}

or

MailMSG mail_message = new MailMSG();

foreach ( i in dirList) {
	object invoiceNo = i.Split("\\").Last().Split(".").First.Split(" ").Last;

	mail_message.Message = "Please see attached document...";
	mail_message.To = emailTo;
	mail_message.EMailType = EmailType.Invoice;
	mail_message.Attachments = i;

	SendEmail(mail_message, invoiceNo);

}

 

[Kosmik]

Are you going to address the object outside of the loop? If so, declare outside otherwise inside so it destructs with the loop. It's all about scope.

 

[Solarion]

Are you going to address the object outside of the loop? If so, declare outside otherwise inside so it destructs with the loop. It's all about scope.

I only address it inside the loop. When you say destructs, do you mean it falls out of memory or is disposed once the loop completes every iteration?

 

[Kosmik]

I only address it inside the loop. When you say destructs, do you mean it falls out of memory or is disposed once the loop completes every iteration?

Both. Depending on scope, various assets of a segment of code will destruct when possible. Obviously this depends on the garbage collection method of the language\compiler you are using too.

I think of it this way. If I need to address the variable elsewhere, it needs to be visible to my code so declare it outside of any "self containing" loops according to the scope required. if it is only utilized within the loop, then declare it within. Yes, you could use the same variable over and over but depending on the complexity of the object, you might have properties that carry over from one iteration to the other that are not desired, so in my mind I'd rather re-create.

 

[Saham]

Are you going to address the object outside of the loop? If so, declare outside otherwise inside so it destructs with the loop. It's all about scope.

I agree, but in this case there is something else to consider as well:
If the mail_message object is created outside the loop then it will be re-used for each call to SendEmail. I know nothing about this call, but if SendEmail keeps the object for later reference then each iteration of the loop will overwrite the previous message's data.

It is much safer to just create a new object each time.

The only reason to re-use the same object is if it is a lot of work to create a new one, or it uses a lot of memory, but those are rare and the documentation would definitely describe how to re-use them safely.

If you needed the variable outside the loop (for checking the last iteration of the loop) then you can declare the variable outside, but create a new instance inside:

MailMSG mail_message;

foreach ( i in dirList) {
    object invoiceNo = i.Split("\\").Last().Split(".").First.Split(" ").Last;

    mail_message = new MailMSG();
    mail_message.Message = "Please see attached document...";
    mail_message.To = emailTo;
    mail_message.EMailType = EmailType.Invoice;
    mail_message.Attachments = i;

    SendEmail(mail_message, invoiceNo);
}

 

[Kosmik]

I agree, but in this case there is something else to consider as well:
If the mail_message object is created outside the loop then it will be re-used for each call to SendEmail. I know nothing about this call, but if SendEmail keeps the object for later reference then each iteration of the loop will overwrite the previous message's data.

If you needed the variable outside the loop (for checking the last iteration of the loop) then you can declare the variable outside, but create a new instance finside:

MailMSG mail_message;

foreach ( i in dirList) {
    object invoiceNo = i.Split("\\").Last().Split(".").First.Split(" ").Last;

    mail_message = new MailMSG();
    mail_message.Message = "Please see attached document...";
    mail_message.To = emailTo;
    mail_message.EMailType = EmailType.Invoice;
    mail_message.Attachments = i;

    SendEmail(mail_message, invoiceNo);
}

Just an extra 2c but if the called method was meant to keep the object for later, it should then deal with that within itself. Kind of breaks the point of refactoring to not.

 

[Solarion]

Just to clarify, this is the call here

        private void SendEmail(MailMessage mail_msg, String invoiceNo)
        {

            Microsoft.Office.Interop.Outlook.Application oApp = new Microsoft.Office.Interop.Outlook.Application();
            Microsoft.Office.Interop.Outlook.MailItem oMsg = (Microsoft.Office.Interop.Outlook.MailItem)oApp.CreateItem(Microsoft.Office.Interop.Outlook.OlItemType.olMailItem);

            oMsg.HTMLBody = mail_msg.Message;
            oMsg.Attachments.Add(mail_msg.Attachments);
            oMsg.Subject = mail_msg.Subject;
            // Add a recipient.
            Microsoft.Office.Interop.Outlook.Recipients oRecips = (Microsoft.Office.Interop.Outlook.Recipients)oMsg.Recipients;
            // Change the recipient in the next line if necessary.
            Microsoft.Office.Interop.Outlook.Recipient oRecip = (Microsoft.Office.Interop.Outlook.Recipient)oRecips.Add(mail_msg.To);
            oRecip.Resolve();
            if (chkDisplay.Checked == true)
            {
                oMsg.Display();
            }
            else if (chkDisplay.Checked == false)
            {
                oMsg.Send();
            }

            oRecip = null;
            oRecips = null;
            oMsg = null;
            oApp = null;

            MessageBox.Show("Email Sent!");
            this.Close();

       }

It destructs it inside the method, but my confusion is whether I need to declare a new one or not each time. Having a slow day.

 

[stricken]

In the first pattern you are (alloc)ating memory and releasing it with each loop.
In the second, you are basically using the instance in a static context, where you alloc the memory once and re-use the object.

For sending a mail it probably doesn't matter, but if you look at what these patterns would do at scale, the first would produce a very bumpy RAM utilization graph, as objects are constantly instantiated and eventually released / garbage collected.

At high load, using the second pattern would result in a smoother RAM utilization graph as the same piece of memory is re-used over and over.

I am a big fan of using static wherever possible. Taking a codebase with 100 000's of lines of codes, instantiating objects that could have been static all over the place can produce a performance hit on your system. When the difference is processing 1000 transactions per second vs 10000 on the same metal, scaled over n machines, the cost implications of inefficient code become significant.

But then it depends on the nature of the object :

depending on the complexity of the object, you might have properties that carry over from one iteration to the other

The object needs to be immutable, and carry over no implicit state as it is re-used... and because you have no control over a 3rd part lib / api, the first pattern is recommended in this case.

/my 2c

 

[cguy]

The second one is in most cases more efficient, but it is cleaner to only have an object in the scope that uses it. You will have to make a call on style vs performance, depending on the constraints of the project. E.g., here I expect that the garbage collector can easily keep up with the performance of your mail sender. If you were putting Vector3D's into a vertex list for real time rendering or some such, it would be far better to create the object outside the loop.

 

[Solarion]

Both. Depending on scope, various assets of a segment of code will destruct when possible. Obviously this depends on the garbage collection method of the language\compiler you are using too.

I think of it this way. If I need to address the variable elsewhere, it needs to be visible to my code so declare it outside of any "self containing" loops according to the scope required. if it is only utilized within the loop, then declare it within. Yes, you could use the same variable over and over but depending on the complexity of the object, you might have properties that carry over from one iteration to the other that are not desired, so in my mind I'd rather re-create.

I think that might be better in this case. I don't want or need anything to carry over.

 

[MrGray]

Performance is slightly better when a variable is declared outside of the loop, but it probably wouldn't make any noticeable difference these days. Scarily, I've been around for long enough that when I started programming it did make a difference.

 

[Kosmik]

I think that might be better in this case. I don't want or need anything to carry over.

Just keep in mind the performance as others have stated but I doubt you are creating a major mass spam mailer I hope and rather just a basic notification

Sahams option gives you the best of both if you need it but it's really up to your discretion and scope.

 

[Solarion]

Thanks guys all of you you've given me new insight into variable declaration. This will only send approx 10 emails at a time so in that case I will declare it inside the loop.

Elsewhere I have another call which will send roughly 300 invoices at a time. Perhaps better to declare outside then. I will have to thoroughly check if anything will get carried over in that case.

 

[Solarion]

I agree, but in this case there is something else to consider as well:
If the mail_message object is created outside the loop then it will be re-used for each call to SendEmail. I know nothing about this call, but if SendEmail keeps the object for later reference then each iteration of the loop will overwrite the previous message's data.

It is much safer to just create a new object each time.

The only reason to re-use the same object is if it is a lot of work to create a new one, or it uses a lot of memory, but those are rare and the documentation would definitely describe how to re-use them safely.

If you needed the variable outside the loop (for checking the last iteration of the loop) then you can declare the variable outside, but create a new instance inside:

Thanks, agree 100%

 

[Shi]

Thanks guys all of you you've given me new insight into variable declaration. This will only send approx 10 emails at a time so in that case I will declare it inside the loop.

Elsewhere I have another call which will send roughly 300 invoices at a time. Perhaps better to declare outside then. I will have to thoroughly check if anything will get carried over in that case.

I think it is best to keep it the same as this process is used to send emails related to billing (which usually only happens once a month). Also, it is advisable to run your billing process on a separate machine so that it doesn't impact on any users.

 

[Solarion]

I think it is best to keep it the same as this process is used to send emails related to billing (which usually only happens once a month). Also, it is advisable to run your billing process on a separate machine so that it doesn't impact on any users.

True.

This whole billing process may take an hour or so to run once a month so I will have to run it on a separate machine.

 

[Saham]

... I've been around for long enough that when I started programming it did make a difference.

Ah, yes, the days when small memory was normal for an affordable CPU, when the amount of heap & stack space made a difference to how we designed our data structures, and there was no garbage collector so we had to make sure to free memory when we were done with it. At some point I had to teach a graduate how to do Strings in C because he only knew C++.

 

[Kosmik]

Ah, yes, the days when small memory was normal for an affordable CPU, when the amount of heap & stack space made a difference to how we designed our data structures, and there was no garbage collector so we had to make sure to free memory when we were done with it. At some point I had to teach a graduate how to do Strings in C because he only knew C++.

PrintF vs cout?

My memory might be failing me though.

 

[[)roi(]]

/edit on review I missed some great advice given (my bad)

It is much safer to just create a new object each time.

...The only reason to re-use the same object is if it is a lot of work to create a new one, or it uses a lot of memory, but those are rare and the documentation would definitely describe how to re-use them safely.

I am a big fan of using static wherever possible....

The object needs to be immutable, and carry over no implicit state as it is re-used... and because you have no control over a 3rd part lib / api, the first pattern is recommended in this case.

Obviously this depends on the garbage collection method of the language\compiler you are using too....

...you might have properties that carry over from one iteration to the other that are not desired, so in my mind I'd rather re-create.

In Summary; there is no obvious absolute with the approach here; but one approach can certainly lead to some surprising bugs. Plus talking about performance on mailer loop is a bit ridiculous if the max tops out at 300; and really who talks performance without any mention of concurrency. As for the envisaged benefits of the setup and tear downs within the loop; this not only implies an in-depth analysis of PHP's GC but also an assumption that the value assignments don't themselves result in new heap allocations.

... if this anyway really needed to scale, then immutability would be a greater win over premature optimisation i.e. it's easier for concurrency, which is after all where performance diverges.

Plus we shouldn't overlook that the quoted code is using Microsoft's Outlook Interop with a setup / tear down every loop, which is likely a bigger issue than the one in focus... but even then I doubt this Interop's internals were designed to deal with any heavy loads and/or concurrency.

 

[rward]

If performance is an issue then:

put the variable inside the loop.
time /usr/bin/php script.php

put the variable outside the loop
time /usr/bin/php script.php

See which one is quicker. Use the smae email address in all the emailsso you work to the same delivery path.