-
Notifications
You must be signed in to change notification settings - Fork 81
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
Use builder pattern #39
Comments
Thank you for the feedback! @jpetso |
I do agree that the initializers do need to be more self documenting, but is the builder design pattern common in C++? I have mostly seen it used with Java. |
Qt uses it in a number of places. The GoF Design Patterns book and Wikipedia both include if, and provide a C++ implementation. It was definitely meant for C++ as well, although the Java community took the whole design patterns thing to a bit closer to heart. It might not be the most common one, but it's fairly easy to grasp and not overly hard to implement. It's a solution to a well-defined problem, and as far as I'm aware there's not much competition to that in C++ land. The most common alternative, I guess, would be to make everything a setter of an already-initialized object, rather than building a valid object before initialization. This has other trade-offs (i.e. necessity to check object validity before performing an action) and is best suited for a slightly different use case. I would also say that because it's easy enough to come up with almost-good-enough alternatives, the builder pattern hasn't seen as much popularity as it deserves. In my opinion, that's not a reason to avoid using it :) |
@jpetso. Yes, but if we went with the "making everything a setter design scheme," I believe that all current models would be valid with preset constants, which removes the need to check object validity. |
If that's not a problem then regular setters might work just fine. The main thing that a builder does for you is force the user to supply all the necessary values so they can be passed to the (private) constructor at once. |
I like the builder pattern because, especially for me, being new to machine learning, it makes it easier to understand the class and its members. |
While initially I found the builder pattern (or a similiar design scheme) verbose and java-y, it has grown on me and I recognize its practicality. I especially would agree with @FlyingGraysons that making the switch would make the library more accessible to beginners. Thank you @jpetso for the suggestion. |
@joshuagruenstein @FlyingGraysons. A getter and setter design scheme would look like this: net::NeuralNet neuralNetwork;
neuralNetwork.setNumInputs(1);
neuralNetwork.setNumOutputs(1);
neuralNetwork.setNumHiddenLayers(2);
neuralNetwork.setNumNeuronsPerHiddenLayer(4);
neuralNetwork.setActivationFunction("sigmoid"); |
Or we could have each setter return the object, allowing for one liners. |
I actually think the builder pattern is clearer and less verbose than a setter system, but that's just me. I also dislike how you could create a neural network without specifying the number of inputs and outputs, that just seems wrong to me. |
Trying to read your example code without comments or the documentation handy in a browser window is a pretty puzzling experience. Lots of numeric values without a hint as to what that value might be. Many developers who do not comment their code all that well will leave an unreadable mess when using Fido.
For instance, give this to someone who might have heard about the basics of machine learning but can't remember all the details (let alone the order of parameters in the Fido API):
Nobody knows what this means without looking up the docs. Now imagine if the code looked like this:
Not only does the coder get autocompletion and can initialize the object without double-checking the docs, but the reader now also has a clue what's going on. It's not even much longer than the current version with comment, but now everyone will write readable code and not just those that take the time to write a helpful comment.
This is called the builder pattern, Wikipedia has an article on it too. I think it would make a great match for many of the classes in your API.
The text was updated successfully, but these errors were encountered: