4
\$\begingroup\$

A manager for a speedy async saving objects to isolated storage, using serialization from Newtonsoft.Json. A project to play with is here.

public static async Task<T> ReadJsonEx<T>(String filepath)
    {
        if (String.IsNullOrEmpty(filepath))
            return default(T);

        return await await Task.Factory.StartNew(async () =>
        {
            using (var store = IsolatedStorageFile.GetUserStoreForApplication())
            using (var stream = new IsolatedStorageFileStream(filepath, FileMode.Open, store))
            using (var sr = new StreamReader(stream))
            using (var jr = new JsonTextReader(sr))
                return await jr.ReadJsonAsyncTask<T>();
        });
    }

    public static async Task<bool> WriteJsonEx<T>(String filepath, T content)
    {
        if (String.IsNullOrEmpty(filepath))
            return false;

        return await await Task.Factory.StartNew(async () =>
        {
            using (var store = IsolatedStorageFile.GetUserStoreForApplication())
            using (var stream = new IsolatedStorageFileStream(filepath, FileMode.Create, store))
            using (var sw = new StreamWriter(stream))
            using (var jw = new JsonTextWriter(sw))
                await jw.WriteJsonAsyncTask(content);

            return true;
        });
    }

Where extensions are

private static readonly JsonSerializer JsonSerializer = new JsonSerializer { NullValueHandling = NullValueHandling.Ignore, MissingMemberHandling = MissingMemberHandling.Ignore };

    public static async Task<bool> WriteJsonAsyncTask<T>(this JsonTextWriter writer, T content)
    {
        writer.Formatting = Formatting.Indented;
        return await TaskEx.Run(() =>
        {
            try { JsonSerializer.Serialize(writer, content); }
            catch (Exception) { return false; }
            return true;
        });
    }

    public static async Task<T> ReadJsonAsyncTask<T>(this JsonTextReader reader)
    {
        return await TaskEx.Run(() => JsonSerializer.Deserialize<T>(reader));
    }

I'm just wondering if this code is secure and if it can be used in the real app. Is the current approach safe enough?

\$\endgroup\$
4
  • 2
    \$\begingroup\$ These edits can probably earn you some rep if you word them into an answer / "selfie" review ;) \$\endgroup\$ – Mathieu Guindon Apr 28 '14 at 17:23
  • \$\begingroup\$ @Mat'sMug fair enough :) Thanks, would move them there. \$\endgroup\$ – Vitalii Vasylenko Apr 28 '14 at 18:25
  • \$\begingroup\$ Also, please don't edit the code in the question. I've rolled back Rev 14 → 13. \$\endgroup\$ – 200_success Apr 28 '14 at 20:09
  • \$\begingroup\$ @200_success it was almost the same, just with extra CreateFolderIfNecessary() call. Though, i got your point: code samples should stay original. \$\endgroup\$ – Vitalii Vasylenko Apr 28 '14 at 20:33
3
\$\begingroup\$

This is very neat. Just a few code formatting nitpicks:

    if (String.IsNullOrEmpty(filepath))
        return default(T);

    return await await Task.Factory.StartNew(async () =>
    {
        using (var store = IsolatedStorageFile.GetUserStoreForApplication())
        using (var stream = new IsolatedStorageFileStream(filepath, FileMode.Open, store))
        using (var sr = new StreamReader(stream))
        using (var jr = new JsonTextReader(sr))
            return await jr.ReadJsonAsyncTask<T>();
    });

Call me over-zealous, I think it would be best to specify the scopes with curly braces - I like the stacked using blocks though, so like this:

    if (String.IsNullOrEmpty(filepath))
    {
        return default(T);
    }

    return await await Task.Factory.StartNew(async () =>
    {
        using (var store = IsolatedStorageFile.GetUserStoreForApplication())
        using (var stream = new IsolatedStorageFileStream(filepath, FileMode.Open, store))
        using (var sr = new StreamReader(stream))
        using (var jr = new JsonTextReader(sr))
        {
            return await jr.ReadJsonAsyncTask<T>();
        }
    });

And here too:

public static async Task<bool> WriteJsonAsyncTask<T>(this JsonTextWriter writer, T content)
{
    writer.Formatting = Formatting.Indented;
    return await TaskEx.Run(() =>
    {
        try { JsonSerializer.Serialize(writer, content); }
        catch (Exception) { return false; }
        return true;
    });
}

I find code reads better going down than going across:

public static async Task<bool> WriteJsonAsyncTask<T>(this JsonTextWriter writer, T content)
{
    writer.Formatting = Formatting.Indented;
    return await TaskEx.Run(() =>
    {
        try 
        {
            JsonSerializer.Serialize(writer, content);
            return true;
        }
        catch (Exception) 
        { 
            return false; 
        }
    });
}
\$\endgroup\$
6
  • 2
    \$\begingroup\$ As a matter of style, I'd also put the return true; within the catch block to show the entire "happy path". \$\endgroup\$ – Jesse C. Slicer Apr 28 '14 at 18:14
  • \$\begingroup\$ @JesseC.Slicer good point - I put it in the try block though ;) \$\endgroup\$ – Mathieu Guindon Apr 28 '14 at 18:15
  • \$\begingroup\$ @Mat'sMug Thanks for a review. However, it is indeed only a matter of style :) \$\endgroup\$ – Vitalii Vasylenko Apr 28 '14 at 18:29
  • \$\begingroup\$ @VitaliiVasylenko Meh, the downside of reviewing nice code ;) \$\endgroup\$ – Mathieu Guindon Apr 28 '14 at 18:35
  • \$\begingroup\$ @Mat'sMug well.. agree :) \$\endgroup\$ – Vitalii Vasylenko Apr 28 '14 at 18:48
3
\$\begingroup\$

Talking about code stability, I had found several issues so far:

  1. I tried to call WriteJsonEx() from the Application_Deactivated() to save some data - sometimes it works, but several times it failed to finish writing before app deactivates.

    I found an answer here. In a short: calling async during app deactivation is a bad idea. Sync calls should be used instead (app has several seconds to store data, so sync calls should works fine).

  2. No folder autocreation

    Just call DirectoryExists() and CreateDirectory() if needed. Will update sample in a while.

  3. Is it thread-safe?

    Probably adding lock() would help, should test it deeper.

EDIT: I'm doing some crash-tests here. Will publish the results in this thread either.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.