Skip to content

Coding Conventions & Standards

Jump to bottom
paranarimasu edited this page Nov 26, 2022 · 42 revisions

Note: WIP. Please contact para in the Discord for corrections or missing information.

This page exists to detail coding conventions and standards in this repository in order to assist developers in writing more consistent and cleaner code.

General

animethemes-server shall follow psr-12 extended coding style standards.

animethemes-server shall follow psr-4 autoloading standards.

Every PHP file shall contain a strict type declaration.

<?php

declare(strict_types=1);

...

Every PHP property, function argument and return value shall contain a type declaration.

protected string $name;

...

protected function formatEmbedFieldValue(mixed $value): string

...

PHPDoc

Every function and property shall contain a Docblock.

/**
 * The array of embed fields.
 *
 * @var DiscordEmbedField[]
 */
protected array $embedFields = [];

...

/**
 * Add discord embed field.
 *
 * @param DiscordEmbedField $embedField
 * @return void
 */
protected function addEmbedField(DiscordEmbedField $embedField)

Every function and property shall contain a short description.

/**
 * Filter set specified by the client. <-- short description
 *
 * @var Query
 */
protected Query $query;

...

/**
 * Build Elasticsearch query. <-- short description
 *
 * @return SearchRequestBuilder
 */
abstract public function buildQuery(): SearchRequestBuilder;

Every var, param and return annotation shall declare the most explicit typehint possible, within reason. If an array shape is more complex than a depth of 1 or there exist a multitude of expected types, we can leave an ambiguous array typehint.

/**
 * The list of fields to be included per type.
 *
 * @var array <-- ambiguous typehint for array that can be of depth greater than 1
 */
protected array $fields;

/**
 * The list of fields and direction to base sorting on.
 *
 * @var array<string, bool> <-- explicit typehint for associative array with expected types for key-value pair
 */
protected array $sorts;

/**
 * The list of filter conditions to apply to the query.
 *
 * @var Condition[] <-- explicit typehint for array of explicit type
 */
protected array $conditions;

Every unhandled exception shall be annotated.

/**
 * Display markdown document.
 *
 * @param string $docPath
 * @return View
 * @throws HttpException
 * @throws NotFoundHttpException
 */
protected function displayMarkdownDocument(string $docPath): View
{
    $document = Jetstream::localizedMarkdownPath($docPath.'.md');

    if ($document === null) {
        abort(404); // throws HttpException or NotFoundHttpException
    }

    ...
}

Prefer importing to using fully qualified names.

// Do this
use App\Http\Api\Filter\Wiki\Anime\AnimeSeasonFilter;

...

public static function filters(): array
{
    return [
        AnimeSeasonFilter::class,
        ...
    ];
}

// Don't do this
public static function filters(): array
{
    return [
        \App\Http\Api\Filter\Wiki\Anime\AnimeSeasonFilter::class,
        ...
    ];
}

Namespacing

Prefer "matching" the most common canonical path of usages.

// App\Contracts\Events\DiscordMessageEvent.php

// DiscordMessageEvent is used by many classes in the App\Event namespace
namespace App\Contracts\Events;
// App\Events\Wiki\Anime\AnimeCreated.php

namespace App\Events\Wiki\Anime;

...

class AnimeCreated extends AnimeEvent implements DiscordMessageEvent, UpdateRelatedIndicesEvent
// App\Events\Billing\Balance\BalanceCreated.php

namespace App\Events\Billing\Balance;

...

class BalanceCreated extends BalanceEvent implements DiscordMessageEvent

Naming

Properties, variables and functions shall use camel casing.

// Do this
protected int $createdFailed = 0;

// Don't do this
protected int $created_failed = 0;

Classes shall use pascal casing.

// Do this
class SendInvitationMail implements ShouldQueue

// Don't do this
class Send_Invitation_Email implements ShouldQueue

Names shall be of the format [a-zA-Z]+.

// Do this
return fn (Video $first, Video $second) => $first->basename <=> $second->basename;

// Do this
return fn (Video $firstVideo, Video $secondVideo) => $firstVideo->basename <=> $secondVideo->basename;

// Don't do this
return fn (Video $video1, Video $video2) => $video1->basename <=> $video2->basename;

// Don't do this
return fn (Video $first_video, Video $second_video) => $first_video->basename <=> $second_video->basename;

Visibility

Prefer protected to private visibility for properties and functions.

// Do this
protected Image $image;

// Do this
protected function getChangedAttributes(Model $model): Collection

// Don't do this
private Image $image;

// Don't do this
private function getChangedAttributes(Model $model): Collection

Limit number of public functions where possible.

// We need to apply the filter in the context of a condition object
public function applyFilter(Builder $builder): Builder

// Determining if a filter matches the intended scope is internal logic and shouldn't be exposed
protected function isMatchingScope(Condition $condition): bool

Prefer public visibility for constants.

// Do this
// We want to use this constant in testing contexts to mock the parser
public const PARAM_SEARCH = 'q';

// Don't do this
// We will not be able to use this constant in testing contexts and will need to redefine it or hard-code the value
private const PARAM_SEARCH = 'q';

Concerns

Every trait shall be contained in the App\Concerns namespace.

namespace App\Concerns\Services\Discord;

...

trait HasDiscordEmbedFields

Contracts

Every interface shall be contained in the App\Contracts namespace.

namespace App\Contracts\Repositories;

...

interface Repository

Enums

Enum class names shall prefix the model name where possible.

namespace App\Enums\Models\Wiki;

...

// Used by the Anime model
final class AnimeSeason extends BaseEnum

If the enum is used by multiple models, prefer excluding a prefix.

namespace App\Enums\Models\Billing;

...

// Used by the Balance and Transaction models
final class Service extends BaseEnum

Events

Model events shall be namespaced by the model name.

namespace App\Events\Wiki\Anime;

class AnimeCreated extends WikiCreatedEvent implements UpdateRelatedIndicesEvent

Model event names shall be of the format {Model|ObservableEvent}.

// Event class for the 'updated' observable event for the Anime model
class AnimeUpdated extends WikiUpdatedEvent implements UpdateRelatedIndicesEvent

Http

Controllers

Controller class names shall suffix Controller.

namespace App\Http\Controllers;

...

class WelcomeController extends Controller

Controller classes shall be as slim as possible. Controllers shall only contain the logic to produce a response from the request. Please make use of middleware and form requests where possible for filtering and validation.

namespace App\Http\Controllers\Wiki;

...

class VideoController extends StreamableController
{
    ...

    public function show(Video $video): StreamedResponse
    {
        // Produce the streamed response from the video
        // There are two middleware that perform additional filtering
        // 1. Check if video streaming is enabled for this environment
        // 2. Record the view count
        return $this->streamContent($video);
    }
}

Controller functions shall be named after resource actions: index, create, store, show, edit, update, destroy.

// Do this
public function show()

// Don't do this
public function do()

// Don't do this
public function encoding()

Api

API resource controller class names shall be of the format {Model}Controller.

namespace App\Http\Controllers\Api\Wiki;

...

// API controller for the Anime model
class AnimeController extends BaseController

Middleware

The middleware handle function shall first define conditions for filtering the request, possibly breaking the chain, and otherwise falling through to handling the next closure in the chain.

namespace App\Http\Middleware;

...

class IsVideoStreamingAllowed
{
    ...

    // If video streaming is not allowed, redirect the user.
    // Otherwise, continue to the next middleware in the chain
    public function handle(Request $request, Closure $next): mixed
    {
        if (! Config::get('app.allow_video_streams', false)) {
            return redirect(route('welcome'));
        }

        return $next($request);
    }
}

Every middleware shall be registered in the web kernel.

// App\Http\Kernel.php

protected $routeMiddleware = [
    ...
    'is_video_streaming_allowed' => IsVideoStreamingAllowed::class,
    ...
];

Requests

Request classes shall suffix Request.

namespace App\Http\Requests;

...

class TransparencyRequest extends FormRequest

Resources

JSON resource class names shall be of the format {Model}Resource.

namespace App\Http\Resources\Wiki\Resource;

...

// JSON API resource class for the Artist model
class ArtistResource extends BaseResource

Resource collection class names shall be of the format {Model}Collection.

namespace App\Http\Resources\Billing\Collection;

...

// JSON API collection class for the Balance model
class BalanceCollection extends BaseCollection

Resource classes shall be namespaced first by the Model namespace and then by {Resource|Collection}.

// The model is contained in the Wiki namespace and the class is a JSON API collection class
namespace App\Http\Resources\Wiki\Collection;

...

class AnimeCollection extends BaseCollection

Resources classes shall define a sensible wrap.

namespace App\Http\Resources\Wiki\Resource;

...

class EntryResource extends BaseResource
{
    ...

    // A sensible wrap
    // We need this property for scoping
    public static $wrap = 'animethemeentry';

    ...
}

Jobs

Job class names shall suffix Job.

namespace App\Jobs;

...

class SendDiscordNotificationJob implements ShouldQueue

Jobs shall explicitly define middleware and a sensible retry time limit.

namespace App\Jobs;

...

class SendDiscordNotificationJob implements ShouldQueue
{
    // define middleware
    public function middleware(): array
    {
        return [new RateLimited()];
    }

    // define sensible retry limit
    public function retryUntil(): DateTime
    {
        return now()->addMinutes(15);
    }
}

Listeners

Listeners shall be namespaced by the model name.

namespace App\Listeners\Wiki\Video;

...

class InitializeVideoTags

Models

Models shall be namespaced by concern.

namespace App\Models\Wiki;

...

class Anime extends BaseModel

Models shall have factories for testing and development.

namespace App\Models;

...

abstract class BaseModel extends Model implements Nameable
{
    ...

    use HasFactory;

    ...
}

Models shall explicitly define fillable attributes.

namespace App\Models\Wiki;

...

class Image extends BaseModel
{
    ...

    protected $fillable = ['path', 'facet', 'size', 'mimetype'];

    ...
}

Models shall prefer dispatchable events to observers.

namespace App\Models\Wiki;

...

class ExternalResource extends BaseModel
{
    ...

    protected $dispatchesEvents = [
        'created' => ExternalResourceCreated::class,
        'deleted' => ExternalResourceDeleted::class,
        'restored' => ExternalResourceRestored::class,
        'updated' => ExternalResourceUpdated::class,
    ];

    ...
}

Models shall explicitly define the table name and primary key.

namespace App\Models\Billing;

...

class Transaction extends BaseModel
{
    ...

    protected $table = 'transaction';

    protected $primaryKey = 'transaction_id';

    ...
}

Searchable models shall define relations to eager load when necessary.

namespace App\Models\Wiki;

...

class Anime extends BaseModel
{
    ...

    protected function makeAllSearchableUsing(Builder $query): Builder
    {
        return $query->with('synonyms');
    }

    ...
}

Searchable models shall define an array that matches the index schema.

namespace App\Models\Wiki;

...

class Anime extends BaseModel
{
    ...

    public function toSearchableArray(): array
    {
        $array = $this->toArray();
        $array['synonyms'] = $this->synonyms->toArray();

        return $array;
    }

    ...
}

Models shall explicitly define casts for fields that are not strings, text, or timestamps.

namespace App\Models\Wiki;

...

class Anime extends BaseModel
{
    ...

    protected $casts = [
        'season' => 'int',
        'year' => 'int',
    ];

    ...
}

Models shall attach pivot classes and timestamps when querying many-to-many relations.

namespace App\Models\Wiki;

...

class Anime extends BaseModel
{
    ...

    public function series(): BelongsToMany
    {
        return $this->belongsToMany('App\Models\Series', 'anime_series', 'anime_id', 'series_id')
            ->using(AnimeSeries::class)
            ->withTimestamps();
    }

    ...
}

Notifications

Notification class names shall suffix Notification.

namespace App\Notifications;

...

class DiscordNotification extends Notification implements ShouldQueue

Pivots

Every many-to-many relationship shall have a pivot class.

namespace App\Models\Wiki;

...

class Anime extends BaseModel
{
    ...

    public function series(): BelongsToMany
    {
        return $this->belongsToMany('App\Models\Series', 'anime_series', 'anime_id', 'series_id')
            ->using(AnimeSeries::class)
            ->withTimestamps();
    }

    ...
}

Pivot class names shall be of the format {ModelOne|ModelTwo}.

namespace App\Pivots;

...

// This class is the many-to-many relationship between Anime and Series
class AnimeSeries extends BasePivot

Pivots shall prefer dispatchable events to observers.

namespace App\Pivots;

...

class VideoEntry extends BasePivot
{
    ...

    protected $dispatchesEvents = [
        'created' => VideoEntryCreated::class,
        'deleted' => VideoEntryDeleted::class,
    ];

    ...
}

Policies

Policy class names shall be of the format {Model}Policy.

namespace App\Policies\Wiki;

...

// The policy class for the Anime model
class AnimePolicy

Policy functions shall be explicitly defined for all possible actions on the model.

namespace App\Policies\Wiki;

...

class AnimePolicy
{
    public function viewAny(): bool {}

    public function view(): bool {}

    public function create(User $user): bool {}

    public function update(User $user): bool {}

    public function delete(User $user): bool {}

    public function restore(User $user): bool {}

    public function forceDelete(User $user): bool {}

    public function attachAnySeries(User $user): bool {}

    public function attachSeries(User $user, Anime $anime, Series $series): bool {}

    public function detachSeries(User $user): bool {}

    public function attachAnyExternalResource(User $user): bool {}

    public function attachExternalResource(User $user): bool {}

    public function detachExternalResource(User $user): bool {}

    public function attachAnyImage(User $user): bool {}

    public function attachImage(User $user, Anime $anime, Image $image): bool {}

    public function detachImage(User $user): bool {}
}

Policy functions shall only inject the needed objects.

namespace App\Policies\Wiki;

...

// The policy class for the Anime model
class AnimePolicy
{
    ...

    // Neither a User or Anime object is needed
    public function view(): bool
    {
        return true;
    }

    ...

    // A User object is needed but not an Anime object
    public function update(User $user): bool
    {
        return $user->hasCurrentTeamPermission('anime:update');
    }

    // All injectible objects are needed
    public function attachSeries(User $user, Anime $anime, Series $series): bool
    {
        if (AnimeSeries::where($anime->getKeyName(), $anime->getKey())->where($series->getKeyName(), $series->getKey())->exists()) {
            return false;
        }

        return $user->hasCurrentTeamPermission('anime:update');
    }
}

Policy functions shall prefer querying for the existence of a pivot model to loading the models in the relation.

// Do this
if (AnimeImage::where($anime->getKeyName(), $anime->getKey())->where($image->getKeyName(), $image->getKey())->exists()) {
    return false;
}

// Don't do this
if ($anime->images->contains($image)) {
    return false;
}

Repositories

Repository class names shall suffix Repository.

namespace App\Repositories\Service\DigitalOcean;

...

class VideoRepository implements Repository

Repository classes shall implement the Repository contract.

namespace App\Repositories\Eloquent;

...

abstract class EloquentRepository implements Repository

Rules

Rule class names shall suffix Rule.

namespace App\Rules\Billing;

...

class TransparencyDateRule implements Rule

Factories

Factory classes classes shall suffix Factory.

namespace Database\Factories\Wiki;

...

class AnimeFactory extends Factory

Factory classes shall assign random values for each attribute.

namespace Database\Factories\Wiki;

...

class AnimeFactory extends Factory
{
    ...

    public function definition(): array
    {
        return [
            'slug' => Str::slug($this->faker->words(3, true), '_'),
            'name' => $this->faker->words(3, true),
            'year' => intval($this->faker->year()),
            'season' => AnimeSeason::getRandomValue(),
            'synopsis' => $this->faker->text,
        ];
    }

    ...
}

Seeders

Seeder classes shall suffix Seeder.

namespace Database\Seeders;

...

class SeriesSeeder extends Seeder

Routes

Every route shall be named.

// Explicitly name the route if not inferred as is done with resource registration
Route::get('/', [WelcomeController::class, 'show'])->name('welcome');

Tests

Test classes shall suffix Test.

namespace Tests\Unit\Events;

...

class UpdateRelatedIndicesTest extends TestCase

Test classes shall be namespaced to match the app namespace of what's being tested.

namespace Tests\Feature\Jobs;

...

class SendDiscordNotificationTest extends TestCase

Test classes shall only add the WithFaker trait if any test function in the class requires faker.

namespace Tests\Feature\Console;

...

class VideoReconcileTest extends TestCase
{
    use WithFaker;

    ...

    public function testDirectoryNoResults()
    {
        $fs = Storage::fake('videos');

        $fs->makeDirectory($this->faker->word()); // requires faker

        $this->artisan(VideoReconcileCommand::class)->expectsOutput('No Videos created or deleted or updated');
    }

    ...
}

Test classes shall add the WithoutEvents trait if any test function in the class creates an object AND events can be suppressed.

namespace Tests\Feature\Http\Api\Wiki\Image;

...

class ImageIndexTest extends TestCase
{
    // We can suppress events for all tests
    use WithoutEvents;

    ...
}

Test class functions shall invoke the withoutEvents function if any test function in the class requires events but this function does not.

namespace Tests\Feature\Http\Api\Anime;

...

class AnimeIndexTest extends TestCase
{
    ...

    public function testPaginated()
    {
        $this->withoutEvents(); // this test can suppress events

        ...
    }

    ...

    public function testAllowedIncludePaths()
    {
        // $this->withoutEvents(); is not called. We need events to fire for this test function.
        $allowedPaths = collect(AnimeCollection::allowedIncludePaths());
        $includedPaths = $allowedPaths->random($this->faker->numberBetween(0, count($allowedPaths)));
    }

    ...
}

Test class function PHPDoc descriptions shall describe a functional requirement of the application.

namespace Tests\Unit\Rules;

...

class ResourceSiteDomainRuleTest extends TestCase
{
    ...

    /**
     * The Resource Site Domain Rule shall return true if the link matches the site.
     *
     * @return void
     */
    public function testResourceSiteDomainRulePasses() {}

    ...
}

Test class functions shall prefer using faker to hard-coding values.

// Do this
$type = $this->faker->word();

// Don't do this
$type = 'test';

Test class functions shall prefer anonymous classes to test classes for testing interfaces and abstract classes.

// Do this
$event = new class implements DiscordMessageEvent
{
    ...
};

$job = new SendDiscordNotification($event);

// Don't do this
$event = new TestDiscordMessageEvent();

$job = new SendDiscordNotification($event);

Test class function shall prefer mocking services to invoking them.

// Do this
public function testNoResults()
{
    $mock = $this->mock(DigitalOceanBalanceRepository::class);

    $mock->shouldReceive('all')
        ->once()
        ->andReturn(Collection::make());

    // bind the mocked instance into the container so that the mock is returned when we ask for this class to be initialized
    $this->app->instance(DigitalOceanBalanceRepository::class, $mock);

    $this->artisan(BalanceReconcileCommand::class, ['service' => Service::DIGITALOCEAN()->key])->expectsOutput('No Balances created or deleted or updated');
}

// Don't do this
public function testNoResults()
{
    // The service will be invoked because we do not have a binding for the service repository
    $this->artisan(BalanceReconcileCommand::class, ['service' => Service::DIGITALOCEAN()->key])->expectsOutput('No Balances created or deleted or updated');
}

Test class functions shall target one assert for one functional requirement.

/**
 * A Discord Notification shall deliver a Discord Message.
 *
 * @return void
 */
public function testToDiscordMessage()
{
    $message = DiscordMessage::create();

    $notification = new DiscordNotification($message);

    // One assertion for one functional requirement
    static::assertInstanceOf(DiscordMessage::class, $notification->toDiscord(new AnonymousNotifiable()));
}
Clone this wiki locally