Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

We are using the Aspose PDF library to work with PDF files. Aspose requires the license to be set once per process. Accordingly, I have created the following common static method that sets the license. Any code that needs Aspose will call into this method.

public class ConfigurationService
{
    private static bool isAsposePdfLicenseSet = false;

    public static void SetAsposePdfLicense()
    {
        if (isAsposePdfLicenseSet)
        {
            return;
        }

        // Set the license... (Moderately expensive - accesses the file system)

        isAsposePdfLicenseSet = true;
    }
}

I need to make sure the license is set before using my class, PdfInspector, but I only need to set it once, so I gave that class a static constructor:

static PdfInspector()
{
    ConfigurationService.SetAsposePdfLicense();
}

However, when I run Code Analysis in Visual Studio, it tells me that static constructors should be avoided. The alternative is to set the license in the constructor:

internal PdfInspector()
{
    ConfigurationService.SetAsposePdfLicense();
}

While this would be called every time the class is instantiated, the SetAsposePdfLicense would short circuit before doing any real logic. Bottom line, which implementation is preferable or is there an alternative that I haven't considered?

share|improve this question

2 Answers 2

up vote 6 down vote accepted

This is what made me want to answer:

// Set the license... (Moderately expensive - accesses the file system)

You do not want any constructor, static or not, to do work like this. A constructor that accesses the file system is screaming "I CAN BLOW UP ANYTIME!", and can throw file system -related exceptions. You don't want that in a constructor, even less a static one:

A static constructor is used to initialize any static data, or to perform a particular action that needs to be performed once only. It is called automatically before the first instance is created or any static members are referenced.

http://msdn.microsoft.com/en-us/library/k9x6w0hc.aspx

Making your field static ensures that the type (not the instance) has the field value, and your logic ensures it's set only once in the lifetime of your application. If your application isn't starting any process (requirement is once per process, right?), it can probably work.

I don't like that you're assuming that your application will only ever call this method in the same process though - it's a fair assumption to make, but since the requirements say once per process, I think your code should account for System.Diagnostics.Process.GetCurrentProcess().Id, and call the SetAsposePdfLicense() method if the method wasn't called in the process your code is running in.

I'm thinking of a factory living in a Singleton scope (or just as a plain Singleton), that would maintain an ICollection<int> (a HashSet<int> would be ideal for this) where each item is a process ID for which SetAsposePdfLicense was called.

public class PdfInspectorFactory
{
    private readonly ICollection<int> _processes = new HashSet<int>();

    private void SetLicenseForCurrentProcess()
    {
        var currentProcessId = System.Diagnostics.Process.GetCurrentProcess().Id;
        if (_processes.Contains(currentProcessId))
        {
            return;
        }

        // set the license...

        _processes.Add(currentProcessId);
    }

    public PdfInspector Create()
    {
        SetLicenseForCurrentProcess();
        var inspector = new PdfInspector();

        // do whatever you need to do with your inspector before returning it

        return inspector;
    }
}

Then your client code can use this PdfInspectorFactory and call its Create method to get a PdfInspector without having to care about whether or not the license is set.

The client code will not call the factory method in its constructor, rather only when it needs it.

public class SomeClass
{
    private readonly PdfInspectorFactory _factory;

    public SomeClass(PdfInspectorFactory factory)
    {
        _factory = factory;
    }

    public void DoSomething()
    {
        try
        {
            var inspector = _factory.Create();
            // here you go, now do what you need to do with your inspector...

        }
        catch(Exception exception)
        {
            // todo: log or otherwise report exception
            throw;
        }
    }
}

The factory instance should only exist as a Singleton instance - if you're using an IoC container that is very easy to accomplish. Otherwise you need to implement the factory class itself, as a Singleton.

share|improve this answer
    
An ISet or an IList? Set or List? Set or List? <s>Million</s> 10 <s>dollar</s> reputation question. –  Simon André Forsberg Jun 17 at 15:06
    
@SimonAndréForsberg You're right, since the order of elements doesn't matter it's best to use an ISet<int> - although any ICollection<int> can work. HashSet<int> would probably be the most efficient implementation, edited. Thanks! –  Mat's Mug Jun 17 at 15:42
    
Wow, thanks. A lot to digest. I had a feeling that the correct answer was "neither". I've recently been learning about (and beginning to use) the factory and IoC patterns so this helps me to better understand their use. I could use some clarification on the distinction between a process and an application and static variable scope therein. Can an application consist of more than one process? –  probackpacker Jun 17 at 16:01
    
Yes. IIRC your program runs in a process that the .NET runtime puts in an app domain; your code is running on a thread that belongs to the process, and you can spawn new threads and new processes as you want - e.g. you may want to launch a command-line tool on a separate process, and do that on a background thread - everything still happens under the same app domain (you can also create a separate app domain with different privileges if you need to). But that whole part of my .NET knowledge is a bit rusty, you'll want to double-check all of it ;) –  Mat's Mug Jun 17 at 16:07
1  
@Mat'sMug, I was curious about the process/ApplicationDomain/static member scope interaction, so I asked this question on Stack Overflow. The conclusion was that, without significant code gymnastics, static members are isolated to a single process. Thanks again for your help! –  probackpacker Jun 18 at 17:08

I think you can use a Nullable<bool> or bool? :

private static bool? isAsposePdfLicenseSet = null;
private static bool IsAsposePdfLicenseSet
{
    get
    {
        if (isAsposePdfLicenseSet == null)
            ConfigurationService.SetAsposePdfLicense();

        return isAsposePdfLicenseSet.Value;
    }
}

The validation logic will be executed as soon as you start using IsAsposePdfLicenseSet. Assuming SetAsposePdfLicense will set the value for the backing field, it will only be initialized once.

share|improve this answer
    
I'm sorry, but I'm not sure what this buys me. I realize that my code in the question may not exactly show what I'm trying to do. I've edited the question for better clarity about how the code is written. –  probackpacker Jun 17 at 14:29
    
A Nullable boolean would not help at all. What purpose would false have in your case? The initial code has only two values: True and false. There's no need to add a third state. –  Simon André Forsberg Jun 17 at 14:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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