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

Updated #1

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

Updated #1

wants to merge 1 commit into from

Conversation

anishraja100
Copy link
Contributor

No description provided.

@aphan42
Copy link
Member

aphan42 commented Feb 28, 2017

Hi Anish,

I promised I'd scrutinize your code; and I did. For the most part the design of your code is great and we know we have proof that it is currently working as of last night.

I have a couple remarks about the design of your code

(1) For most of your systems classes, you have all member variables declared public. This is in general not best practice. I will give a concrete example.

In Deadbands.h you declare the low and high value as public, meaning that any other file which includes it can access or modify these values directly. This can lead to unexpected behavior.

Also another example: why should I be able to access an object of CANTalon that controls the drive system from e.g. the code that controls gear pickup? Just some thinking points.

I suggest perhaps going through the hassle of writing getters and setters to protect the abstraction of the class that you have created.

(2) Also you claimed that virtual destructors are everywhere, but they are generally not used unless you have someplace in which you want some sort of polymorphic behavior. For the most part that doesn't exist except for the IterativeRobot derivation in the main robot controls. Perhaps regular destructors are the way to go here.

Happy coding. I unfortunately do not have the ability to approve your merge request; you'll have to ask the God ChinnyK himself.

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.

2 participants