Mettre un lien vers le site et/ou les profils de Sandi ou Katrina ce serait cool. Ah mais c'est à la toute fin de l'article 💡 ! D'ailleurs il y a une typo, c'est Sandi avec un "i".
Plus de dépendances ! J'ai souvenir d'autre type de couplage/dépendance :
- La dépendance temporelle : A doit être exécuté avant B
- C'est de la mauvaise encapsulation
- La dépendance à une API : A connait la signature de B (nom de méthode, nombre d'arguments, ...)
- C'est plus sournois et l'inversion de controle peut être une solution
C'est pas un peu étrange ce modèle au pluriel : app/models/user_expenses.rb
?
Je ne fais plus de Rails mais dans mon souvenir les modèles étaient plutôt au singulier.
Ce serait chouette d'expliquer que la classe CSVExporter
sert à extraire la logique d'export du modèle.
def export_expenses(user_expenses)
# not relevant here
end
Super relevant à mon avis car export_expenses
depends du modèle user_expenses
. C'est important car on voit bien que si on ne peut pas se débarrasser d'une dépendence, on peut toujours en limiter la surface. C'est peut être un sujet pour un autre article ceci dit.
Très bien ce allow_any_instance_of(CSVExporter)
, ça souligne un symptome caractéristique d'un excès de dépendences.
Il faut donc se poser la question de savoir quelles informations sont vraiment nécessaires pour le fonctionnement de la classe UserExpenses.
Tout à fait. Pour moi, UserExpenses#export
est un faux ami. Sous couvert de rendre le code plus simple on introduit une dépendence fictive. Un client peut tout à fait utiliser CSVExporter
directement, ça supprimerait complètement cette responsabilité de la class UserExpenses
et donc la dépendence qui va avec:
user_expenses = UserExpenses.find_by(user_id: params[:user_id])
send_data CSVExporter.new.export_expenses(user_expenses)
Une approche pour diminuer les dépendences c'est aussi de laisser la coordination au code appelant.
Si vraiment le code d'orchestration est trop complexe, on peut toujours considérer que c'est un objet à part entière:
class UserExpensesExport
def initialize(user_id)
@user_id = user_id
end
def export
user_expenses = UserExpenses.find_by(@user_id)
CSVExporter.new.export_expenses(user_expenses)
end
end
Il manque un petit .new
sur cette ligne:
when :json then JSONExporter
Il n'y a plus les csv_cols
dans le dernier exemple. Ça montre que csv_cols
ne colle pas bien avec la notion de JSON.
In fine sur le code de l'exemple, j'aurai tendance à préférer :
class UserExpensesReport
SUPPORTED_FORMATS = {
"csv" => CSVExporter,
"json" => JSONExporter,
}
def initialize(user_id:)
@user_id = user_id
end
def build(format:, fields: nil)
user_expenses = UserExpenses.find_by!(user_id: @user_id)
exporter = SUPPORTED_FORMATS.fetch(format).new(fields: fields)
exporter.export_expenses(user_expenses)
end
end
# in app/controllers/exports_controller.rb
class ExportsController
def create
user = User.find(params.fetch(:user_id))
user_expenses_export = UserExpensesExport.new(user_id: user.id)
user_expenses_export_data = user_expenses_export.build(
format: params.fetch(:format, "csv"),
fields: user.custom_export_fields,
)
send_data user_expenses_export_data
end
end
Il est possible d'utiliser stub_const
dans les tests pour injecter un fake dans SUPPORTED_FORMATS
.
Merci pour tes réponses (que je viens de voir), je n'en attendais pas tant !
Il y a beaucoup de gouts et de couleurs dans tout ça, l'important, c'est de faire la chasse aux dépendances.
Les solutions pour s'en extraire sont tellement variées...
Choisir des exemples c'est 50% du boulot je trouve, c'est très très délicat. Je pense que les imperfections de l'exemple de desservent pas le propos 👍
Je me suis laissé avoir par le
find_by
🙈T'as raison, c'est difficile de rester dans les clous avec le format d'un article.
Ah oui, je n'avais pas fait attention, instinctivement, j'imaginais que ça allait être la même API pour chaque exporter.
Il manque un
self.
dans ce cas du coup 😉 (ou bien unmodule_function
)Mais oui, c'est vrai ! Ca permet d'avoir une dépendance de moins.
Idéalement non, c'est vrai.
J'aurai tendance à définir une interface pour les exporter (CSV, JSON, etc) et considérer cette abstraction en tant que dépendance comme tu le suggère. Dans les cas ou cette abstraction change, je suis OK à opérer un petit changement sur toutes les implémentations. D'où
l'astucele hack d'avoir des constructeurs avec des arguments libres (càdinitialize(useful_stuff:, **_)
).Un composant doit bien s'en charger à un moment, que ce soit dans un controlleur ou non.
Dans mon exemple,
UserExpensesReport
avait le role d'unifier les exports sous différents format, du coup j'aurais dit que oui 🤔