5
\$\begingroup\$

I'm at loggerheads between performing operations one-by-one within a method, performing all of them at once (in one block) within the method, or breaking them up into four different methods.

In terms of best-practice, which of the following three options seems most advantageous, and why would it be preferred over the other options?

1st Option

Call the code:

string filename = string.Empty;
DialogResult result = openFileDialog1.ShowDialog();
if (result == DialogResult.OK)
{
    filename = openFileDialog1.FileName;
}
else
{
    MessageBox.Show("No file selected - exiting");
    return;
}
SpacelessWordBreakAerator(filename);

Implementation of code:

    private void SpacelessWordBreakAerator(string filename)
    {
        string soughtCombo = string.Empty;
        string desiredCombo = string.Empty;
        List<string> specialWordEndings = new List<String>();
        specialWordEndings.Add("é");
        specialWordEndings.Add("í");
        specialWordEndings.Add("ñ"); 
        specialWordEndings.Add("?");
        specialWordEndings.Add("!");
        specialWordEndings.Add(",");
        specialWordEndings.Add(".");
        specialWordEndings.Add(":");
        specialWordEndings.Add(";");
        specialWordEndings.Add("\"");
        specialWordEndings.Add("»");
        specialWordEndings.Add("ß");
        List<string> specialWordBeginnings = new List<String>();
        specialWordBeginnings.Add("¿");
        specialWordBeginnings.Add("¡");
        specialWordBeginnings.Add("\"");
        specialWordBeginnings.Add("«");

        // Aerate "special" combinations:
        Cursor.Current = Cursors.WaitCursor;
        try
        {
            using (DocX document = DocX.Load(filename))
            {
                foreach (string endChar in specialWordEndings)
                {
                    foreach (string beginChar in specialWordBeginnings)
                    {
                        soughtCombo = string.Format("{0}{1}", endChar, beginChar);
                        desiredCombo = string.Format("{0} {1}", endChar, beginChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }
                document.Save(); // Save after each step
            }
            // Aerate "special" ending with "normal" beginning - create a new DocX object with each different logic block
            using (DocX document = DocX.Load(filename))
            {
                foreach (string endChar in specialWordEndings)
                {
                    for (int i = FIRST_CAP_POS; i <= LAST_CAP_POS; i++)
                    {
                        char upperChar = (char)i;
                        soughtCombo = string.Format("{0}{1}", endChar, upperChar);
                        desiredCombo = string.Format("{0} {1}", endChar, upperChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }
                document.Save();
            }
            // Aerate "normal" ending with "special" beginning
            using (DocX document = DocX.Load(filename))
            {
                for (int i = FIRST_LOWER_POS; i <= LAST_LOWER_POS; i++)
                {
                    char lowerChar = (char)i;
                    foreach (string beginChar in specialWordBeginnings)
                    {
                        soughtCombo = string.Format("{0}{1}", lowerChar, beginChar);
                        desiredCombo = string.Format("{0} {1}", lowerChar, beginChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }
                document.Save();
            }
            // Aerate "normal" combinations (this is the legacy "InsertSpaceBetweenLowercaseAndUppercaseChars" helper method)
            using (DocX document = DocX.Load(filename))
            {
                for (int i = FIRST_LOWER_POS; i <= LAST_LOWER_POS; i++)
                {
                    char lowerChar = (char)i;
                    for (int j = FIRST_CAP_POS; j <= LAST_CAP_POS; j++)
                    {
                        char upperChar = (char)j;
                        string originalStr = string.Format("{0}{1}", lowerChar, upperChar);
                        string newStr = string.Format("{0} {1}", lowerChar, upperChar);
                        document.ReplaceText(originalStr, newStr);
                    }
                }
                document.Save();
            }
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
        MessageBox.Show("SpacelessWordBreakAerator() finished! Aintcha glad?!?");
    }

2nd Option

Call the code: (same as first option)

Implementation of code:

    private void SpacelessWordBreakAerator(string filename)
    {
// same var and const declarations as in first option...

        Cursor.Current = Cursors.WaitCursor;
        try
        {
// Keep using the DocX object until the end, and save it only after all operations have completed
            using (DocX document = DocX.Load(filename))
            {
                // Aerate "special" combinations:
                foreach (string endChar in specialWordEndings)
                {
                    foreach (string beginChar in specialWordBeginnings)
                    {
                        soughtCombo = string.Format("{0}{1}", endChar, beginChar);
                        desiredCombo = string.Format("{0} {1}", endChar, beginChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }

            // Aerate "special" ending with "normal" beginning
                foreach (string endChar in specialWordEndings)
                {
                    for (int i = FIRST_CAP_POS; i <= LAST_CAP_POS; i++)
                    {
                        char upperChar = (char)i;
                        soughtCombo = string.Format("{0}{1}", endChar, upperChar);
                        desiredCombo = string.Format("{0} {1}", endChar, upperChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }

            // Aerate "normal" ending with "special" beginning
                for (int i = FIRST_LOWER_POS; i <= LAST_LOWER_POS; i++)
                {
                    char lowerChar = (char)i;
                    foreach (string beginChar in specialWordBeginnings)
                    {
                        soughtCombo = string.Format("{0}{1}", lowerChar, beginChar);
                        desiredCombo = string.Format("{0} {1}", lowerChar, beginChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }

            // Aerate "normal" combinations (this is the legacy "InsertSpaceBetweenLowercaseAndUppercaseChars" helper method)
                for (int i = FIRST_LOWER_POS; i <= LAST_LOWER_POS; i++)
                {
                    char lowerChar = (char)i;
                    for (int j = FIRST_CAP_POS; j <= LAST_CAP_POS; j++)
                    {
                        char upperChar = (char)j;
                        string originalStr = string.Format("{0}{1}", lowerChar, upperChar);
                        string newStr = string.Format("{0} {1}", lowerChar, upperChar);
                        document.ReplaceText(originalStr, newStr);
                    }

                }
                document.Save();
            }
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
        MessageBox.Show("SpacelessWordBreakAerator() finished! Aintcha glad?!?");
    }

3rd Option

Call the code:

string filename = string.Empty;
DialogResult result = openFileDialog1.ShowDialog();
if (result == DialogResult.OK)
{
    filename = openFileDialog1.FileName;
}
else
{
    MessageBox.Show("No file selected - exiting");
    return;
}
// Break it up into four separate methods
SpacelessWordBreakUnusualCombo(filename);
SpacelessWordBreakUnusualEndNormalBegin(filename);
SpacelessWordBreakNormalEndUnusualBegin(filename);
SpacelessWordBreakNormalCombo(filename);

Implementation of code:

    private void SpacelessWordBreakUnusualCombo(string filename)
    {
        string soughtCombo = string.Empty;
        string desiredCombo = string.Empty;
        List<string> specialWordEndings = new List<String>();
        specialWordEndings.Add("é");
        specialWordEndings.Add("í");
        specialWordEndings.Add("ñ"); 
        specialWordEndings.Add("?");
        specialWordEndings.Add("!");
        specialWordEndings.Add(",");
        specialWordEndings.Add(".");
        specialWordEndings.Add(":");
        specialWordEndings.Add(";");
        specialWordEndings.Add("\"");
        specialWordEndings.Add("»");
        specialWordEndings.Add("ß");
        List<string> specialWordBeginnings = new List<String>();
        specialWordBeginnings.Add("¿");
        specialWordBeginnings.Add("¡");
        specialWordBeginnings.Add("\"");
        specialWordBeginnings.Add("«");

        // Aerate "special" combinations:
        Cursor.Current = Cursors.WaitCursor;
        try
        {
            using (DocX document = DocX.Load(filename))
            {
                foreach (string endChar in specialWordEndings)
                {
                    foreach (string beginChar in specialWordBeginnings)
                    {
                        soughtCombo = string.Format("{0}{1}", endChar, beginChar);
                        desiredCombo = string.Format("{0} {1}", endChar, beginChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }
                document.Save(); // Save after each step
            }
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
        MessageBox.Show("SpacelessWordBreakAerator() finished! What a relief!");
    }

    private void SpacelessWordBreakNormalBeginUnusualEnd(filename)
    {
        string soughtCombo = string.Empty;
        string desiredCombo = string.Empty;
        List<string> specialWordEndings = new List<String>();
        specialWordEndings.Add("é");
        specialWordEndings.Add("í");
        specialWordEndings.Add("ñ"); 
        specialWordEndings.Add("?");
        specialWordEndings.Add("!");
        specialWordEndings.Add(",");
        specialWordEndings.Add(".");
        specialWordEndings.Add(":");
        specialWordEndings.Add(";");
        specialWordEndings.Add("\"");
        specialWordEndings.Add("»");
        specialWordEndings.Add("ß");

        // Aerate "special" combinations:
        Cursor.Current = Cursors.WaitCursor;
        try
        {
            // Aerate "special" ending with "normal" beginning
            using (DocX document = DocX.Load(filename))
            {
                foreach (string endChar in specialWordEndings)
                {
                    for (int i = FIRST_CAP_POS; i <= LAST_CAP_POS; i++)
                    {
                        char upperChar = (char)i;
                        soughtCombo = string.Format("{0}{1}", endChar, upperChar);
                        desiredCombo = string.Format("{0} {1}", endChar, upperChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }
                document.Save();
            }
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
        MessageBox.Show("SpacelessWordBreakAerator() finished! Ah, the beauty of it all!");
    }

    private void SpacelessWordBreakUnusualBeginNormalEnd(filename)
    {
        string soughtCombo = string.Empty;
        string desiredCombo = string.Empty;
        List<string> specialWordBeginnings = new List<String>();
        specialWordBeginnings.Add("¿");
        specialWordBeginnings.Add("¡");
        specialWordBeginnings.Add("\"");
        specialWordBeginnings.Add("«");

        Cursor.Current = Cursors.WaitCursor;
        try
        {
            using (DocX document = DocX.Load(filename))
            {
                for (int i = FIRST_LOWER_POS; i <= LAST_LOWER_POS; i++)
                {
                    char lowerChar = (char)i;
                    foreach (string beginChar in specialWordBeginnings)
                    {
                        soughtCombo = string.Format("{0}{1}", lowerChar, beginChar);
                        desiredCombo = string.Format("{0} {1}", lowerChar, beginChar);
                        document.ReplaceText(soughtCombo, desiredCombo);
                    }
                }
                document.Save();
            }
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
        MessageBox.Show("SpacelessWordBreakAerator() finished! Oh, wonderful day!");
    }

    private void SpacelessWordBreakNormalCombo(filename)
    {
        string soughtCombo = string.Empty;
        string desiredCombo = string.Empty;

        Cursor.Current = Cursors.WaitCursor;
        try
        {
            using (DocX document = DocX.Load(filename))
            {
                for (int i = FIRST_LOWER_POS; i <= LAST_LOWER_POS; i++)
                {
                    char lowerChar = (char)i;
                    for (int j = FIRST_CAP_POS; j <= LAST_CAP_POS; j++)
                    {
                        char upperChar = (char)j;
                        string originalStr = string.Format("{0}{1}", lowerChar, upperChar);
                        string newStr = string.Format("{0} {1}", lowerChar, upperChar);
                        document.ReplaceText(originalStr, newStr);
                    }
                }
                document.Save();
            }
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
        MessageBox.Show("SpacelessWordBreakAerator() finished! Happy happy joy joy!");
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ Why do you call .add() so many times? Why not go List<string> specialWordEndings = new List<String>(){"a","b","c"};? \$\endgroup\$ – Sam Jan 8 '14 at 10:42
  • \$\begingroup\$ Good point; that would be better form. \$\endgroup\$ – B. Clay Shannon Jan 8 '14 at 16:05
  • \$\begingroup\$ Have you looked at my answer? Your update suffers from the very same problems, which I gave you a complete solution for. \$\endgroup\$ – Dan Jan 8 '14 at 17:07
  • \$\begingroup\$ Also, I wouldn't use names like Popul8 and DuckbilledPlatypus in production code.. \$\endgroup\$ – Dan Jan 8 '14 at 17:08
  • 1
    \$\begingroup\$ Ah, platypus makes sense then, sorry! Why write Popul8 when you can write Populate? As for the rest—you asked for a review, and there are still issues in your code (namely, code repetition and useless int -> char conversions). I'm not sure why you don't want to fix those, especially if I gave you working code for that. Anyway, that's up to you of course. \$\endgroup\$ – Dan Jan 8 '14 at 19:32
7
\$\begingroup\$

I'd take the last option and then extract common functionality between

  • SpacelessWordBreakUnusualCombo
  • SpacelessWordBreakNormalBeginUnusualEnd
  • SpacelessWordBreakUnusualBeginNormalEnd

and other methods into a single method that accepts parameters.

On the glance, they look too similar so it should be just one method.

If I'm not mistaken, it should look like this:

private void ReplaceTextInFile(string filename, IDictionary<string, string> replacementMap)
{
    Cursor.Current = Cursors.WaitCursor;

    try
    {
        using (DocX document = DocX.Load(filename))
        {
            foreach (var replacement in replacementMap)
            {
                document.ReplaceText(replacement.Key, replacement.Value);
            }

            document.Save();
        }
    }
    finally
    {
        Cursor.Current = Cursors.Default;
    }
}

All your methods seem to be special cases of this method, with different replacementMap parameter.

Then create a method to build a Dictionary<string, string> representing all replacements you'll ever need:

    private static readonly List<string> SpecialWordBeginnings = new List<string> { "¿", "¡", "\"", "«" };
    private static readonly List<string> SpecialWordEndings = new List<string> { "é", "í", "ñ", "?", "!", ",", ".", ";", "\"", "»", "ß" };

    private static readonly IDictionary<string, string> ReplacementMap = BuildReplacementMap();

    private static IDictionary<string, string> BuildReplacementMap()
    {
        var result = new Dictionary<string, string>();

        foreach (var ending in SpecialWordEndings)
        {
            foreach (var beginning in SpecialWordBeginnings)
            {
                result.Add(
                    ending + beginning,
                    ending + " " + beginning
                );
            }

            for (var uppercaseChar = 'A'; uppercaseChar <= 'Z'; uppercaseChar++)
            {
                result.Add(
                    ending + uppercaseChar,
                    ending + " " + uppercaseChar
                );
            }
        }

        for (var lowercaseChar = 'a'; lowercaseChar <= 'z'; lowercaseChar++)
        {
            foreach (var beginning in SpecialWordBeginnings)
            {
                result.Add(
                    lowercaseChar + beginning,
                    lowercaseChar + " " + beginning
                );
            }

            for (var uppercaseChar = 'A'; uppercaseChar <= 'Z'; uppercaseChar++)
            {
                result.Add(
                    string.Concat(lowercaseChar, uppercaseChar),
                    lowercaseChar + " " + uppercaseChar
                );
            }
        }

        return result;
    }

Also note that you can do for on chars directly, no need to convert from int.

Since the replacement map is always the same, it is better to compute it once and store in a static field (which I did).

Finally, pass the map to our first method:

ReplaceTextInFile(filename, ReplacementMap);
\$\endgroup\$
  • \$\begingroup\$ I would move specialWordEndings and specialWordBeginnings to class static variables. This will save on memory and time when instantiating the class. \$\endgroup\$ – Jeff Vanzella Jan 7 '14 at 22:43
  • \$\begingroup\$ The map is data, and should probably be coming from a config file/database. Otherwise, spot on. \$\endgroup\$ – jmoreno Jan 8 '14 at 8:47
  • \$\begingroup\$ @jmoreno: I stand by generating it though. If the map itself came from database, it would not be trivial to add one more word beginning or ending (or e.g. support Cyrillic characters). Generating it preserves the logic and shows the intent. \$\endgroup\$ – Dan Jan 8 '14 at 10:50
  • \$\begingroup\$ @DanAbramov: I was referring to SpecialWordBeginnings and SpecialWordEndings. How much effort would be involved in adding "@" to SpecialWordBeginnings... \$\endgroup\$ – jmoreno Jan 8 '14 at 17:24
  • \$\begingroup\$ @jmoreno: Got it, sorry! \$\endgroup\$ – Dan Jan 8 '14 at 19:32
3
\$\begingroup\$

I would suggest a change to the top level logic to address the following issues.

  • Don't read/write the file multiple times.
  • Make it easy to add new DocX transformers.
  • Make each transformer an explicit class.

I would make a DocXTransformer class something like below.

public class DocXTransformer
{
    private DocX document;

    public DocXTransformer(string filename)
    {
        document = DocX.Load(filename);
    }

    public void Transform(IEnumerable<ITransformer> transformers)
    {
        foreach( var transform in transformers)
        {
            document = transform.Execute(document);
        }
        document.Save();
    }
}

Creating the DocXTransformer will load the file. Calling Transform, will execute a list of Transformer classes, which each implement the ITransform interface.

public interface ITransformer
{
    DocX Execute(DocX document);
}

I think this make the different logic parts separate and you will get cleaner code, where it's going to be easy to add a new transformer, while still paying some attention to performance.

\$\endgroup\$
  • \$\begingroup\$ It may be a good direction but until we know for sure there that are different kinds of transforms, this sounds like a premature generalization to me. Still, +1. \$\endgroup\$ – Dan Jan 8 '14 at 10:49

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.