2
\$\begingroup\$

Is this way acceptable to do a C# singleton for use in ASP.NET webforms legacy app:

public class FileSingleton
{
    private static readonly Lazy<FileSingleton> lazy =
        new Lazy<FileSingleton>(() => new FileSingleton( new FileRepository()));


    public static FileRepository Repository { get; private set; }

    public static FileSingleton Instance { get { return lazy.Value; } }

    private FileSingleton(FileRepository repository)
    {
        Repository = repository;
    }
}

I created a repository for dealing with files in Azure. It has some overhead, so I don't want to instantiate it on every request. My design is based off of this post.

protected void Application_Start(object sender, EventArgs e) 
{

    HttpContext.Current.Items.Add("fileRepository", FileSingleton.Instance);
}

Does that seem reasonable? Is there a better way to do this?

\$\endgroup\$
  • \$\begingroup\$ Have you considered Dependency Injection for Webforms? might help with some of the overhead. Is this hitting a DB or the file system? \$\endgroup\$ – DDiVita Aug 9 '17 at 20:50
  • 1
    \$\begingroup\$ This is hitting file storage in Azure. Sorry, I guess I should have given a bit more detail. I would love to use DI here but this legacy website is kind of a house of cards and I thought introducing something like Ninject or Unity would be 1) overkill, 2) not worth the time since this app may be replaced and 3) possibly introduce some unnecessary risk as this code is a bit of a giant mess. \$\endgroup\$ – Dan Csharpster Aug 10 '17 at 2:25
  • \$\begingroup\$ @DanCsharpster Do you remember what design you ended up using for this? I'm in the same boat with an old Web Forms app. \$\endgroup\$ – Eric Eskildsen Feb 7 at 20:27
1
\$\begingroup\$

First of all a bug, Repository property has to be an instance property, not static. Not that big issue because you're working with a singleton but it defeats the purpose of its surrounding code. Don't go to fix this, wait for our refactoring...

Class itself should be sealed, I don't see any chance to extend it (and you probably want to avoid it). Again, wait little bit more...

Note that, according to C# version you're using, you may write read-only properties as:

public FileRepository Repository { get; }    
public static FileSingleton Instance => lazy.Value;

About Application_Start. I do not see surrounding code then I'm guessing but I have few perplexities. HttpContext.Items is usually handy to share data between HTTP modules and/or HTTP handlers. In this case you're storing an instance of FileSingleton, it means that each user has to know that type. In this case they can directly access FileSingleton.Instance.


As last note I'd consider to merge FileSingleton and FileRepository (if you own that code and it can be changed). Consider that to access FileRepository methods you need to write something like this:

FileSingleton.Instance.Repository.DoSomething();

Isn't it too verbose? It should be:

FileSingleton.Repository.DoSomething();

Or:

FileRepository.Instance.DoSomething();

If possible I'd go with the last one to do not introduce another type with the only responsibility to keep an instance of something else. If not then I'd refactor your code to:

public static class FileSingleton
{
     private static readonly Lazy<FileRepository> _repository =
        new Lazy<FileRepository>(() => new FileRepository());

     public static FileRepository Instance => _repository.Value;
}
\$\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.