Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

secondman's avatar

Refactor to Collection Methods

I have this ugly little beast here that I'd love to refactor to using collection methods, but try as I might I can't seem to get it to work:

foreach ($this->record->substances as $substance) {
  if ($substance->listings->isNotEmpty()) {
    foreach ($substance->listings as $listing) {
      if ($listing->sublist_id === $this->listId) {
        $this->listErrors->push($substance);
      }
    }
  }
}

Any ideas?

0 likes
12 replies
Sinnbeck's avatar

I assume $this->listErrors is an array?

$errors = $this->record->substances->map(function($substance) {
  if ($substance->listings->isNotEmpty()) {
    foreach ($substance->listings as $listing) {
      if ($listing->sublist_id === $this->listId) {
        return $substanceń
      }
    }
  }
})->filter()->values();
 $this->listErrors = array_merge($this->listErrors, $errors);
webrobert's avatar
collect($this->record->substances)
    ->filter( fn($substance ) => $substance->listings->isNotEmpty() && 
        collect($substance->listings)
            ->filter(fn($listing) => $listing->sublist_id === $this->listId)
            ->IsNotEmpty()
    )
    ->each( fn($substance) => $this->listErrors->push($substance) );

ha, its funny as I was doing this.. I was thinking, it really depends what else you are doing. There are a handful of ways this could go.

1 like
secondman's avatar

@webrobert

Perfect. I get confused because the items are already collections so I expect them not to need the collect helper method to wrap them, my initial thought is always to do

$this->listErrors = $this->record->substances->filter(function($substance) {
	if ($substance->listings->isNotEmpty()) {
		$substance->listings->map(function($listing) use($substance) {
			if ($listing->sublist_id === $this->listId) {
				return $substance;
			}
		})
 	}
});

But this didn't work ... it returned substances that didn't match.

One of these days I'll get my head around these damn collections.

Thanks again.

Sinnbeck's avatar
Sinnbeck
3 years ago
Best Answer
Level 102

@secondman So ->listErrors is empty? I assumed you wanted to merge into it

Then I would do

$this->listErrors = $this->record->substances->filter(function($substance) {
    return $substance->listings->isNotEmpty() && $substance->listings->contains('sublist_id', $this->listId);
})->values();
1 like
webrobert's avatar

@secondman, I fell asleep and lost my best answer to @sinnbeck!

See what I get for making best answers before bed. ;)

I wrapped it because I didnt know if it was already. and collect is smart enough not to wrap it twice.

dd is always helpful to see what is happening

dd(
    $wrapped = collect($this->record->substances), 
    $this->record->substances, // was it already a collection? database collection?
    collect($wrapped)
);

contains was also nice refactor

Getting started with collections... if you understand filter, map, flatMap, pluck and how to unpack the collection again.. values, all, toArray then you have a great start. dd and have fun. Then you can start exploring all the other methods as needed.

1 like
webrobert's avatar

@Sinnbeck haha, its cool. It a balance here. Also I get what its like to be the OP and be wanting an answer. So making someone wait 6 hours to get one is silly. #WorldDominationAnswersUnited. PS I broke top 50, shhh.

1 like
Sinnbeck's avatar

@webrobert I know the feeling though. I wrote my first on my phone, and had misunderstood the data :)

Congrats!! I just made "Most best answers of anyone". *sshhhh

1 like

Please or to participate in this conversation.