Quand j'ai revu le code, j'ai senti que je signalais toujours des points similaires, j'ai donc résumé les points de vue qui me tiennent à cœur. Il y a des perspectives qui ne dépendent pas du langage utilisé, mais comme j'utilise principalement Rails au travail, il y a aussi des perspectives qui sont spécifiques à Ruby et Rails.

Pour le moment, j'ai brièvement écrit ce à quoi je peux penser pour le moment, mais je l'ajouterai si je me souviens du point de vue qui me tient à cœur dans le futur. Il est bon de continuer à livrer (CD) à la fois des articles de développement et Qiita sans viser la perfection dès le début.

Retraits et règles de codage

Utilisez des outils d'analyse statique pour vérifier les parties statiques de votre code, telles que les retraits et les règles de codage suivants. C'est une perte de temps pour les humains de vérifier visuellement, et des fuites et un flou se produisent. De plus, je ne suis distrait que par les détails et néglige les points à souligner.

Rubocop est un outil d'analyse statique bien connu de Ruby. L'introduction des fonctions qui supportent un tel développement est souvent reportée dans le développement initial, mais il est très difficile d'insérer des outils d'analyse statique au milieu. Si vous essayez de l'ajouter plus tard, vous obtiendrez un code qui assouplit ou exempte les règles, alors assurez-vous de l'inclure au stade de développement initial. Personnellement, je pense qu'il est préférable de le serrer et de se détendre au besoin.

Êtes-vous trop lié à la vérification de code statique?

Je viens d'écrire que la vérification de code statique devrait être utilisée avant, et c'est un thème contradictoire immédiatement, mais s'il est difficile de faire des corrections pour corriger l'indication de vérification de code statique, la vérification de code statique est assouplie ou exception Il vaut mieux définir. Une méthode courante est les méthodes soulignées. Lorsque la méthode dépasse un certain nombre de lignes, on dit la diviser, mais s'il convient que le traitement soit une unité, ce ne sera que difficile à comprendre, donc je pense qu'il n'est pas nécessaire de le forcer.

Le code d'état HTTP est-il approprié?

Avec Rails, cela fonctionne souvent sans problème même si le code d'état n'est pas cohérent avec la méthode ou la réponse, j'essaie donc de vérifier si le code d'état est utilisé correctement. Je pense que l'utilisation détaillée est différente selon la personne, mais j'écrirai mon interprétation du code de statut principal.

2XX Il s'agit du code d'état lorsque la demande est traitée normalement.

201 Created Il est utilisé lorsqu'une ressource est générée par une requête POST ou PUT. Lors de l'utilisation de 201, la ressource générée est souvent renvoyée dans le corps de la réponse. Si vous n'avez rien à retourner, je pense que vous devriez utiliser 204 No Content.

204 No Content Il est souvent utilisé lorsqu'une ressource est supprimée par une requête DELETE, mais il est également utilisé lorsqu'il n'y a pas de corps de réponse, même dans POST ou PUT. Si vous l'utilisez, ne mettez pas de valeur dans le corps de la réponse.

200 OK Si vous souhaitez renvoyer une réponse normale autre que celle ci-dessus, vous devez essentiellement utiliser 200 OK.

Série 3XX

Il s'agit du code d'état lorsque vous souhaitez rediriger vers une autre page.

301 Moved Permanently Utilisez 301 si vous souhaitez rediriger définitivement vers une autre page. Par exemple, lors de la migration d'une page Web, il est utilisé pour rediriger de l'URL de l'ancienne page vers la nouvelle page. Il est également utile comme mesure de référencement. Si vous spécifiez 301, le moteur de recherche indexera la nouvelle URL pour vous.

303 See Other Je n'en étais pas au courant jusqu'à récemment, mais si vous souhaitez rediriger vers la page de détails de la ressource après avoir enregistré / mis à jour la ressource, utilisez 303. Le modèle est le suivant. Enregistrez de nouvelles ressources avec POST / resources et redirigez vers la page (GET / resources /: id) qui affiche les informations sur les ressources

302 Found Contrairement à 301, utilisez 302 si vous souhaitez rediriger temporairement vers une autre page. Si vous utilisez redirect_to dans Rails, ce sera 302 par défaut, mais compte tenu du but de la redirection, 302 a peu d'utilisations, et je pense qu'il est souvent plus approprié de le distribuer à 301 ou 303.

Série 4XX

Il s'agit du code d'état lorsqu'une erreur induite par le client se produit.

404 Not Found Code d'état utilisé lorsque la page est introuvable. Puisque "404" est souvent affiché à l'écran, je pense que beaucoup de gens le savent.

Cela peut être une idée différente pour certaines personnes, mais j'essaie d'utiliser correctement 404 et 200, en considérant si la ressource manquante est anormale ou normale pour le client. Voici quelques exemples concrets pour faciliter l'imagination.

Si les ressources ne peuvent pas être trouvées avec GET / resources /: id Renvoie 404 car le client spécifie un identifiant qui n'existe pas
S'il y a 0 TODOs dans GET / todos pour obtenir la liste TODO Puisque 0 TODO est un fonctionnement normal, 200 est renvoyé.
Lorsque j'ai spécifié les conditions de restriction dans la recherche de magasin, il est devenu 0 Renvoie 404 qui ne remplit pas les conditions spécifiées par le client Du point de vue du référencement, je ne veux pas que le moteur de recherche indexe 0 pages de résultats de recherche de boutique, donc je ne veux pas indexer en spécifiant 404.

401 Unauthorized À utiliser lorsqu'aucune information d'identification valide n'est transmise pour une action qui nécessite une authentification.

403 Forbidden Il est utilisé lorsque l'authentification est effectuée mais que les droits d'utilisation sont insuffisants. Par exemple, lorsqu'un utilisateur en lecture seule tente de mettre à jour une ressource.

Cependant, il y a une chose à considérer lors de l'utilisation du 403. Lorsque vous accédez à une ressource privée pour un utilisateur. Dans ce cas, si vous y réfléchissez systématiquement, vous voudrez utiliser 403 car vous n'avez pas de droits d'accès, mais si vous renvoyez 403, l'utilisateur sera informé que la ressource existe. Dans un tel cas, il vaut mieux ne pas informer l'existence elle-même, il est donc préférable de retourner 404 avec le sens qu'il n'existe pas.

400 Bad Request Il est utilisé lorsqu'une demande non valide arrive, par exemple lorsque les paramètres requis ne sont pas spécifiés. Si cela ne s'applique pas aux 401, 403, 404, j'utilise généralement 400.

410 Gone Il est utilisé lors de l'accès à l'URL à supprimer définitivement, par exemple après la fin de la période et la page supprimée sur la page de la campagne à durée limitée. C'est dommage, mais dernièrement, il y a beaucoup de pages qui restent sans surveillance même après la fin de la période de campagne, et cela devient un bruit lors de la recherche, ce qui est ennuyeux. Je voudrais que vous ayez une tâche de le supprimer lorsque la date limite est passée et de l'exécuter correctement.

Série 5XX

Il s'agit du code d'état lorsqu'une erreur induite par le serveur se produit.

500 Internal Server Error À utiliser lorsqu'une erreur inattendue se produit sur le serveur. Je ne pense pas qu'il soit courant pour un programme de générer explicitement une erreur 500, donc s'il y a une partie qui renvoie explicitement une erreur 500, je pense qu'il est préférable de la modifier pour qu'une erreur appropriée soit renvoyée.

503 Service Unavailable Il est utilisé lorsque la page de maintenance est affichée car elle n'est pas normalement accessible en raison d'une maintenance régulière.

Le message d'erreur adressé au client est-il approprié?

Vous souhaiterez peut-être renvoyer un message d'erreur au client lorsqu'une erreur se produit, mais assurez-vous que le message est correct. Le point de vue est de savoir si vous pouvez recevoir le message d'erreur et prendre des mesures. Un mauvais exemple courant est un modèle qui renvoie un message qui ne spécifie pas un élément tel que "élément d'entrée non valide" lorsqu'une condition d'entrée n'est pas remplie sur un écran avec plusieurs éléments d'entrée. Avec cela, vous ne savez pas quel élément est erroné et vous ne savez pas quoi faire. Disons l'élément et la cause de l'erreur comme "Entrez la valeur de l'élément xx comme une valeur numérique".

Cependant, il y a des cas où il est NG de dire à l'utilisateur en détail. Par exemple, si vous spécifiez un ID existant sur l'écran de connexion où vous entrez l'ID / mot de passe et que vous renvoyez «mauvais mot de passe» lorsque le mot de passe est incorrect, l'utilisateur sera informé que l'ID existe. .. Avec cette implémentation, un utilisateur malveillant peut entrer un ID approprié pour créer une liste d'ID existants. Même si l'ID n'existe pas ou que seul le mot de passe est différent, il est bon de renvoyer un message tel que «Informations de connexion non valides» qui ne peut pas identifier ce qui ne va pas.

la réponse api renvoie une valeur significative

Je pense que les applications Web modernes ont souvent des frontaux et des back-ends séparés. En conséquence, le backend fournit souvent des fonctionnalités en tant qu'API. Lors de la création d'une API, lors du retour d'un élément énuméré, il doit être retourné afin que la signification puisse être comprise simplement en regardant la valeur. Par exemple, si vous avez le statut suivant:

enum status {
  todo: 1,
  in_progress: 2,
  done: 3
}

Lorsque vous le retournez à la réception, vous devez renvoyer le nom (comme «faire») au lieu de la valeur numérique (1, 2, 3). Si vous renvoyez une valeur numérique telle que «status = 1», que vaut 1 lorsque vous voyez la réponse? Cependant, si vous renvoyez le nom comme status = 'todo', vous pouvez comprendre la signification simplement en le regardant.

L'ordre du tableau est-il unique?

Lors du retour d'un tableau, vérifiez si l'ordre du tableau est unique.

Dans ce qui suit, le tableau des critiques réduit par user_id est retourné, mais comme l'ordre n'est pas spécifié, il n'est pas certain dans quel ordre ils seront retournés. C'est bien s'il est certain que l'appelant ne se soucie pas de l'ordre, mais spécifions l'ordre pour que l'ordre soit corrigé.

def my_reviews(user_id)
  Review.where(user_id: user_id)
end

Ci-dessous, l'ordre décroissant de created_at est spécifié. Si tel est le cas, il semble que vous puissiez mettre en ordre les nouvellement créés.

def my_reviews(user_id)
  Review.where(user_id: user_id).order(created_at: :desc)
end

Cependant, cela pose encore des problèmes. Si created_at a le même avis, la commande n'est pas fixe. Afin de déterminer sûrement l'ordre, il est bon de déterminer si les colonnes spécifiées dans l'ordre sont uniques, comme indiqué ci-dessous. Cette fois, j'ai spécifié l'ordre décroissant de l'identifiant pour le deuxième tri pour le rendre unique.

def my_reviews(user_id)
  # 1. created_Disposer dans l'ordre décroissant de at. Cependant, créé_à n'a pas de restrictions uniques, donc ça peut être le même jour
  # 2.Si c'est le même jour, classez-les par ordre décroissant d'id. L'identifiant est unique, il ne peut donc pas avoir la même valeur
  # =>La commande est déterminée de manière unique
  Review.where(user_id: user_id).order(created_at: :desc, id: :desc)
end

Ce que les autres peuvent lire et comprendre en douceur

Lors des révisions de code, vous pouvez voir des implémentations qui prennent beaucoup de temps à comprendre ou qui sont trompeuses. Une telle implémentation devrait être aussi simple que possible, car tous ceux qui verront ce code à l'avenir ressentiront la même chose et augmenteront l'effort de lecture du code et augmenteront la probabilité d'incorporer des bogues. Si vous ne pouvez pas faire simple, vous pouvez ajouter un commentaire. Si vous utilisez un framework tel que Rails, vous pouvez mal comprendre que vous l'utilisez en dehors du framework comme indiqué ci-dessous. Débarrassons-nous du code trompeur avec une forte probabilité de ne pas créer de bogues plus tard.

class Review
  belongs_to :user
  enum status {
    open: 0,
    close: 1,
  }
end

class User
  #× Même s'il s'agit de l'association par défaut, elle est réduite en spécifiant les conditions
  has_many :reviews, -> { open }

  #○ Si vous souhaitez définir une condition, utilisez un nom qui la comprend.
  has_many :open_reviews, -> { open }, class_name: 'Review'
end

class Hoge
  def fuga(user_id)
    user = User.find user_id
    #Si vous ne regardez qu'ici, il y a de fortes chances que vous pensiez à tort que vous avez acquis tous les avis des utilisateurs.
    user.reviews
  end
end

YAGNI Comme beaucoup d'entre vous le savent peut-être, il y a le mot YAGNI. Voir wikipedia pour plus de détails. https://ja.wikipedia.org/wiki/YAGNI

Comme mentionné ci-dessus, il y a peu de chances que vous puissiez imaginer et implémenter une fonctionnalité que vous n'avez pas encore utilisée et l'utiliser telle quelle. Soyez prudent si vous voyez le code que vous écrivez en imaginant le futur. Il est douloureux de supprimer le code implémenté, mais il est souvent préférable de le supprimer. Si vous le laissez, cela peut vous gêner plus tard, ou vous devrez peut-être le réparer même si vous ne l'avez pas utilisé au moment du refactoring, ce qui peut réduire votre productivité.

Réduisez la portée des méthodes et des constantes

Créez des méthodes et des constantes qui ne sont pas appelées de l'extérieur comme privées. S'il est public, la plage d'influence lors de la modification de la méthode ou de la constante sera la source entière, mais si elle est privée, vous n'avez qu'à vous soucier de la même classe, il est donc assez facile d'étudier la plage d'influence de la modification de la source telle que la refactorisation. Devenir.

La responsabilité est-elle appropriée?

Chaque classe a ses propres responsabilités. Vérifiez que vos responsabilités sont respectées.

Par exemple, considérez l'action qui active votre compte.

Nécessite un jeton pour s'activer
Mettre à jour l'état du compte et la date et l'heure d'activation pour supprimer le jeton

Étant donné que le traitement exécuté au moment de l'activation doit être connu du modèle de compte qui a la responsabilité du compte, il est préférable de le mettre en œuvre avec la politique comme ○ ci-dessous.

class AccountController
  #× Le contrôleur sait quoi faire au moment de l'activation
  def activate
    account = Account.find_by(token: params[:token])
    account.status = :activated
    account.activated_at = Time.now
    account.token = nil
    account.save!
  end

  #○ Le contrôleur ne sait pas quoi faire au moment de l'activation et demande au modèle de compte
  def activate
    account = Account.find_by(token: params[:token])
    account.activate!
  end
end

class Account
  def activate!
    status = :activated
    activated_at = Time.now
    token = nil
    save!
  end
end

includes, eager_load, preload Si vous n'êtes pas sûr de ces différences, veuillez consulter l'article Qiita ci-dessous, que vous auriez dû voir au moins une fois si vous utilisez Rails. https://qiita.com/k0kubun/items/80c5a5494f53bb88dc58

includes est une existence pratique qui utilise correctement preload et eager_load. Quand j'ai commencé à utiliser Rails, j'ai pensé qu'il serait préférable d'utiliser des includes, mais l'idée actuelle est le contraire. J'essaie d'utiliser la précharge de base ou eager_load. C'est parce que je pense que nous devrions savoir s'il faut adhérer ou non lors de sa mise en œuvre. Les inclusions qui peuvent ou non se joindre en fonction de l'implémentation sont difficiles à contrôler et peuvent provoquer un comportement inattendu, il est donc préférable de ne pas les utiliser.

Y a-t-il une jointure inutile?

ActiveRecord est très utile, mais on a l'impression d'être loin de l'ingénieur Rails. Si vous écrivez vous-même du SQL, vous serez prêt à émettre du SQL que vous n'écririez pas. Parmi eux, celui que vous voyez en particulier est le schéma de l'adhésion en vain. Un exemple concret est présenté ci-dessous. Une méthode find_by_organization qui spécifie le organizatoin_id pour obtenir les utilisateurs appartenant à cette organisation.

class User
  has_many :user_organization

  def self.find_by_organization(organization_id)
    #× Les tables d'organisation n'ont pas besoin d'être jointes
    # select * from users inner join user_organizations on users.id = user_organizations.user_id inner join organizations on user_organizations.organization_id = organizations.id where organizations.id = #{organization_id}
    joins(user_organization: :organization).merge(Organization.where(id: organization_id))

    # ○ user_Seules les organisations sont combinées
    # select * from users inner join user_organizations on users.id = user_organizations.user_id where user_organizations.organization_id = #{organization_id}
    joins(:user_organization).merge(UserOrganization.where(organization_id: organization_id))
  end
end

class Organization
  has_many :user_organization
end

class UserOrganization
  belongs_to :user
  belongs_to :organization
end

[RUBY] Le point de vue lors de la révision du code d'une application web (Rails)