git: The good and the bad

I've now spent a few days working on a new project that has all the source control in git. Knowing that I was going to be assigned to this project at some point, I spent parts of the last few months learning git. Now I actually have to put the knowledge to use. While I have read/write permissions to the main repository, I am choosing to submit pull requests and let another developer accept my changes until I'm more comfortable with git.

I'm getting used to certain aspects of git, have done some branches, committed changes, and submitted pull requests. For the most part everything has been going fine. I am starting to like the concept of creating branches for various changes and being able to park them and work on something else, switch back to what I was working on and then merge everything back in. However, the merging is one part that is kind of confusing to me, still. Apparently there is an auto merge, but I'm not exactly sure how to fix things if that fails.

I committed a change to my local repository today and realized I committed it while working on the wrong branch. So, I was easily able to revert the entire commit and then set a new branch off the commit and then I can work off that. I found that to be pretty cool. I haven't merged my new branch back in, but that will be the next test. Also, I like how git handles renaming of files; it is smart enough to handle the moves and the git log command with --follow will show the entire history of the file; I had to use that in order to figure out who to blame on some code that is never used, but was never removed.

After I made a bunch of moves, I committed everything and issued my pull request. The developer that handles the pull requests said that I left out 11 files. Strange, the files were on my local file system, my repository was just pulled from the upstream repository and git didn't say that any files were untracked. I nuked the files locally, re-did the pull and got the files back after the other developer put them back. It seems that some things in git are a bit confusing; it may be quite powerful, but with that comes confusion, at least on my part, on how to use it.

In order to make my life easier, I've been using Tower as a GUI for git. I've also been working a little with the command line.

Either I'm a bit thickheaded about learning git or it is a tool that does so much that learning it takes an entire course. Hopefully after a few more weeks with it, I'll be more comfortable. At the same time, I hope the other developers on my project don't bite off my head when I commit strange branches left and right!

Another rant about developers that should find a new line of work

I've been assigned to a new project where one of my tasks is to start refactoring and cleaning up the code to make it more readable and maintainable. I've ranted in the past about poorly written code, but some of what I've seen is unfathomable. Writing iOS software isn't that hard; writing good iOS software that someone else will maintain is much harder. Apparently the second half is where developers fall flat on their faces.

For instance, I saw code that checked to see if the device was an iPhone 4 in order to use 2x graphics. It's understandable that this check was needed for this case, but the code specifically checked for a GSM iPhone 4. This means that the check fails on a Verizon iPhone 4. Instead of doing the proper check for the scale of the screen, it became a bug that no one discovered until I hit it today. I also saw instances where after loading an loading from the network, it was converted into a UIImage and then set in an NSDictionary. Problem was, no check was ever made to ensure that the UIImage was non-nil and then when it was added to the NSDictionary, it blew up. Lovely.

Unless your company is named __MyCompanyName__, you shouldn't have this in the header of any file. Change it!

Another task that I took on was rearranging the project as files were strewn everywhere. No one else wanted to do this, so I took on this grunt task. It's definitely not fun, but neither is hunting through hundreds of files placed all over the place. Resources (images, xib, plists) DO NOT belong in the source folder; they belong in Resources. This is especially important for localization to make sure that you have all the right files. In addition, with all your images in one place, you can identify which images don't have 2x counterparts for the retina display.

I think that this goes back to what I've said before; anyone that hires an outside developer (or even uses their own staff) needs to find a trusted and experience developer to at least look at the code.

I'm sure that the more I dig into this, the more disgusted I'll be, but so far, the work isn't really difficult, just time consuming. With all the cool parts of writing code, developers do have to take on the icky parts.

Where did common courtesy go?

While I was running today, I noticed a pair of glasses in the middle of the street. I picked them up as I noticed some people had just walked by and thought that they had dropped them. When I approached the two (a nurse taking her patient for a walk), the nurse said that the glasses weren't hers and they had seen them in the street.

OK, that's fine, but leaving the glasses in the street to get crushed? I saw that there was a box for underground telephone lines, so I set the glasses on the box hoping that someone will come back and find them. At least by putting them there, the owner has half a chance of recovering non-broken glasses. Why the nurse didn't pick up the glasses and do the same thing I did, I have no idea.

Sacrificing Readability for Cleverness

I was looking at some code in the Three20 library when I came across this:

BOOL TTIsKeyboardVisible() {
  // Operates on the assumption that the keyboard is visible if and only if there is a first
  // responder; i.e. a control responding to key events
  UIWindow* window = [UIApplication sharedApplication].keyWindow;
  return !![window findFirstResponder];
}

The last line had me scratching my head as it has a double negation. While this looks like a mistake, it isn't; this is a developer being clever. Let's break down the line:

[window findFirstResponder]

This returns a nil value or a non-nil value. If we then negate it the result is YES if the firstResponder is nil and NO if the firstResponder is non-nil. If we negate it again, the rest is NO if the firstResponder is nil and YES if the firstResponder is non-nil. So, the bottom line is:

return [window findFirstResponder] != nil;

Why did the developer use the hard to read !![window findFirstResponder] when [window findFirstResponder] != nil is much more readable?

(We'll ignore the fact that initial assumption is not always valid; I believe there are cases where the firstResponder isn't a text field. Using keyboard notifications is the way to tell if a keyboard is visible.)

Writing clever code like this drives me insane as there is no reason to take shortcuts; a few extra characters isn't the end of the world and the next person that reads the code has to closely examine the syntax to ensure that the !! isn't an error.