Moved email attachments into an array #1377

Closed
wants to merge 1 commit into
from

1 participant

@anthonysterling

I've consolidated the $attach(name|type|disp) properties into an
array. I think the code looks a little cleaner this way, and I could
not see a reason not to refactor.

Feedback appreciated.

Anthony.

@anthonysterling anthonysterling Moved email attachments into an array
I've consolidated the $_attach_(name|type|disp) properties into an
array. I think the code looks a little cleaner this way, and I could
not see a reason not to refactor.

Feedback appreciated.

Anthony.
00ce6be
@narfbg narfbg commented on the diff May 22, 2012
system/libraries/Email.php
@@ -1035,49 +1033,41 @@ protected function _build_message()
break;
}
- $attachment = array();
- for ($i = 0, $c = count($this->_attach_name), $z = 0; $i < $c; $i++)
+ foreach ($this->_attachments as $attachment)
@narfbg
narfbg added a line comment May 22, 2012

While the for() loop was functionally needed with the 3 arrays, it's also typically faster and there's no reason to change it to a foreach().

@philsturgeon
philsturgeon added a line comment May 22, 2012

I think for readability now that this is a multi-dim array its ok to change it to a foreach. The speed different is infintessimal and thanks to all the other speed improvements you've made we can probably afford to lose this 0.000001 second change in the 3.0 branch. :)

@anthonysterling
anthonysterling added a line comment May 22, 2012

Do you not think it's a little cleaner? It also reduces LOC by ~25% whilst doing so. Obviously your call, but I'll add a test to this and leave it with you.

Anthony.

@narfbg
narfbg added a line comment May 22, 2012

I personally would always chose for() over foreach(). But it's Phil's call as well and our arguments differ, so I guess it doesn't really matter.
What's LOC?

@anthonysterling
anthonysterling added a line comment May 22, 2012

Sorry, LOC.

@philsturgeon
philsturgeon added a line comment May 22, 2012

Let's stick with the foreach, it makes things nice and readable.

When that other commit is in and the test is written I'll be happy to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@narfbg narfbg commented on the diff May 22, 2012
system/libraries/Email.php
{
if ( ! file_exists($filename))
{
$this->_set_error_message('lang:email_attachment_missing', $filename);
return FALSE;
}
-
- $file = filesize($filename) +1;
-
- if ( ! $fp = fopen($filename, FOPEN_READ))
+
+ if ( ! $file = fopen($filename, FOPEN_READ))
@narfbg
narfbg added a line comment May 22, 2012

Okay, $file above might not be needed (+1). But we don't need to rename this one as well. :)

@anthonysterling
anthonysterling added a line comment May 22, 2012

You're right, $fp seems to be a more accepted name for a file pointer. I'll change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment