Array input to function, a new approach

[djr33]

I was reading through some third party code and I had an idea of how to approach something differently. I think I've also used the method in the third party code, so now I'm questioning that, and I have a new idea. In the end it's roughly the same, but I think it's simpler. It certainly involves less tabs, which is better for readability.


Sometimes we can to do operations in bulk on an array rather than just on a single value. For example, let's assume we want to do something to a number of database entries.

As an example, let's assume we want to ban users. Here's what the standard way would look like:


function banusers($users) {
if (!is_array($users)) {
$users = array($users); //make a single input into an array
}
foreach($users as $user) {
if (/*$user is a valid input and permissions are right*/) {
/*actually ban the user here*/
}
}
}

That's fine, but the more I think about it, that's counterintuitive. Why force an array when the goal is to operate on individual items?

Would this be more logical?

function banuser($user) {
if (is_array($user)) {
foreach($user as $u) {
banuser($u);
}
}
if (/*$user is a valid input and permissions are right*/) {
/*actually ban the user here*/
}
}
Shouldn't the multiple variant be the exception?


Again, I'm just thinking out loud here. Either one works, and this isn't a "problem" to be solved.

[jscheuer1]

You would have to time it and/or do other diagnostics while each were running to know if there were any difference. It looks to me that the first one takes just slightly longer if it's a string, but not much because it's only one in any case. The second looks to take longer for each iteration if it's an array. If it's a large array, and I'm right, that might add up. In most cases it just won't matter which is faster. And, yes, they're otherwise equivalent. At least as far as I can see.

[djr33]

Mostly I'm thinking about this from a design perspective, not a performance perspective. It makes little sense to me to fake an array instead of properly parsing an array as an exception.

As for time, I think it might be faster the new way. You're correct about the first iteration of the function, but that's not going to add up to much unless you have an enormous array. Instead, I *think* running a loop around a function call is less work than running a loop around something more complicated like a database operation. But I might be wrong. Anyway, that's secondary in my mind to the more coherent code it produces.

[Nile]

It really depends on how everything is set up and how your domain logic works. I guess I would consider any problem like this arising from poor design in the first place. Normally, for each user I'd have an instance (User) and a mapper for that user instance (UserMapper). Then to ban the user, we'd just call a setBanStatus (or something) method on that User instance. In the source, we'd see something like:


<?php
class User extends DomainObjectAbstract
{
protected $id = null;
protected $banStatus = null;
//protected $username, etc...

public function setBanStatus ( $status )
{
$this->banStatus = $status;
}

//additional setter methods

public function getBanStatus ()
{
return $this->banStatus;
}

//additional getter methods

}

Then to map the user to the database, you'd want a class capable of CRUD -- create, read (aka, fetch), update (aka, save), delete (aka remove... below, create/update function in the same method):


<?php
class UserMapper extends DataMapperAbstract
{
//the abstract class will take care of our database handle
//fetch and remove methods which use a `User`'s getId() method to target a user

public function save ( User $user )
{
$id = $user->getId();
$ban = $user->getBanStatus();

$prepare = $this->connection->prepare("
INSERT INTO Users (id, banStatus)
VALUES (:id,
:banStatus) ON DUPLICATE KEY
UPDATE banStatus = :banStatus2
");

$prepare->bindParam( ':id', $id, \PDO::PARAM_INT );
$prepare->bindParam( ':banStatus', $banStatus, \PDO::PARAM_STR );
$prepare->bindParam( ':banStatus2', $banStatus, \PDO::PARAM_STR );

$prepare->execute();

}
}

You could essentially ban a single user like:


$bob = new User();
$bob->setId( 5 );

$bobMapper = new UserMapper();
$bobMapper->fetch ( $bob );

$bob->setBanStatus( 1 ); // 1 = ban?

$bobMapper->save ( $bob );

For multiple users I'd use a collection pattern, which is essentially a glorified array. Though the User interface I've provided above is subject to change, collection patterns will undoubtely change depending on how you need to use them. You could create a UserCollection, set conditions on the UserCollection in a setBanStatuses method, then when you need to update it in the UserCollectionMapper (which wouldn't need to have a create method, allegedly... depending on your needs, of course), just select all the users and update them.

Another approach is to violate dependency injection (which, in some ways, your original idea to create an array from a single instance does) and create a user mapper within the user collection mapper, but that seems like a bad idea entirely and could get extremely method.

Probably a better approach (and this depends entirely on how your database is setup) is to have a BanUser and BanUserCollection method, but then, as I said before, you'd probably also have a table of banned users in your database, which doesn't sound too fancy either. Another sloppy idea, the ban classes could possibly access the user table themselves. Rereading, my first idea seems the most logical, but again it all depends on how the logic in your interface works, how the database structure works, and how you want to handle different functions like updating users.

Though this is less of a response and more of a "reprogram what you're doing." In any case, I'm sure finding a nice balance between what you're doing and clean coding/interfacing/designing is possible.

[djr33]

To be clear: I'm not trying to ban users. That was just an example. There are times when we want to modify the database directly-- perhaps we want to clear a certain set of error logs.
As for your example, I don't see why you'd have active objects for each of the users on the whole website! You'd have to load them, and a function like mine (either version) would allow you to do that. And you don't deal with multiple users in the example. That's what's relevant, not how to ban or whether we're using OOP.

Anyway, I'm just talking generically: if you want alternatively string or array input to a function, what is the best way to do it? I suggest recursively applying the function to the array, rather than forcing the string into an array and looping through that array while changing it.

[Nile]

To be clear: I'm not trying to ban users
I see. If this isn't user-specific or interface-related (i.e., this may be for just one function you're creating for a small, 1-page project or something), you wouldn't really need to pull in all the advanced design patterns.


As for your example, I don't see why you'd have active objects for each of the users on the whole website!
Actually my example functions the opposite. All of your users are straight from the database until you need them. At that time, you'll create a temporary instance. Then, if you modify it, just update the user instance and wallah. For more information search domain model/domain object/data mappers on Google. In other words, the User instances are just temporary holders of all-things related to that user. It's a way of accessing that information, not of storing it. Really, if you're using practical, SOLID (http://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29), non-spaghetti code principles, you should have an instance for things like this anyway.



And you don't deal with multiple users in the example.
I thought I did. That's what a UserCollection is for.


Anyway, I'm just talking generically: if you want alternatively string or array input to a function, what is the best way to do it? I suggest recursively applying the function to the array, rather than forcing the string into an array and looping through that array while changing it.
I see what you're saying, though. Forcing an array seems like a violating of depency injection, if we consider the array a dependency (which it technically isn't, but it still seems a bit sketchy). The self-calling style seems better design-wise, but I don't see why we wouldn't just let the client inject an array themself. Though it's important to remember DRY (http://en.wikipedia.org/wiki/Don%27t_repeat_yourself), it's also important to remember YAGNI (http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) and POGE (http://en.wikipedia.org/wiki/Principle_of_good_enough).

[djr33]

I see. If this isn't user-specific or interface-related (i.e., this may be for just one function you're creating for a small, 1-page project or something), you wouldn't really need to pull in all the advanced design patterns.I'm talking about function design. There are no specifics. The banning example was just an illustration.
I happen to be looking through some forum software (SMF) while trying to modify it. This strategy is used often. In this case, it's about marking a topic or an array of topics as unread. A lot of code out there isn't OOP. If you're doing something different where you have no arrays anyway, then obviously this doesn't apply.



I thought I did. That's what a UserCollection is for.It's not illustrated in your code-- at some point you're going to need a loop. I'm suggesting that at that point you'd be better off recursively applying the function to the array, rather than faking a non-array input as an array. However you do this, you'd run into that situation-- a loop by default, or a loop only when needed. I'm saying only when needed.


I see what you're saying, though.Ok :)
That's all. It's just a thought.

Forcing an array seems like a violating of depency injection, if we consider the array a dependency (which it technically isn't, but it still seems a bit sketchy). The recursive style seems better design-wise, but I don't see why we wouldn't just let the client inject an array themself. I don't follow. The point is that the array must at some point be treated as a set of strings which are handled separately. All I'm talking about is how to do that-- either treat a string as an array or treat the array as a set of strings. Personally I like the option I have above, for code design purposes.
It is, after all, illogical to use an array as input to a string function-- if you do that, it's an exception. Treat it like one.

[Nile]

It's not illustrated in your code-- at some point you're going to need a loop. I'm suggesting that at that point you'd be better off recursively applying the function to the array, rather than faking a non-array input as an array. However you do this, you'd run into that situation-- a loop by default, or a loop only when needed. I'm saying only when needed.

I got lazy and didn't feel like drawing out another example, but the only place you'd need to use a loop is to get all the user ID's from their instance (getId()). Once you have an array of IDs, you can just append them to a string (or just implode them on a comma) and use that string in your query to update the database. Though I know we're not talking about this situation anymore.


I don't follow. The point is that the array must at some point be treated as a set of strings which are handled separately. All I'm talking about is how to do that-- either treat a string as an array or treat the array as a set of strings. Personally I like the option I have above, for code design purposes.
It is, after all, illogical to use an array as input to a string function-- if you do that, it's an exception. Treat it like one.

Well this is where it varies depending on the situation. In reality, the function you have isn't necessary a string function (by that I assume you mean it operates based on the strings in the array). If we're talking about users, they could be instances, and then, like I said, it'd be logical to have a collection. Though, again, I definitely do find your self-calling solution better than the first and I'd be interested to hear what others say.

[djr33]

I got lazy and didn't feel like drawing out another example,...Ok, but you have to understand my confusion: you literally only gave examples of code unrelated to the original question of how to deal with multiple values as array input in a function.


Well this is where it varies depending on the situation. In reality, the function you have isn't necessary a string function (by that I assume you mean it operates based on the strings in the array). If we're talking about users, they could be instances, and then, like I said, it'd be logical to have a collection.Ok, sure. Wouldn't that still require some kind of loop at some point, though?


Though, again, I definitely do find your recursive solution better than the first and I'd be interested to hear what others say. Ok, great. And there might be a better way to approach all of this unrelated to the function. But assuming you're choosing (a) or (b), I'd say choose (b). I think we agree there :)

[For the record, I like SMF in general, but the code isn't all that pretty. It's often very hard to find out what's going on. Maybe OOP would help. I'm not really defending the design in general, just wondering about that specific instance.]

[Nile]

Ok, but you have to understand my confusion: you literally only gave examples of code unrelated to the original question of how to deal with multiple values as array input in a function.
Yes, sorry about that. I figured establishing how things would generally be laid out would allow an understanding of the general basis of how the pattern works. Then it'd be easier to explain how manipulating multiple instances would work (once already drawing out how individuals would work). Though I see how it's confusing.

[djr33]

Alright. That's fine. And we seem to agree about the original question. (Please I don't disagree about your other points.) :)

[traq]

regarding your original question,
function something( $input ){
if( is_array( $input ) ){
foreach( $input as $item ){ /* do something */ }
}
else{ /* do something */ }
}

# VS. #

function something( $input ){
$input = (array) $input;
foreach( $input as $item ){ /* do something */ }
}
…definitely #2. You don't duplicate any code, which makes everything more succinct and less error-prone. Remember, DRY (http://en.wikipedia.org/wiki/Don't_repeat_yourself).

[djr33]

Hmm... that's slightly different.

First, your code is more succinct than the examples I was looking at earlier (setting the type rather than checking if it's an array and if so doing something special).

Second, I don't see why there's any "repeating" going on with either example from the first post, actually. They may not be optimized, but I don't think it's due to repeating in any sense that would matter.



function something( $input ){
$input = (array) $input;
foreach( $input as $item ){ /* do something */ }
} That looks very clean, I admit. But it still seems awkward and undesirable to me. Two reasons:
1. This function is about strings (or ints, etc.), not about arrays. Using everything as an array is backwards, though it works.
2. For reasons of tabs and at least apparent simplicity, wrapping the contents of the entire function in a foreach loop (it might be 1000 lines, who knows) seems undesirable to me. Keeping the actual purpose of the function at a less indented level (literally and figuratively) just makes more sense to me. Here's an example:


//here's the simple version:
function notify($email) {
mail($email,'Notification: standard message.');
}

//but now we might want to sometimes allow an array as input, rather than in many places using a foreach loop for that (and violating the "DRY" idea above

//traq's idea:
function notify($email) {
$email = (array) $email;
foreach($email as $e) {
mail($e,'Notification: standard message');
}
}

//my idea:
function notify($email) {
if (is_array($email)) { //ah, an array, let's get rid of this
foreach($email as $e) {
notify($e);
}
return 'array'; //or whatever you want there
}
else if (!is_string($email)) { //uh-oh, bad input, don't even start running the core of the function
return false;
}
//pre-processing done, moving on to core of the function

//do whatever you want, no complications:
mail($email,'Notification: standard message');
}
Clearly my version looks like the original function more. It just involves an exception at the top, and you can also include your type/security checking there (in my "else if" block) rather than embedding it somewhere below, awkwardly, in the foreach. Seems preferable to me. It's not the shortest code, nor necessarily the fastest to process, though, I admit.



Interestingly, I notice that my version is actually recursive which is potentially convenient if you have multi-level arrays. I don't know that it would occur often, but you could just combine a few different arrays together and they'd all run smoothly through that. With a single layered foreach loop that wouldn't work.




(You're right about needing an "else" in my original code, example #2. Thanks. I might actually use a "return" statement instead, but the same idea basically.)





--

The more I think about it, Nile's original point might be right. It's very hard to come up with a convincing example of when this would really be the best way to design a function. But it's certainly a common practice from what I've seen...


There's also a very weird result here where you can't conveniently have a return value for the whole process. Using arrays in this case is a little awkward actually. Convenient to a degree I suppose.

[Nile]

I do agree with @traq that injecting the array is probably the way to go.


Interestingly, I notice that my version is actually recursive which is potentially convenient if you have multi-level arrays. I don't know that it would occur often, but you could just combine a few different arrays together and they'd all run smoothly through that. With a single layered foreach loop that wouldn't work.
I noticed this too. Interesting.


The more I think about it, Nile's original point might be right. It's very hard to come up with a convincing example of when this would really be the best way to design a function. But it's certainly a common practice from what I've seen...
I'm obligated to agree :). Of course we always need to remember that tons of "common practice(s)" we see are complete violations of general rules (or principles*) of programming.

In my opinion, depending on how convoluted those functions get, what we're really looking at in the examples are violations of separation of concerns. Ideally, if this is a complicated process, there should be something dealing with collections and something dealing with individual instances. After all, you're right that:

This function is about strings (or ints, etc.), not about arrays. Using everything as an array is backwards, though it works.
So to separate the concerns, we should just give them different methods/controls/classes/etc.. (of course, there are situational choices to make). Also, note that I'm not talking about SRP (single responsibility principle). I do agree that these are very similar responsibilities (situationally, again, this could change), but if these are genuine concerns dealt with by jamming multiple functionalities into one function, separation needs to take place.

*Making this clarification because I once told a friend they were violating the single responsibility principle. His response was "there are no rules in programming." :facepalm:

[djr33]

Right. The whole thing is collapsing what should be, perhaps, two functions, one for strings, and another for passing array items as strings to that first function. So myfunction($string) and myfunction_forarrays($array). Basically what I have above, but not shoved into a single function. Maybe that actually is the real answer. Don't take shortcuts then it's silly to do so. But that would mean remembering which function takes only strings and which one takes only arrays when referring to them later.

[Nile]

Don't take shortcuts then it's silly to do so. But that would mean remembering which function takes only strings and which one takes only arrays when referring to them later.
Consistency beats this one, of course. I'm positive I've seen users name multiple patterns different things in their code, and it's awful. If I'm going to make multiple functions that deal with arrays in the same way by calling the instance-based function, I'm definitely going to use the same naming patterns.

[djr33]

Ah. Well, naming should be consistent (though it often isn't, especially when multiple designers are involved in an open source project). But I just meant in general that you'd need to remember that there are two functions and when to use each, rather than just assuming the basic (string) one always works. It wouldn't be hard if you did that consistently, but it would require some planning ahead. A good thing, probably.

[traq]

recursion is a good alternative, too.


Right. The whole thing is collapsing what should be, perhaps, two functions, one for strings, and another for passing array items as strings to that first function. So myfunction($string) and myfunction_forarrays($array). Basically what I have above, but not shoved into a single function. Maybe that actually is the real answer. Don't take shortcuts then it's silly to do so. But that would mean remembering which function takes only strings and which one takes only arrays when referring to them later.
I disagree. It's better in a single function because it's all in the same place and is easier to understand and manage. When this single function becomes too large or complex, it should become a class (or a controlling function that delegates to each type-specific function).

[djr33]

I disagree. It's better in a single function because it's all in the same place and is easier to understand and manage. When this single function becomes too large or complex, it should become a class (or a controlling function that delegates to each type-specific function). Really, though, that's a violation of DRY-- every function like this would need that same extra code. I suppose there's no good solution :p
And there are many solutions that work.