Baconit, the excellent reddit client for Windows Phone and Windows 10 devices was made open source a while ago. As a .NET developer, I was interested in seeing the source code of a successful app I use often.
Code review
Disclaimer: I want to make a point, to make this point succinctly I will solely focus on what I think can be improved.
Secondly, I’m not a professional app developer and my expertise is more towards the backend, so I will limit the code review to the BaconBackend.
Thirdly, if you are the author or a contributor, don’t get offended or if you do, skip ahead to the Success review part 😉
Code file unit
Visual Studio is a great and versatile IDE. It has a Solution Explorer with great filtering capabilities, allowing you to navigate and discover your code, giving you an overview in the form of a tree.
That’s why I never understand why developers like putting more than one type, let alone a bunch of types, into one single code file. This totally puts the Solution Explorer out of order.
One of many examples, Collector.cs is a container containing 500 lines for 9 types, which I have to scroll through to get a grip on. I cannot get an overview at a glance.
Please, one type per code file, use namespaces to group types. A code file shouldn’t be a surprise package.
// This is a comment
Next thing I notice are comments stating the obvious, generating useless noise.
Here’s a random one: BaconBackend.DataObjects.User.HasMail – Indicates if the user has mail.
Browsing through the code, I suspect it was decided to indiscriminately document everything with xml comments. Public members, private members, doesn’t matter.
This makes the code base verbose and noisy, with detrimental value. Developers often get in this GhosDoc mode, adding a lot of generic comments, instead of adding few valuable comments – making you pause to think about why you have to add a comment in the first place.
Please don’t make comments the default, prefer verbose variable and method names that carry as much meaning as possible.
Let me Helper you
Helpers are a code smell to me. They smell like a shortcut or design fatigue, where you just say, screw it – I just need to get this done and don’t feel like finding this piece of code a place in the bigger picture.
The worst offender I have seen, has to be BaconBackend.Helpers.MiscellaneousHelper.
Miscallenous. Helper.
Unless you’re making an application for Santa, Helpers are not part of your domain.
Don’t send a manager to do a service’s job
More focused than helpers, but still too vague for a clear defined responsibility, are the typical managers.
Instead of SubredditManager that has a ChangeSubscriptionStatus method (among many other unrelated members), I’d prefer a Subreddit namespace with focused services like Subscriber. This makes it clear where to go for Subreddit related functionality and what functionality there is available to use.
Automated tests
There are none which is a sin in any non-trivial application. I think this is a big road block towards any change.
My complaints above might be superficial or not, but without a test harness in place, the risk will often outweigh the benefit of any improvement.
Success review
But what are these potential code improvements worth? I mean, I know what they’re claimed to be worth; I fight this battle every day. But at the same time you have to wonder, what will it buy you?
I feel software developers focus a lot of energy on “the way” software should be developed, but have nothing to show for but some dull enterprise software package no one loves to use.
Meanwhile, Baconit does its job and improves the Windows Phone, which usually gets ignored in the app space.
That’s why I think the points I made in the code review, should always be ignored if they stop or slow you down from delivering value.
One of my New Year’s resolutions is to be more like Quinn Damerell and get stuff done.
So, what if Baconit would add a meetup feature, where you could detect other Baconit users in your proximity? I’m going to add a MeetupManager right now… 🙂
Spare Time Code 