How to refactor this setTags method?

Question

I want to follows a rule one function do one thing only. So i need to refactor this method. That do many thing to do a task (setTags). Mostly inside foreach the code will re-use to other method (setGenres, setLanguage, setCharacter, setCountries) with same style code.

This raw original method. in bottom my refactor method.

public function setTags($tags)
{
    foreach ($tags as $name)
    {
        $tag = Tag::firstOrCreate([
            'name' => ucwords($name)
        ]);

        if (empty($this->media->tags()->where('tag_id', $tag->id)->exists())) {
            $this->media->tags()->attach($tag->id);
        }
    }
}

Refactor method

// is it right to name it createModelValue?
public function createModelValue($model, $value) {
  $collection = $model::firstOrCreate([
                  'name' => ucwords($value)
                ]);

  if (!empty($collection)) {
    return $collection;
  }else{ return NULL; }
}

// new problem how to pass $model (tag) as function relationship to replace tags function
public function setModel($model) {
  if (empty($this->media->tags()->where("{$model}_id", $model->id)->exists())) {
              $this->media->tags()->attach($model->id);
  }
}

user_2760986 · Answer

Laravel has a sync() method that will come in handy here, instead of attaching each tag individually, you can just say attach all tags with following id and remove everything else.

$tagsCreated = collect($tags)->map(function (string $tag) {
    return Tag::firstOrCreate([
        'name' => ucwords($tag)
    ]);
});

$this->media->tags()->sync($tagsCreated);

In all honesty, being explicit in code is sometimes better than being clever. Your first version of your method is very specific and clearly indicates what it does. Your refactored clever version, i was like "uh whats going on". So choosing between these two, i would like the one above. Also your method only does one, it set tags from an array of strings. Could that be optimized yes, but very minor. The principle is more applicable if you have an email service that sends emails, you should not start to create pdf's at the same time.

I have done similar logic as the refactored but consider how big the project is and how many times you repeated the logic before you start that.

A generic refactor could be as simple as this, if you are feeling adventurous you could implement tags as a trait and/or an interface.

function setTags($model, $tags) {
    $tagsCreated = collect($tags)->map(function (string $tag) {
        return Tag::firstOrCreate([
            'name' => ucwords($tag)
        ]);
    });

    $model->tags()->sync($tagsCreated);
}

How to refactor this setTags method?

1 Answers