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!!!

Share

Unit Tests in User Configurable Applications

Yesterday was another one of those little lessons in Unit Testing.

In one of the applications I work on there is a lot of clever string manipulation in order to derive intelligence from the data contained in the string.  Lots of array generation and scoring of individual elements within those arrays and so on.  The problem was during debugging within Visual Studio I was getting the desired output from the process I was unit testing.  All good.  The problem was as soon as that same test hit the build server it was failing.

I spent quite a while trying to track down what might be happening.  Since it was all geared around string manipulation I was looking for something related to any string comparison code I’ve written.  Is this a culture issue?  Am I making some obvious mistake with how some of the arrays are being treated?  Round and round looking at the same code trying to figure it out.

Those moments when your “coding cool” starts to ebb away and morph into pure frustration at mild annoyances.

After about an hour (a few cups of coffee) I suddenly realised what the problem was.

Since I was obviously debugging within Visual Studio this making use of the application in the context of which I have been using the application to debug.  Meaning that the user configurable options that I have set were being used.  This was not the case on the build server as the application hadn’t ever been run on that machine and certainly not in the context of the build servers build process.  So the two were not matching in configuration.

The setting in question allows the user to switch on deeper “fuzzy matching” on the string manipulation code.  If the code runs through it’s normal matching process and still hasn’t found an obvious match if this “Use Fuzzy Matching” item is turned out another set of rules come into play and the system then attempts to make a fuzzy “best guess” which, obviously, will create a different result.

So, todays lesson is when there are user configs involved in the code being tested, make sure you explicitly set these in each unit test!

Share