12

I really like this article about leaving the code/camp site in a nicer state than you found it - it seems like a practical approach in the real world to keep on top of code cleanliness.

I also really like feature branches as a way of developing features in isolation so that, if you don't like it, you can easily not merge it etc.

However, if I'm working on a feature branch, and I spot some ugly code, should I fix it?

It feels like there are a number of down sides to fixing it:

  • When I merge the branch back in, the diff will be messy, cluttered with variable renames or function extraction
  • If the feature gets abandoned, you either have to cherry pick the cleanup commit (which may or may not work depending on how the code near it has changed making a messy merge), re-do it or just abandon it.

On the flip side, if I don't do it whilst I am in the file, then clearly I'll forget to do it in a couple of days time when I merge the branch in.

I was warned this was opinion based (I think just off the fact the title includes should), but I feel like there is an answer (certainly people use both these approaches so they must have an answer). Also, questions about development methodologies are on topic and I think they necessitate a degree of opinion.

Community
  • 1
T. Kiley
  • 255
  • 1
  • 5
  • 1
    Possible duplicate of [How to refactor when all your development is on branches?](http://programmers.stackexchange.com/questions/214658/how-to-refactor-when-all-your-development-is-on-branches) – gnat May 03 '16 at 13:49
  • @gnat A useful read, thanks. I don't think it is quite a dupe since that is about branches that have a long turn around time. I'm asking specifically about how to reconcile the good camper approach to refactoring with feature branch dev. – T. Kiley May 03 '16 at 13:53
  • It depends on the stage of development the project is in. If the project has undergone a fair amount of testing and is fielded then I think it is very risky to change anything that isn't part of what was intended to be fixed. Many people have inserted bugs changing things that shouldn't have affected anything. If the project is in the development stage then the cleaner the code is to start the better, so then I'd probably cleanup the entire file if need be. – Dunk May 03 '16 at 22:02

4 Answers4

9

You should only 'fix' code in a feature branch if you are changing that piece of code anyway as part of the feature.

Eg. I am working on the 'print rabbits' feature and I find the printer code

Public class Printer(string type)
{
    If(type=="bunnies")
    {
        //print a bunny
    }
.....
}

I change it to:

Public class Printer(string type)
{
     PrintFunctionDictionary[type].Print();
}

Why:

  • I'm working on the code,
  • I need to change it to add functionality,
  • the added functionality suggests a refactored way of tackling the problem.

I dont randomly hit some other part of the code base and 'make it better' as this would:

  • Clash with people working on other features.
  • Use time which should be allocated to developing the feature.
  • Add arbitrary code to a branch which, if the feature isnt finished, may not be merged into the main product. Similarly, if I roll back the feature, I will lose my refactoring.
Ewan
  • 70,664
  • 5
  • 76
  • 161
  • 1
    I agree that this is a good thing to try, and in an ideal world this would always work. However, in real world code, the situations are often more complex - when working on a feature, one can typically finds parts of the code which might be worth to be refactored independently from the feature. That code might interfere with the implementation of the feature, but not be restricted to the methods or classes in direct relation to the feature. And a change does not necessarily disturb others. – Doc Brown May 03 '16 at 14:31
  • 1
    well you can always do a seperate refactoring branch. As I see it though feature branches are mainly a project management thing that lets you go "feature X wasnt finished but we can release with all the others" and "feature X is released" so we dont expect feature Y to change. By adding refactoring to a feature you potentialy break these benefits – Ewan May 03 '16 at 14:46
5

If you want your refactorings to "live indepently" from your current feature branch, do not make the changes there. Instead, do the refactoring on the main development branch (or a "refactoring branch", if it is common in your team not to apply changes directly to the dev branch). So anyone of your team (including you) can merge the changes into the active feature branches they are working on. However, be careful not to apply any global refactorings throughout "half of the code base" without asking your colleagues for permission first - they might not be so happy if your refactorings interfer with their current work too much.

The exception is here when the improvements you make are local to the parts of the code base you touch exactly in that feature branch, and it makes no sense to give them a different life cycle than your "new feature".

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
3

The purpose of branch types are to provide intention for handling them. If you're following a GitFlow style of branching, then you likely have types like feature, hotfix, release, etc.. In the case of a feature branch, its intention is to encapsulate a merge into another branch (i.e. develop) that shows the developer responsible for merging, what this feature IS. If the code you are cleaning up is not part of that feature, don't change it.

Instead, find the lowest possible branch the ugly code is in (likely develop) and branch from there. Change the code and propose it be merged in as a feature. If you need that code in what you're working on, and especially want to avoid merge conflicts, merge that branch into YOUR branch.

Here is a pretty good explanation of different strategies: https://www.atlassian.com/git/tutorials/comparing-workflows/

TomSchober
  • 219
  • 1
  • 7
0

if I'm working on a feature branch, and I spot some ugly code, should I fix it?

It's probably fine to fix 'ugly code' on sight, depending on the tempo of the project, the 'ugliness' of the code, etc., but try not to do this on the feature branch itself.

  • If your feature branch is entirely local, just stash or commit unsaved changes, check out the development branch, make the change, then return to your feature branch and rebase off develop.
  • If you can't rebase off develop (e.g. your feature branch is on a public server), you can still cherry-pick that commit off develop if you need it or want to avoid conflicts later on.
  • If you are editing a file and really have to commit the fix to the ugly code right now and really can't switch to develop, you can make the fix, use git add -p to make the fix, commit that change only, and before you merge/push (indeed, preferably after your next commit), use interactive rebase to move that commit to the earliest possible point in your branch, or possibly even cherry-pick onto develop, depending on your history.

I would also do this with anything else that is 'fixing' the development branch (where 'fixes' are changes you or any other developer might make on sight to ensure that code adheres to standards). This helps...

  • to keep your fixes and your feature commits in different groups so the log is more readable,
  • to keep development as close as possible to being releasable at any time, and
  • to avoid duplication of work (multiple people fixing the same issue in subtly different ways on different branches).
user52889
  • 1,683
  • 11
  • 13