Code Reviewing – Xamarin Components and MvvmCross Plugins

At the moment I’m working through the implementation of a mobile application build on top of the Xamarin platform targeting Android, iOS and eventually Windows Phone as well.

I’ve done a lot of Mvvm in the past with various personal and commerical WPF applications so not a lot of this is new.  The paradigms are very familiar but some of the approaches are new.  I’ve not done a whole lot with Portable Class Libraries for instance.  I’m also new to the device platforms I’m working on as well but viewing through the familiar C#/Java code makes things a lot more comfortable even if the underlying implementations are slightly different.

I’m reviewing a lot of security code at the moment, plugins and approaches and frankly I’m slightly concerned by what I’m seeing.  Before I say anything else I’m finding the development flow with Xamarin and MvvmCross to be really great.  Both of these platforms and frameworks are really great.

Lots of hardcoded strings used for encryption.  OK, not so bad considering the inevitable trade off between reliability, ease of use and the areas where one is comfortable to accept some risks given the contexts.

With all that said I’m seeing some very poor coding practices employed in some areas.

• Lots of badly formatted code(not following established coding standards)
• Code littered with magic strings even within the same class file.
• Numerous instances of “” rather than string.Empty.  (pedantic? maybe but mobile apps need to be effecient and this simply isn’t)
• Swathes of methods and properties with no documentation or comments.
• Lots of hardcoded values not stored in constants

That is just lazy coding, nothing else.  A quick run of any of the code analysis in Visual Studio will highlight all of this.  It seems that none of the component authors I’ve seen thus far have ReSharper for instance.  Some of this I’m seeing even in released components available through the Xamarin Component Store.  Really not good.

I’m also seeing some components named in such a way that if you didn’t review the source code you could easily assume they are doing something when in fact they are not.

The long and short of it is I would suggest that you review the code for any component you plan to use before shoving it all in your solutions and forgetting about it.  You really shouldn’t be doing that btw, not when the source code is available to see online via a simple left click!!

Anyway, I’m finding lots of good examples of how to do things but relatively poorly implemented in many instances.  So far I have basically backed out from using any components in favour of implementing them myself.  In the small number of cases when I have used a component I’ve made notes in the classes, added tickets to YouTrack and added TODOs in the code that will be reviewed and ported into my own codebase prior to product release.

DO YOUR REVIEWS PEOPLE!!!

Example – IHS.MvvmCross.Plugins.Keychain

I’ve been building my own proprietary solution to interact with the various keystores on the mobile platforms.  I have to say that I’m loving the architectures.  Having come from years of WPF/Prism/Mvvm this all feels like second nature to me these days.  But along the way I’m finding some real horrors, it has to be said.  Take this as an example:

public string GetPassword(string serviceName, string account)
{
    var storedAccount = FindAccountsForService(serviceName).FirstOrDefault(ac => ac.Username == account);
    return storedAccount != null ? storedAccount.Password : null;
}

private IEnumerable<LoginDetails> FindAccountsForService(string serviceId)
{
    var r = new List<LoginDetails>();

    var postfix = "-" + serviceId;

    var aliases = _keyStore.Aliases();
    while (aliases.HasMoreElements)
    {
        var alias = aliases.NextElement().ToString();
        if (alias.EndsWith(postfix))
        {
            var e = _keyStore.GetEntry(alias, _passwordProtection) as KeyStore.SecretKeyEntry;
            if (e != null)
            {
                var bytes = e.SecretKey.GetEncoded();
                var serialized = Encoding.UTF8.GetString(bytes);
                var acct = LoginDetails.Deserialize(serialized);
                r.Add(acct);
            }
        }
    }

    r.Sort((a, b) => a.Username.CompareTo(b.Username));

    return r;
}

I literally do not know where to start to point out the problem and inefficiencies with the code I’ve shown above. What’s wrong with doing this instead?

var alias = MakeAlias(username, serviceId);
_keyStore.GetEntry(alias, _passwordProtection);

Why iterate over every single possibly stored value in the key file, then deserialise each entry in the store, build a list containing many entries and then pull out the specific one you are actually looking for? What a complete waste of valuable resources!!!