Two overloaded methods to move an e-mail message to another mailbox folder

Question

I have an API for managing an email mailbox. There is a class called Message, and it has two Move overloads:

/// <summary>
/// Move this message to the mailbox folder whose path is provided.
/// </summary>
/// <param name="toFolderPath">The path to the mailbox folder to which this message will be moved.</param>
/// <param name="createFolder">If true, the destination folder will be created first if it doesn't exist.</param>
/// <exception cref="InvalidOperationException">
/// <para>An exception will be thrown if this message does not reside in a mailbox because
/// only messages in mailboxes can be moved between folders.  Use <see cref="ContainingFolder"/> to determine if
/// the message resides within a mailbox and folder; that value will be null if it does not.</para>
/// <para>An exception will be thrown if the destination folder does not exist and if the provided <paramref name="createFolder"/> is false.</para>
/// </exception>
public void Move(string toFolderPath, bool createFolder)
{
    ValidateMoveCopy();
    Move(GetCreateFolder(toFolderPath, createFolder));
}

/// <summary>
/// Move this message to the provided mailbox folder.
/// </summary>
/// <param name="toFolder">The mailbox folder to which this message will be moved.</param>
/// <exception cref="InvalidOperationException">An exception will be thrown if this message does not reside in a mailbox because
/// only messages in mailboxes can be moved between folders.  Use <see cref="ContainingFolder"/> to determine if
/// the message resides within a mailbox and folder; that value will be null if it does not.</exception>
public void Move(MailboxFolder toFolder)
{
    ValidateMoveCopy();
    ParentMailbox.MoveMessage(this, toFolder);
}

The first overload will retrieve the mailbox folder or create it if it doesn't yet exist, but only if the createFolder argument is true; then it will just call the second overload.

What I'd like to avoid is calling the ValidateMoveCopy twice when the first overload is used (once in that method, and once in the second overload); however, I don't want GetCreateFolder to be called in the first method if validation fails.

Here are GetCreateFolder and ValidateMoveCopy:

/// <summary>
/// Return the folder with the provided path, if it exists, from the parent mailbox,
/// or else create it if it should be created based on <paramref name="createIfMissing"/>
/// </summary>
/// <param name="withFolderPath"></param>
/// <param name="createIfMissing"></param>
/// <returns></returns>
private MailboxFolder GetCreateFolder(string withFolderPath, bool createIfMissing)
{
    if (createIfMissing)
    {
        return ParentMailbox.CreateFolder(withFolderPath); // create or retrieve if existing.
    }
    else // folder won't be created if it doesn't exist.
    {
        var ret = ParentMailbox.GetFolder(withFolderPath);
        if (ret == null) throw new InvalidOperationException(string.Format(@"The folder ""{0}"" does not exist in mailbox ""{1}"" and will not be created.", withFolderPath, ParentMailbox.Name));
        return ret;
    }
}

/// <summary>
/// Validate this message for a "move" or "copy" operation, and throw an exception if not valid.
/// </summary>
private void ValidateMoveCopy()
{
    if (ParentMailbox == null) throw new InvalidOperationException("This message does not reside in a mailbox and cannot be moved or copied.");
}

Is there a good way to do this? Since both overloads are public, I don't want to do something like add a bool isAlreadyValidated to the second overload. I could use a private field:

/// <summary>
/// True only if a move operation is underway and it has already been validated.
/// </summary>
private bool _isValidatedForMove = false;

and use it like this:

/// <summary>
/// Move this message to the mailbox folder whose path is provided.
/// </summary>
/// <param name="toFolderPath">The path to the mailbox folder to which this message will be moved.</param>
/// <param name="createFolder">If true, the destination folder will be created first if it doesn't exist.</param>
/// <exception cref="InvalidOperationException">
/// <para>An exception will be thrown if this message does not reside in a mailbox because
/// only messages in mailboxes can be moved between folders.  Use <see cref="ContainingFolder"/> to determine if
/// the message resides within a mailbox and folder; that value will be null if it does not.</para>
/// <para>An exception will be thrown if the destination folder does not exist and if the provided <paramref name="createFolder"/> is false.</para>
/// </exception>
public void Move(string toFolderPath, bool createFolder)
{
    ValidateMoveCopy();
    _isValidatedForMove = true;
    Move(GetCreateFolder(toFolderPath, createFolder));
    _isValidatedForMove = false;
}

/// <summary>
/// Move this message to the provided mailbox folder.
/// </summary>
/// <param name="toFolder">The mailbox folder to which this message will be moved.</param>
/// <exception cref="InvalidOperationException">An exception will be thrown if this message does not reside in a mailbox because
/// only messages in mailboxes can be moved between folders.  Use <see cref="ContainingFolder"/> to determine if
/// the message resides within a mailbox and folder; that value will be null if it does not.</exception>
public void Move(MailboxFolder toFolder)
{
    if(!_isValidatedForMove) ValidateMoveCopy();
    ParentMailbox.MoveMessage(this, toFolder);
}

But that feels kind of like a code smell for some reason. I welcome any thoughts or suggestions. By the way, I'm aware this code isn't thread safe.

Answer 1

I think the best option would be to introduce a third, private method:

private void Move(MailboxFolder toFolder, bool requiresValidation)
{
    if (requiresValidation)
    {
        ValidateMoveCopy();
    }
    ParentMailbox.MoveMessage(this, toFolder);
}

Then your other methods just call that one.

public void Move(MailboxFolder toFolder)
{
    Move(toFolder, true);
}

public void Move(string toFolderPath, bool createFolder)
{
    ValidateMoveCopy();
    MailboxFolder toFolder = GetOrCreateFolder(toFolderPath, createFolder);
    Move(toFolder, false);
}   

I renamed GetCreateFolder to GetOrCreateFolder which is slightly easier to read IMO. I've also introduced a variable for toFolder as I don't like lines of code that do 2 things.

Edit:

Short and concise methods are brilliant and a very good thing to aim for. However, consider your method at a glance:

Move(GetCreateFolder(toFolderPath, createFolder));

How easy is it to spot the side effect there (possibly creating a new folder) vs my version:

MailboxFolder toFolder = GetOrCreateFolder(toFolderPath, createFolder);
Move(toFolder, false);

It's a stylistic thing without doubt and if you prefer the brevity then that's absolutely fine. I should have been more explicit about that being a preference thing rather than a rule!