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

Member variable/method access control #6

Open
vmagro opened this issue Dec 29, 2014 · 1 comment
Open

Member variable/method access control #6

vmagro opened this issue Dec 29, 2014 · 1 comment

Comments

@vmagro
Copy link
Member

vmagro commented Dec 29, 2014

As a general rule of thumb, everything other than static variables should always be declared as private. If you need to access or set the value of a class's member variable, you should have getter and setter methods to access/set the value respectively.

Here are some spots where I noticed you either didn't declare an access modifier or used it incorrectly:

  • the drive mode booleans
    • should be private and have getters/setters if necessary
  • the constructor of Drive
    • The constructor of Drive should be declared as private, not public
    • This might not make immediate sense but the reason for making it private is to enforce that no one else can create a Drive instance except code in the Drive class. Since a robot can only have one drivetrain, you should employ the Singleton pattern.
    • For an example of a Singleton, see this part of the Drive code from 2014
  • gyroEnabled
    • should be private and have getter/setter if necessary
  • driveHalo and driveTank
    • Since these methods are public, anyone can call them to control the drivetrain, completely bypassing the current mode selection. This is allowing other code to potentially circumvent your selected driving mode, which could cause hard to debug issues.
    • They should both be private and only called from the public drive() method that will then call the appropriate choice based on the current driving mode.
  • setDriveshifter
    • this should obviously be private for the same reason as directly above
    • you even say not to call it in the comment right above the method declaration
      • you shouldn't just "ask nicely" to not call it, you should not allow anyone to call it at all by declaring it private
  • straightPidEnabled
    • should be private and have getter/setter if necessary
@vmagro vmagro reopened this Dec 29, 2014
@vmagro
Copy link
Member Author

vmagro commented Dec 30, 2014

One more thing: the skim method in Drive should also be private, right now it's the default, which means it will be able to be called from any other class in that package.

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

No branches or pull requests

2 participants