Question

I've made a document class that downloads and reads the text in it. The smart thing is that it only downloads and reads the text in the document when or if it's needed. By using the Text property it'll try to read the document, if it hasn't been downloaded it'll download it then read it.

It's very nice. However I've noticed my use of Exceptions leads to some funky code. See below.

Document class

public delegate byte[] DownloadBinaryDelegate(IDocument document);
public delegate string TextReaderDelegate(IDocument document);

public class Document
{
        public DownloadBinaryDelegate DownloadBinaryDelegate { private get; set; }
        public TextReaderDelegate TextReaderDelegate { private get; set; }

        private bool _binaryIsSet;
        private byte[] _binary;
        public byte[] Binary
        {
            get 
            {
                if (_binaryIsSet)
                    return _binary;

                if (DownloadBinaryDelegate == null)
                    throw new NullReferenceException("No delegate attached to DownloadBinaryDelegate.");

                Binary = DownloadBinaryDelegate(this);
                DownloadBinaryDelegate = null; // unhock delegate as it's no longer needed.

                return _binary;
            }
            set
            {
                if (_binaryIsSet) 
                    return;

                _binary = value;
                _binaryIsSet = true;
            }
        }

        private bool _textIsSet;
        private string _text;
        public string Text
        {
            get
            {
                if (_textIsSet)
                    return _text;

                if (TextReaderDelegate == null)
                    throw new NullReferenceException("No delegate attached to TextReaderDelegate.");

                Text = TextReaderDelegate(this); // this delegate will call Binary and return the translated text.
                TextReaderDelegate = null; // unhock delegate as it's no longer needed.

                return _text;
            }
            set
            {
                if (_textIsSet)
                    return;

                _text = value;
                _textIsSet = true;
            }
        }

The Problem

What I wrote in the first place.

if (document.Text == null) // text is not set
{
    if (document.Binary == null) // binary has not been downloaded
        document.DownloadBinaryDelegate = Util.DownloadDocument;

    document.TextReaderDelegate = Util.ReadDocument;
}

Totally forgetting that the Text property throws an exception. So I have to write something like this, which is a bit funky code.

// check if text has already been read and set
try
{
    var isTextSet = document.Text == null;
}
catch (NullReferenceException)
{
    document.DownloadBinaryDelegate = Util.DownloadDocument;
    document.TextReaderDelegate = Util.ReadDocument;
}

I hope you can see what I mean.

So my question, is this a bad design? How would you have done it? Keep in mind I would still like the current functionality.

Was it helpful?

Solution 2

I was unable to use Lazy as I'm using delegate(this) which is not allowed.

So I ended up using the fine answer from here: What exception type to use when a property cannot be null?

public class Document
{
    private DownloadBinaryDelegate _downloadBinaryDelegate;
    public void SetDownloadBinaryDelegate(DownloadBinaryDelegate downloadBinary)
    {
        if (downloadBinary == null)
            throw new ArgumentNullException("downloadBinary");

        _downloadBinaryDelegate = downloadBinary;
    }

    private TextReaderDelegate _textReaderDelegate;
    public void SetTextReaderDelegate(TextReaderDelegate readerDelegate)
    {
        if (readerDelegate == null)
            throw new ArgumentNullException("readerDelegate");

        _textReaderDelegate = readerDelegate;
    }

    private bool _binaryIsSet;
    private byte[] _bytes;

    public void SetBinary(byte[] bytes, bool forceOverwrite = false)
    {
        if (_binaryIsSet && !forceOverwrite)
            return;

        _bytes = bytes;
        _binaryIsSet = true;
    }

    public byte[] GetBinary()
    {
        if (_binaryIsSet)
            return _bytes;

        if (_downloadBinaryDelegate == null)
            throw new InvalidOperationException("No delegate attached to DownloadBinaryDelegate. Use SetDownloadBinaryDelegate.");

        SetBinary(_downloadBinaryDelegate(this));
        _downloadBinaryDelegate = null; // unhock delegate as it's no longer needed.

        return _bytes;
    }

    public bool TryGetBinary(out byte[] bytes)
    {
        if (_binaryIsSet)
        {
            bytes = _bytes;
            return true;
        }

        if (_downloadBinaryDelegate != null)
        {
            bytes = GetBinary(); // is this legit?
            return true;
        }

        bytes = null;
        return false;
    }

    private bool _textIsSet;
    private string _text;

    public void SetText(string text, bool forceOverwrite = false)
    {
        if (_textIsSet && !forceOverwrite)
            return;

        _text = text;
        _textIsSet = true;
    }

    public string GetText()
    {
        if (_textIsSet)
            return _text;

        if (_textReaderDelegate == null)
            throw new InvalidOperationException("No delegate attached to TextReaderDelegate. Use SetTextReaderDelegate.");

        SetText(_textReaderDelegate(this)); // this delegate will call Binary and return the read text.
        _textReaderDelegate = null; // unhock delegate as it's no longer needed.

        return _text;
    }

    public bool TryGetText(out string text)
    {
        byte[] bytes;
        if (!TryGetBinary(out bytes))
        {
            text = null;
            return false;
        }

        if (_textIsSet)
        {
            text = _text;
            return true;
        }

        if (_textReaderDelegate != null)
        {
            text = GetText(); // is this legit?
            return true;
        }

        text = null;
        return false;
    }
}

OTHER TIPS

Lazy initialization is already baked into the .NET framework. I would suggest re-implementing your class using Lazy<T>.

To answer your specific question, it sounds like your class will always require the Binary and Text delegates, so I would make them required parameters to the constructor.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top