webdevs.blog

The code smells are places in the system architecture that indicate problems that require more attention. These are mostly connected with code readability, and even sometimes with system optimisation so we should give special attention to them.

The article describes the most common code smells that we try to avoid during our development process.

Duplications

We should avoid duplicating specific parts of the system for no apparent reason. Keeping the codebase smaller always will be more profitable for the application than using unnecessary large modules that are caused just by all laziness. We should always think about possible ways that will allow us to create the less complicated code, and decide on duplication only if we have strong arguments.

Let’s analyse the following code that prints the boxes with simple information and different placement.

@if(! empty($data->title_top))
  <div class="block__point block__point--top">
    <h5>{{ $data->title_top }}</h5>
  </div>
@endif

@if(! empty($data->title_right))
  <div class="block__point block__point--right">
    <h5>{{ $data->title_right }}</h5>
  </div>
@endif

@if(! empty($data->title_bottom))
  <div class="block__point block__point--bottom">
    <h5>{{ $data->title_bottom }}</h5>
  </div>
@endif

@if(! empty($data->title_left))
  <div class="block__point block__point--left">
    <h5>{{ $data->title_left }}</h5>
  </div>
@endif
Code language: Twig (twig)

The only differences are placement postfixes in the classes and variable names, and that’s too little for treating that as correct. We should consider the consequences of such decisions.

Let’s assume that there will be another eight placements soon. With this flow, we’ll have to add another 46 lines of code that do the same thing. Again, let’s assume that we need to change variable names after code review. We’ll have to review every single block and change the values manually. And again – let’s assume that each box should include additional information. We’ll have to modify every single block and add the new tag there. It indicates the problem perfectly.

One of the possible solutions is creating the data structures that will allow rendering the code without duplication. The array may be a great choice. Another option is to create a new component and just reuse it by passing specific values. We should always consider a more optimized approach.

$data->titles = [
  'top' => 'Top Title',
  'right' => 'Right Title',
  'bottom' => 'Bottom Title',
  'left' => 'Left Title'
]

// Template
@for($data->titles as $placement => $title)
  @if(! empty($title))
    <div class="block__point block__point--{{ $placement }}">
      <h5>{{ $title }}</h5>
    </div>
  @endif
@endfor;
Code language: PHP (php)

The duplications could be a problem not only on the code layer but also on the architectural layer. Let’s analyse the following HTML structures.

I’m sure that many developers may try to implement the following designs separately. But isn’t it a duplication process? The only differences are the styles and the background, so the best choice will be creating one component that will be changed using CSS modifications and simple conditions.

It shows that we should think about the consequences of duplication processes all the time.

Spaghetti

We should avoid mixing two or more layers in one file as much as possible. Such decisions make the code understandable, ugly and not maintainable in long term. Also, the teamwork is getting complicated because of no clear border between two different layers.

public function widget($args, $instance)
{
  echo $args['before_widget'];
  echo '<div class="support-widget">';
    echo '<div class="support-widget__title">'.$args['before_title'].apply_filters('widget_title', $instance['title']).$args['after_title'].'</div>';
    echo '<div class="support-widget__block"><h5>'.get_field('available', 'widget_'.$args['widget_id']).'</h5>';
    $available_hours = get_field('available_hours', 'widget_'.$args['widget_id']);
    foreach( $available_hours as $item ):
      echo '<p class="support-widget__hours">'.$item['item'].'</p>';
    endforeach;
    $email = get_field('email', 'widget_'.$args['widget_id']);
    echo '</div>';
    echo '<div class="support-widget__block">';
    foreach( $email as $item ):
      echo '<h5 class="support-widget__title">'.$item['title'].'</h5>'; 
      echo '<p class="support-widget__url"><a href="mailto:'.$item['address'].'">'.$item['address'].'</a></p>'; 
    endforeach;
    echo '</div>';
    $phone = get_field('phone', 'widget_'.$args['widget_id']);
    echo '<div class="support-widget__block">';
    foreach( $phone as $item ):
      echo '<h5 class="support-widget__title">'.$item['title'].'</h5>'; 
      echo '<p class="support-widget__url"><a href="tel:'.$item['number'].'">'.$item['number'].'</a></p>'; 
    endforeach;
    echo '</div>';
  echo '</div>';
  echo $args['after_widget'];
}
Code language: PHP (php)

Mixing the code in such a complicated way is strongly not recommended in the development process. We should consider separating different layers, for example using MVC model. The solution for the problem above is super simple: Move the code responsible for preparing the data to the controller that will just pass all the information to the template file.

public function widget($args, $instance)
{
  echo $args['before_widget'] . template('partials.support', $args) . $args['after_widget'];
}
Code language: PHP (php)

Unnecessary Variables

We should avoid creating additional variables that suppose to improve code readability. In most cases, this process is counterproductive and has no additional value.

@php $recentPosts = $data->recent_post @endphp
<div class="recent-posts">
  @foreach($recentPosts as $post)
    {{ $post->ID }}
  @endforeach;
</div>
Code language: Twig (twig)

Why not to use $data->recentPosts variable? What additional value the $recentPosts has? None. The code below looks much cleaner.

<div class="recent-posts">
  @foreach($data->recentPosts as $post)
    {{ $post->ID }}
  @endforeach;
</div>
Code language: Twig (twig)

Unnecessary Operations

We should not generate unnecessary operations. The solution below takes care of setting the $data->otherPosts based on the $recentPosts. The $data->recentPosts variable should be used if is set, otherwise the system should load some objects from the database.

$recentPosts = get_posts([
  'posts_per_page' => '10',
  'order' => 'asc'
]);

if ($data->recentPosts) {
  $data->otherPosts = $data->recentPosts;
} else {
  $data->otherPosts = $recentPosts;
}
Code language: Twig (twig)

What is the problem here? The get_posts function fires even when is not necessary, and that’s bad behaviour. The solution is simple:

$data->otherPosts = $data->recentPosts ?? get_posts([
  'posts_per_page' => '10',
  'order' => 'asc'
]);
Code language: Twig (twig)

Conditions

We should avoid generating duplicated operations just for creating conditions.

@if(! empty(get_field('title')) )
  <div>
    {{ get_field('title') }}
  </div>
@endif
Code language: Twig (twig)

This code states: “Get the title and check if not empty. If the title is not empty, get the title and display it.” Why force the code to load the same value two times? If we assume that the operation will be demanding and one query will take 5 seconds, we increase the performance two times! Just use previously generated value.

@php $title = get_field('title') @endphp
@if(! empty($title) )
  <div>
    {{ $title }}
  </div>
@endif
Code language: Twig (twig)

Code Style

The code should be always correctly formatted based on the rules provided in the project. We recommend to set up the linters (phpcs, eslint), that validates that automatically.

SWITCH

In the case of having more conditions that bases on one specific value, we should use switch statement instead of more if's.

// Not Recommended
if ($data === '1') { ... }
elseif ($data === '2') { ... }
elseif ($data === '3') { ... }
else { ... }

// Recommended
switch ($data) {
    case '1':
      break;
  case '2':
      break;
  case '3':
      break;
    default:
      break;
}
Code language: PHP (php)

Empty Tags

Do not leave empty tags in the HTML structure. Put the conditions in the place that will just don’t display the whole tag if the value doesn’t exist.

Naming Convention

Do not duplicate prefixes in variable names. If we have a contact block that should display more details, there is no need to create the following names:

<div class="contact">
  <div class="contact__name">
    {{ $contactName }}
  </div>
  <div class="contact__description">
    {{ $contactDescription }}
  </div>
  <div class="contact__photo">
    {{ $contactPhoto }}
  </div>
</div>
Code language: Twig (twig)

<div class="contact">
  <div class="contact__name">
    {{ $name }}
  </div>
  <div class="contact__description">
    {{ $description }}
  </div>
  <div class="contact__photo">
    {{ $photo }}
  </div>
</div>
Code language: Twig (twig)