Skip to content

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.

3 participants