Symfony Live London was last Friday which was great, and I’d recommend it to anyone in the Symfony world.
I’ve said for a while that Symfony is the lead in the Clean Code area of PHP, so it wasn’t surprising that many of the talks were about that, rather than Symfony itself. I stuck with track one all the way through, and all of the talks could be considered talks more about clean code than Symfony. Many were about writing code that works easily with Symfony but not for it. Matthias’ talk could have just as easily been called “The Framework Trap”, to go along side Jakub’s.
One of the disappointing aspects of the conference for me was that it felt a lot like receiving lectures at university for some talks. “Here’s, academically, how you fix this problem in a controlled, greenfield environment.” The best talks I saw looked at specific issues, like Konstantin’s. He was telling a story here, rather than teaching. Teaching by storytelling has been the way humans have learned for thousands of years, and there’s not nearly enough of it now.
I’d love to go to a conference dedicated to people telling stories about working on their (possibly legacy) projects and making them better. Some of the talks should be able how they did that and completely failed – losing months of dev time and ultimately ending up with a worse product. Technical discussions on how they realised they were crowbarring a design pattern that didn’t fit (emphasis on how they realised). MyBuilder could come and tell their story. phpBB could tell us how their move to Symfony was. I’d pay for that.
Because our logic isn’t as separated from our database as we’d like, many of our API tests touch the ES database. In order to avoid futsing up our actually data, we just want to make sure it’s only ever talking to indexes starting with “test_”.
My suggestion was that we should refactor this so that we don’t need to be exposing what should be a private method.
The argument was that this is a private method; it’s convention in JS to just use this underscore syntax. But that argument really just comes down to “that’s the way it’s always been done.”
Leaving something public really limits how much you can change later – regardless of it being an “underscore private method” you’re still letting others see it, and you’ve no way to know that it’s safe to delete that method once you’ve changed your implementation.
Even if you know that’s not a concern, it’s still a good habit to get out of. And a free one at that.
 Which had nothing to do with the feature set being added, as always seems to be the case: comment on the low hanging fruit, rather than the structure or flow of the code.
 In this example, the proxy probably won’t be used again and has no need to be extended.
Those that know me know I’m not incredibly into food; despite that I decided to sign up for an Abel & Cole veg box, and try and cook something. This is probably third or forth time I’ve ever tried to cook from a recipe!
In our box recently we received Spring Greens, which are a lot like chard, apparently, and Tim wanted some cachew nuts so we went with this recipe.
Another alteration we made was (because we couldn’t find any in Morrisons) pepper instead of chilli. Quite different, but worked really well. It was another ingrediant from our veg box, so I was pretty chuffed to be using them up.
First task was for me to start slicing up some onions. I’ve no idea what sliced means so I just cut them up a bit – horizontally and vertically. That’s probably fine.
Meanwhile, Tim was busy toasting the rice. We figured that meant “burn it a little”. We were very curious about this, as burning food was often the opposite of what the recipe tells us to do. We browned it off quite well, I think. It’s resistance to burning might have something do with the olive oil we generously gulped in.
Next we added two cups of water to it, and it smelled really nice. Not like burnt food at all, but something we quite wanted to shove in our faces.
This is a technique Abel and Cole seem to love – “cups” and “gulps” and “handfuls”. Not actual measurements. Put Tim and I right out of our computer science comfort zones, but it all seemed to work out!
We went with the whole pepper, even though it was supposed to be substituting a single chilli. I like pepper, so we figured we’d go wild.
We were to wait until the onions were glossy. I’ve no idea how glossy onions are supposed to get, so we just went with it. I’ve learnt cooking is a lot about “yuh – that’s probably looks done”. How liberating. At that point we threw in some garlic and the peppers. And more olive oil, of course.
We almost forgot about the spring greens and threw it in at the last minute. We were umming and ahhing over if we should be chopping it up or not, but decided that if it’s anything like spinach it’ll shrivel up when it hits the hit. It did a little bit, but we were probably supposed to tear it up as we were putting it in the pan.
By this time the water had boiled off the rice and it seemed to have steamed itself happily.
“Steam the rice” was a confusing command for us. We actually bought a steamer (was £15 at Morrisons, so we thought it was worth the punt) but decided against using it. It looked like the rice was full of steam anyway, since we had the lid on the pan. We almost never use that lid, so it was nice to get that out of the cupboard.
Because we were late throwing in the greens there was a worry that the rice would go cold before everything else was done. Not sure what we could do about that, but it turned out to be fine anyway.
Finishing up just meant frying the cashew nuts, which we messed up a little. We didn’t realise how quickly they would burn, and so ended up a little chard (no pun intended…).
It looked nothing like what was in the recipe book, but we were never expecting it too. It was really delicious though. I wasn’t in high hopes about liking it at all – I was actually mentally preparing a back-up meal for if this was foul. But we ate every bit of it.
As you can see, I took the excuse to break out my camera and catalogue our cooking experience. I was using my Canon 60D with a 50mm lens. Really narrow focal range, which can be tricky to work with, but really good at giving emphasis. Maybe my favourite lens I’ve played with.
I enjoyed not having too much time to fuss around with the shot. We were busy cooking! I had to shot and go.
There’s lots of places that you can find talking about how to write a good commit message. Some will even give good technical reasons for doing so, but only a few will give you actually good reasons. When’s the last time you had to use git log --oneline or send a commit via email? Maybe Linus does this everyday, but you don’t.
The one reason I find most important when you’re thinking about the git history as a whole (not just on a per-commit basis) is during peer review. In my current work place and my last the way we did code reviews was by a collogue pushing to github, and opening a pull request. It was then someone else’s job to review and pull that in. That seems to be a very popular workflow from talking to a number of tech teams the past few months whilst interviewing.
Forget about being easily able to look back in the project’s history and see why something was changed, or being able to see a good message on git bisect. They’ll come incidentally with the following method.
Review is the one stage in product development when you’ll be reading commit messages the most, but it seems like no commit messages I’ve encountered recently, at the very least on my team, are written for that part of the process.
Failing to describe the scope of the message
The first thing I want from a commit message is to know the mindset I should be in whilst reviewing. What are you trying to achieve with the code change you’ve introduced? This Doctrine commit message could be slightly better by saying “always add ‘isEmbeddedClass’ to the serialise array”. I could look at the commit message, and at the code change and see “yup, this is doing exactly what the author wanted it to do.” From that point, I can start thinking of side affects the code change might have on edge cases.
The problem with “fix bug” is that I can’t tell what issue with the code you’re talking about, and so the only review I can do is “yep, that’s syntactically correct”. I can still think about potential side affects, but I’ve no idea if the original writer has thought about and accepts them.
Splitting one unit of work across too many commits
A unit of work should be valuable in some way. A commit should hold one unit of work. If we were to revert this one commit from your pull request, how many other commits would have to be reverted too before we got to a working piece of code? It should be as few as possible without affecting other features.
This usually happens when you’re working in stages and commit each stage. That’s fine at giving you good rollback points, but they should be merged together later on when you’re self-reviewing. An example of this is in the BrowserQuest repo, two commits right after each other with the same commit message: ae773f and 9c1d5.
A more common (and more annoying) issue is when a file has been forgotten to be added, and so the second commit is “lol, forgot to add this file”.
These are exactly what --amend flag to git was added for; instead of adding a new commit, just push the changes into the last commit.
Never mix styling or whitespace changes with functional changes
Most teams with an old codebase have the idea to clean as they go along – that’s a good idea (rather than a huge, up front investment).
These commits should be on their own, because I’m not even going to read them. Your team mates should just be able to auto-merge these commits in (especially in a language like PHP where whitespace doesn’t matter).
The problem is that if you’re changing from tabs to spaces, the git diff will say that every line has changed. Your functional changes will get buried and it won’t be possible to see if anything will have unexpected side effects.
Personal blog of Shane Preece. Occasional tech minddump.