Conversation
- Usage of primitives instead of wrappers - Usage of proper builder for ControlsOuput
tarehart
left a comment
There was a problem hiding this comment.
Looks good overall, thanks for the improvements!
| return Math.max(-1, Math.min(1, value)); | ||
| public ControlsOutput() { } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
I'm concerned about making the ControlsOutput object immutable. In my own bot logic, there are situations where I like to construct output in multiple stages, e.g. one function to determine pitch yaw roll, and another to decide whether to boost. My goal was to make that type of thing ergonomic.
Can you explain more about why you want to go immutable?
There was a problem hiding this comment.
The main reason why I wanted this particular object to be immutable is that mutable objects, and especially data classes can easily lead to bugs. It is not rare that a single object is handled by several others, and once modified by one, it affects the others which may not want the object to be modified for them.
Also, based on this example, I thought it was the usual practice to create a new ControlsOutput every frame, thus I did not really see the issue you mentioned.
Concerning the example you have given, I reckon you could simply pass the builder to your functions instead of an already-created ControlsOutput? When your function has computed what you want, you just call the corresponding withXXX function of the builder. And when you have called all the functions, you eventually build the object.
Basically, if the object gets immutable, I think it will avoid possible bugs such as those I mentioned earlier. On the other hand, it leads to constant instantiation per game tick, which might cause a performance issue. It really is up to you to decide which of the two you prefer.
If you want to keep ControlsOuput a mutable class, then I'll commit a change to remove the builder.
There was a problem hiding this comment.
I do believe there is value in keeping it simple. Inexperienced programmers might not fully understand the nuances of immutable classes and factories methods.
Experienced programmers can still choose to modify the example bot to fit their needs, although I am personally not very concerned about the performance hit.
There was a problem hiding this comment.
Yeah, I agree with kipje on this one, I'm wary of imposing a full builder pattern on novices.
In other situations, e.g. with the vector classes, I'm completely in favor of immutability. In this specific case, people's bot code almost never reads the attributes of ControlsOutput. 99% of the time people will just write and return, never getting into those buggy situations, so I don't think it's worth it.
|
Accidentally resolved the conversation, I'm responding here. To me it is precisely for rookie developers that the class should be immutable, since they may not fully understand that it they modify the object at a place, it will be modified for all places that use this object |
|
The last commit removed the builder, keeping ControlsOuput mutable. I also split the main method in two, separating the bot logic with the window |
|
Looks good, thank you! |
This pull request brings a couple of changes in the example. They are not related to performance, but they rather improve the readability and the cleanliness of the code.
In a nutshell, these changes are:
@Overrideannotations, which lead to better readability. It makes clearer the fact that the given method initially belongs to an upper classLists instead ofArrayListstoString()methods forVector2andVector3, to ease the debuggingintinstead ofInteger), to avoid double object creation