This repository has been archived by the owner on Jan 13, 2025. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable the robot to interface with NetworkTables and utilize the transmitted values #37
base: master
Are you sure you want to change the base?
Enable the robot to interface with NetworkTables and utilize the transmitted values #37
Changes from 8 commits
c5dafdd
d035411
3ed97cd
c34062d
5122464
8ed3a00
6aea015
dd95166
b5d60e4
cb5375a
73620a5
4cb9d6c
6a5164e
069de95
0f45bdf
f47c472
dda2b43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something to consider (what's more typical, easier to test, less error prone): instead of having this as a "utility class" (class with only static members and methods), make the members and methods non-static, and create an instance of this in GetAprilTagPoseData.
In a unit test, you could have a "fake" implementation of this that provided data from memory etc.
If this isn't making sense, happy to talk thorugh this on chat or f2f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we want to have an instance of this class? The design of this class is to be a utility class for the first layer of getting data from NetworkTables to the robot. IMO I don't think it would be good to make it non-static if we are going to want different parts of code using it at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you typically do not put this kind of state in utility classes; utility classes are typically stateless:
https://medium.com/@salmankhan_27014/best-practices-for-creating-utility-classes-in-software-development-6f9ad1e8d27a
Having all of this state in static members can make the implementation and the usage harder to read and manage, and it makes the code harder to test.
for the use case you're describing, you can use the "Singleton" design pattern:
https://softwareengineering.stackexchange.com/questions/235527/when-to-use-a-singleton-and-when-to-use-a-static-class
https://www.digitalocean.com/community/tutorials/java-singleton-design-pattern-best-practices-examples
In a Singleton, you have all of the relevant state in an instance, and you only allow creation of a single instance (which you could replace through eg package visible methods) for usage throughout your code.
Does that make sense? Happy to explain/chat more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is non-blocking ofc :) - more just feedback on what typical industry practices are for coding something like this - and how you can make this code easier to maintain and test.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I'll take a look tonight at the meeting! Thanks!