6
\$\begingroup\$

I recently started learning C# and am taking the opportunity to try and accelerate my introduction by helping the RubberDuck team (Website and GitHub Repo). The full class code that I'm working on can be found here. Its purpose is to create a Module in VBA that will allow unit testing.

  1. Since FakesFieldDeclarationFormat and AssertFieldDeclarationFormat are used only once would it be better to move them to just above where they are fed into DeclarationFormatFor?

  2. Would it be better to migrate FolderAnnotation into TestModuleEmptyTemplate since when formattedModuleTemplate is created that is ultimately what is happening?

  3. Does anyone have any suggestions for improving the names?

  4. Anything else that help improve the code and that may assist me in learning is welcome.

The main code in question is below.

    private const string TestModuleEmptyTemplate = "'@TestModule\r\n{0}\r\n{1}\r\n{2}\r\n\r\n";
    private const string FolderAnnotation = "'@Folder(\"Tests\")\r\n";

    private const string FakesFieldDeclarationFormat = "Private Fakes As {0}";
    private const string AssertFieldDeclarationFormat = "Private Assert As {0}";

    private readonly string _moduleInit = string.Concat(
        "'@ModuleInitialize\r\n",
        "Public Sub ModuleInitialize()\r\n",
        $"    '{RubberduckUI.UnitTest_NewModule_RunOnce}.\r\n",
        "    {0}\r\n",
        "    {1}\r\n",
        "End Sub\r\n\r\n",
        "'@ModuleCleanup\r\n",
        "Public Sub ModuleCleanup()\r\n",
        $"    '{RubberduckUI.UnitTest_NewModule_RunOnce}.\r\n",
        "    Set Assert = Nothing\r\n",
        "    Set Fakes = nothing\r\n",
        "End Sub\r\n\r\n"
    );

    private readonly string _methodInit = string.Concat(
        "'@TestInitialize\r\n"
        , "Public Sub TestInitialize()\r\n"
        , "    '", RubberduckUI.UnitTest_NewModule_RunBeforeTest, ".\r\n"
        , "End Sub\r\n\r\n"
        , "'@TestCleanup\r\n"
        , "Public Sub TestCleanup()\r\n"
        , "    '", RubberduckUI.UnitTest_NewModule_RunAfterTest, ".\r\n"
        , "End Sub\r\n\r\n"
    );

    private const string TestModuleBaseName = "TestModule";

    private string GetTestModule(IUnitTestSettings settings)
    {
        var assertType = string.Format("Rubberduck.{0}AssertClass", settings.AssertMode == AssertMode.StrictAssert ? string.Empty : "Permissive");
        var assertDeclaredAs = DeclarationFormatFor(AssertFieldDeclarationFormat, assertType, settings);

        var fakesType = "Rubberduck.IFake";
        var fakesDeclaredAs = DeclarationFormatFor(FakesFieldDeclarationFormat, fakesType, settings); 

        var formattedModuleTemplate = string.Format(TestModuleEmptyTemplate, FolderAnnotation, assertDeclaredAs, fakesDeclaredAs);

        if (settings.ModuleInit)
        {
            var assertBinding = InstantiationFormatFor(assertType, settings);
            var assertSetAs = $"Set Assert = {assertBinding}";

            var fakesBinding = InstantiationFormatFor(fakesType, settings);
            var fakesSetAs = $"Set Fakes = {fakesBinding}";

            formattedModuleTemplate += string.Format(_moduleInit, assertSetAs, fakesSetAs);
        }

        if (settings.MethodInit)
        {
            formattedModuleTemplate += _methodInit;
        }

        return formattedModuleTemplate;
    }

    private string InstantiationFormatFor(string type, IUnitTestSettings settings) 
    {
        const string EarlyBoundInstantiationFormat = "New {0}";
        const string LateBoundInstantiationFormat = "CreateObject(\"{0}\")";
        return string.Format(settings.BindingMode == BindingMode.EarlyBinding ? EarlyBoundInstantiationFormat : LateBoundInstantiationFormat, type); 
    }

    private string DeclarationFormatFor(string declarationFormat, string type, IUnitTestSettings settings) 
    {
        return string.Format(declarationFormat, settings.BindingMode == BindingMode.EarlyBinding ? type : "Object");
    }

The final output for VBA:

Early Binding:

Option Explicit

Option Private Module

'@TestModule
'@Folder("Tests")

Private Assert As Rubberduck.AssertClass
Private Fakes As Rubberduck.IFake

'@ModuleInitialize
Public Sub ModuleInitialize()
    'this method runs once per module.
    Set Assert = New Rubberduck.AssertClass
    Set Fakes = New Rubberduck.IFake
End Sub

'@ModuleCleanup
Public Sub ModuleCleanup()
    'this method runs once per module.
    Set Assert = Nothing
    Set Fakes = Nothing
End Sub

'@TestInitialize
Public Sub TestInitialize()
    'this method runs before every test in the module.
End Sub

'@TestCleanup
Public Sub TestCleanup()
    'this method runs after every test in the module.
End Sub

Late Binding:

Option Explicit

Option Private Module

'@TestModule
'@Folder("Tests")

Private Assert As Object
Private Fakes As Object

'@ModuleInitialize
Public Sub ModuleInitialize()
    'this method runs once per module.
    Set Assert = CreateObject("Rubberduck.AssertClass")
    Set Fakes = CreateObject("Rubberduck.IFake")
End Sub

'@ModuleCleanup
Public Sub ModuleCleanup()
    'this method runs once per module.
    Set Assert = Nothing
    Set Fakes = Nothing
End Sub

'@TestInitialize
Public Sub TestInitialize()
    'this method runs before every test in the module.
End Sub

'@TestCleanup
Public Sub TestCleanup()
    'this method runs after every test in the module.
End Sub
\$\endgroup\$
  • \$\begingroup\$ Oops, should've caught that on github - Set Fakes = New Rubberduck.IFake should be Set Fakes = New Rubberduck.FakesProvider, declared as IFakesProvider - an IFake would be e.g. MsgBoxFake, and we don't New up an interface just like that anyway. \$\endgroup\$ – Mathieu Guindon Apr 13 '17 at 11:53
6
\$\begingroup\$

Only targeting _moduleInit and _methodInit

IMO the use of string.Concat with many parameters together with \r\n is hurting the readability a lot.

How about using a StringBuilder instead like so

private readonly string _moduleInit = new StringBuilder(256)
    .AppendLine("'@ModuleInitialize")
    .AppendLine("Public Sub ModuleInitialize()")
    .AppendLine($"    '{RubberduckUI.UnitTest_NewModule_RunOnce}.")
    .AppendLine("    {0}")
    .AppendLine("    {1}")
    .AppendLine("End Sub")
    .AppendLine()
    .AppendLine("'@ModuleCleanup")
    .AppendLine("Public Sub ModuleCleanup()")
    .AppendLine($"    '{RubberduckUI.UnitTest_NewModule_RunOnce}.")
    .AppendLine("    Set Assert = Nothing")
    .AppendLine("    Set Fakes = Nothing")
    .AppendLine("End Sub")
    .AppendLine()
    .ToString();  

which is easier to read. Now you will also notice that the Nothing in Set Fakes = Nothing is cased correctly.

For _methodInit this should be applied as well and you should decide if you want to use string interpolation $ or not. You used it for _moduleInit but you didn't for _methodInit. If you have choosen a style, you should stick to it.

\$\endgroup\$
  • \$\begingroup\$ Since I'm learning why is StringBuilder preferred? It does make it easier to read but is that the only reason as to why it's used? Casing was an oversight. Regarding the constructor, I think that's the correct term, for StringBuilder why provide the 256 number into it? \$\endgroup\$ – IvenBach Apr 13 '17 at 15:51
  • \$\begingroup\$ If you are using String.Concat() then StringBuilder isn't preffered but if you are concating strings by using + in a loop then each time you do this a new string object is created because strings are immutable. \$\endgroup\$ – Heslacher Apr 13 '17 at 15:58
2
\$\begingroup\$

Personally, I would use large string literals for your _moduleInit and _methodInit values, as they are more visually alike to their expected output:

    private readonly string _moduleInit =
$@"'@ModuleInitialize
Public Sub ModuleInitialize()
    '{RubberduckUI.UnitTest_NewModule_RunOnce}.
    {{0}}
    {{1}}
End Sub

'@ModuleCleanup
Public Sub ModuleCleanup()
    '{RubberduckUI.UnitTest_NewModule_RunOnce}.
    Set Assert = Nothing
    Set Fakes = nothing
End Sub

";

    private readonly string _methodInit =
$@"'@TestInitialize
Public Sub TestInitialize()
    '{RubberduckUI.UnitTest_NewModule_RunBeforeTest}.
End Sub


'@TestCleanup
Public Sub TestCleanup()
    '{RubberduckUI.UnitTest_NewModule_RunAfterTest}.
End Sub

";

The {{}} tells the interpolated string to output braces rather than attempt to perform a replacement, so it keeps the format placeholders intact, so that's no worry for the entire string being interpolated.

\$\endgroup\$
  • \$\begingroup\$ The thing I don't like about this, is that it wrecks the indentation. We use multiline verbatim strings in unit tests, but not in the actual RD code base. \$\endgroup\$ – Mathieu Guindon Apr 13 '17 at 18:49
  • \$\begingroup\$ Didn't know about doubling up the {{ }}, now I do. Is the double curly braces only for interpolated strings? Paralleling how one has to escape certain characters in a literal string. \$\endgroup\$ – IvenBach Apr 16 '17 at 0:43
  • \$\begingroup\$ @IvenBach it's a way to escape curly braces for any kind of string formatting. Since interpolated strings are really just syntactic sugar on top of string formatting, it works there as well. \$\endgroup\$ – Dan Lyons Apr 17 '17 at 15:11
1
\$\begingroup\$

There's one more alternative. Put the texts in *.txt files and compile them as Embeded Resource. Then your code is free of string concatenations. Define placeholders to insert custom values where necessary.

See How to read embedded resource text file

\$\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.