kchapple
commented
4 years ago Hi Fred, I added additional docs, and also removed that constructor parameter, because it was unnecessary. You're correct it is built into framework and doesn't create additional dependencies, and now that is reflected in docs, and will try to keep that mindset going forward. Makes sense. Thanks!
The __construct in the DURC model can be passed in a validator... why? What is the thinking there? Are you accounting for custom validators? How does this fit into the DURC generated model idea? Why not always insist that DURC handle the validator generation? If I am going to use my validator, how do I do that? Do I create a separate file? Modify the user-editable DURC object? How would I pass this in and from where?
Please do not use the ternary operator in this codebase... this function is actually a good example of why that style of coding is problematic. Using a full if-statement, with curly brackets, gives you the opportunity to add code comments that detail why there is an if statement in the first place. The user needs to understand what it means for this if statement to be true and for it to be false, not in terms of 'execution' but in terms of 'implication'.
This is also the place where I would like you to document that to are deciding to use the validator functions from Illuminate. That should happen when you declare the validators variable. It is not enough to say that it is an Illuminate Validator, it must say why it is an illuminate validator. Something like "We chose to use the Illuminate Validator because it is fully featured and does not create an additional dependency beyond Laravel" which is my assumption.. but may not be correct.. that was your choice, so tell me there what your thinking is.
I think I understand what you are doing, but unlike most parts of DURC, this code is actually going to be a part of other people's applications. This is the code that creates a pretty major dependency for all DURC objects and marries DURC even more deeply into the Laravel framework, all of which is fine, but this means that this is one of the few DURC files that is likely to be read very carefully in the future, by people who are chasing complex bugs...
Lets try to improve the documentation in this file across the board... I think I have a "return('bob');" as a comment later in the file... obviously cruft from a very early development stage... lets take a moment and clean that up...