1

We had a debate on our team about how clean the Master branch should be.

The application is written and maintained by two people, me (a developer) and a GUI/UX designer. The GUI/UX designer does a lot of prototyping (or "sandbox"-type experimentation) to test or fix various layout issues in JS & CSS. This preliminary or tentative work introduces some "dirty" code such as inline CSS, scattered JS, poor formatting, etc. She would like to directly check that into Master as soon as functionally her goals her achieved, and I stop her.

My own checkins into Master are very clean and I always do a Diff to make sure they're final, formatted, and modularized. I clean up the preliminary or tentative changes I had to make to ensure that Master is "official." Should Master be allowed to get dirty?

gene b.
  • 315
  • 1
  • 7
  • 1
    Possible duplicate of [To check-in small chunks and frequently, or to check-in large chunks but infrequently](https://softwareengineering.stackexchange.com/questions/96302/to-check-in-small-chunks-and-frequently-or-to-check-in-large-chunks-but-infrequ) – gnat Jan 23 '18 at 16:35
  • 1
    Possible duplicate of [Is it ever OK to commit non-working code?](https://softwareengineering.stackexchange.com/questions/211608/is-it-ever-ok-to-commit-non-working-code) – 8bittree Jan 23 '18 at 18:26
  • This isn't so much about non-working code as it is about non-cleaned-up code. Similar but different. – gene b. Jan 23 '18 at 18:27
  • Which kind if VCS are you using? It makes a difference if you use a DVCS like Git (which allows easily "private" checkins to a local branch), or SVN, where all branches are typically on a central server, visible for everyone. – Doc Brown Jan 24 '18 at 06:56

2 Answers2

5

This is ultimately a decision a team needs to make based on it's coding standards and release cycle.

If you release fairly infrequently (every couple of sprints) the argument could be made that checking into master to be improved at a later date is reasonable. However assuming a few things I'll draw up a soap box:

If:

  • You work in an environment where Master should always be production ready (and can be released at any time)
  • You have the ability to check in without committing to master (perhaps a local git copy of the master repository) using a pull request mechanism

Then I would suggest that any code which is in the master branch should be production ready. This includes:

  • Clear and readable,
  • Properly unit tested,
  • Meets all other coding standards,
  • and it builds!

Otherwise the need may come to deploy (at the end of a release, sprint, or as an urgent fix) and poor quality code is in your master branch. This will ultimately lead to code which isn't improved and technical debt in the future.

Liath
  • 3,406
  • 1
  • 21
  • 33
  • 1
    We release infrequently, so your Choice (1) applies here. But in my experience, no one ever allows time at the end of a project for technical debt or clean-up. It never happens. You have to report what you did every day and if you say you're working on tech clean-up rather than functional stuff, eyebrows get raised. My experience is this: if you don't clean it up now, no one ever will. – gene b. Jan 23 '18 at 17:31
  • @geneb. I agree entirely - do it right first time! – Liath Jan 24 '18 at 09:15
  • @geneb. however just to echo Ewan's comment, make sure the TEAM agrees what is correct! – Liath Jan 24 '18 at 09:16
  • 3
    " improved at a later date" - the past 30 years tell me this never happens. :( – MetalMikester Jan 24 '18 at 12:33
0

I think we can all agree that checking in non functional code is a bad thing(tm)

However, your definition of 'dirty' is pretty subjective.

I would suggest you switch to gitflow, allow any commits to feature branches. but have a PR/Code review process before the feature branch can be merged into development.

Additionally, Have a think and make sure you are not applying stricter rules to others than you apply to yourself.

Its all too easy to give yourself an out "Oh I need to meet the deadline/That hack is acceptable/It has to be done that way because I googled for ages but couldn't find an answer" When you are reviewing your own code and then pick up others on Pascal vs camel case

Ewan
  • 70,664
  • 5
  • 76
  • 161
  • +1 for the "holding others to a higher standard" it's important the team agree to coding standards and the team maintain them through CRs – Liath Jan 23 '18 at 17:06