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

Deleting unused code #415

Open
aprospero opened this issue Mar 22, 2018 · 9 comments
Open

Deleting unused code #415

aprospero opened this issue Mar 22, 2018 · 9 comments

Comments

@aprospero
Copy link
Contributor

Why

The less complex a codebase the more people are able to contribute and bugs can easier be located or even prevented. Code complexity can have many reasons. Big Pro: unused Code is a reason that is most easy to handle. :)

Exceptions

There are exceptions - as always. If for example a piece of code was heavily optimized and the original purpose isn't obvious anymore it is sometimes of use when the original code is left as comment.
There are certainly a bunch of other good reasons for leaving unused code in the code base. Feel free to comment and add yours!

No Exception

IMHO the argument 'but maybe we'll need it later!' is none. We can easily get code back with a cherry pick. I myself sometimes commit Code that is unnecessary at that time and delete it right in the next commit just to have it in the repository.
This way we could have both, a clean code base and code snippets that are possibly needed later on.

@BarbourSmith
Copy link
Member

I fully fully support this idea! The Ground Control code base could also use a combing

@aprospero
Copy link
Contributor Author

Hmmm,

I'm working on this right now and I think about extending the issue to a more general clean up. I would propose the following

issue / pull request schedule

  1. Code formatting - simply unify indentation, brace setting, comment style, etc. A clean coding style improves readability. As we're talking about it: maybe we should define some coding guidelines? Have you already thought/talked about that?
  2. Convert Float to Double - on Arduino they are the same and using both is only good for confusion. If we maybe change the platform once we'll hopefully don't need to optimize computing speed vs. precision anymore so we'd still like to have double.
  3. Clean up and overhaul one class/codefile after the other. Deleting unused or duplicate code to simplify program flow.

The last one is in fact the original issue, but the former ones are equally important and would clutter the intended pull request really heavily (indentation will literally change every line of most code files).

People who are working with forked maslow versions will have a hard time to keep their branches up to date. But I think in order to have long term maintainable code it's worth the hassle. Let's just try to make it not too hard for them.

What do you all think?

@BarbourSmith
Copy link
Member

I am fully in support of this.

I agree that we need to try to do it respectfully so that we don't step on anyone's toes who has a branch out right now. I propose that we give everyone a couple days of heads up before doing the spring cleaning so that we have a chance to merge any outstanding branches.

Just let me know when you are ready and I'll ask around about any outstanding branches.

@blurfl
Copy link
Collaborator

blurfl commented Mar 26, 2018

I have a few branches going. While tidy is good, there is a re-learning curve every time things get moved around.

@aprospero
Copy link
Contributor Author

Yeah, I agree.
This is why we should do it once and entirely. And that's why we IMHO should think about coding guidelines. They usually minimize the need to do maintenance.
Don't get me wrong, I don't want to install any nitpicking tyranny. Guidelines, not laws hammered in stone.
This would need a bit of collaboration upfront to set these guidelines, though...

What do you think?

@blurfl: how do you attach labels to an issue? I haven't found this feature, yet...

@BarbourSmith
Copy link
Member

Adding a label should be over on the right hand side:

image

@blurfl
Copy link
Collaborator

blurfl commented Mar 26, 2018

how do you attach labels to an issue? I haven't found this feature, yet...

Assigning labels needs write permission, to which Bar holds the keys. Is there a label you want assigned?

@BarbourSmith
Copy link
Member

Oh, darn never mind 😬

I forget what I can see that is hidden normally

@aprospero
Copy link
Contributor Author

No, I don't need write permissions - was just curious. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants