Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Focus recommendations around injection #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adamnfish
Copy link
Contributor

Remove irrelevant detail to focus on the guidance. (We get it)

Remove irrelevant detail to focus on the guidance.
@adamnfish adamnfish requested a review from mchv November 17, 2017 17:59
@mchv
Copy link
Member

mchv commented Nov 18, 2017

I think the details are useful just because I have got asked the reasons why it is bad many times. I understand that for a more senior developer, this may sounds unnecessary and even considered noise polluting the main objective of the recommendations.

@markjamesbutler
Copy link
Member

@mchv @adamnfish did we reach a conclusion on this? Personally, I think the detail could be useful for devs. Although it could be moved to a separate location.

@adamnfish
Copy link
Contributor Author

Forbidding something "in general" is inappropriate in this context and leads to people having (and acting on) strong feelings one way or the other instead of having a discussion that's focused on effectively solving problems. My PR tries to remove this dogmatic view in favour of a guideline and a piece of concrete advice because the existing explanation is unfocused, verbose, and doesn't closely relate to the guidance it exists to support.

The paragraphs it replaces are about computer science in general, containing links to concepts that seem tangentially related to the issue at hand. If we want to add a reading list to this repository then I think these would make good additions, but they feel out of place to me here, which adds to the feeling that this is unfocused and verbose.

For example, repeating the Wikipedia definition of dependence injection seems redundant to me. Leave a link if we must, but the target audience is engineers so even if they haven't heard of it they know how to use Google (or ask peers).

Why explain this point without explaining any of the others? What is it about Dependency Injection that warrants a multi-paragraph explanation compared to immutability or guidance that we should update Scala before adding features?

Looking at both of the above, we see that here there has been an inclination to explain from first principles as well as the decision to single this point out for detailed explanation. In both these cases, I think that the difference is not one of computer science, but emerges from this being written as banning dependency injection in general.

The only piece of concrete guidance in the existing paragraphs is to use Play's Compile Time Dependency Injection, but that is still dependency injection! The explanation doesn't support the guidance and reads like an opinion on an aspect of computer science rather than a contribution to the list of recommendations for engineers in this department. I agree with the advice, but I think that it appearing here detracts from the document.

In the review of this PR we are talking about two separate points. The first is that this forbids dependency injection in general. The second is a question of what level of supporting detail is appropriate for the points in this list. My feeling is that fixing the first naturally fixes the second. As I've argued above, I think the need for supporting detail is a symptom of this point being poorly worded.

However, if we agree that the reasoning behind these guidelines is important then let's justify which points need explaining and come up with focused explanations of why the guidance exists. This may include examples of projects that had problems when the guidance wasn't followed, or contrasts between projects that have taken different approaches. If we can give a clear explanation of why the guidance makes sense then that would be perfect, but we aren't trying to teach Computer Science (or even Software Engineering) in this document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants