-2

This is my code so far:

public class ServerUtility
{
    public static int LogError(string source, string detail)
    {
        int iresult = -99;
        try
        {
            BaseRepository repo = new BaseRepository();
            using (SqlConnection con = new SqlConnection(repo.connectionString))
            {
                con.Open();
                using (SqlCommand cmd = new SqlCommand($"INSERT INTO [dbo].[LogError]([source],[detail],[date])VALUES('{source.Replace("'", "''")}','{detail.Replace("'", "''")}',GETDATE());SELECT @@IDENTITY", con))
                {
                    string getValue = cmd.ExecuteScalar().ToString();
                    iresult = Convert.ToInt32(getValue);
                } // command disposed here

            } //connection closed and disposed here
        }
        catch (Exception ex) { throw ex; }
        return iresult;
    }
}

My question is on GetInstance method :

BaseRepository repo = new BaseRepository();
using (SqlConnection con = new SqlConnection(repo.connectionString))

I am always set it as new object, on my static function, just for get constant value from AppSetting.

What actually happened if I implement this code to my project? How about the performance?

Is it cause performance issue?

Thanks

toha
  • 5,095
  • 4
  • 40
  • 52
  • 1
    Your implementation is wrong,you should create a new SenderBackupProvider instance only when oInstance is null – gtosto Dec 06 '17 at 03:35
  • 2
    And there's no point checking that the return from `new` is null - it never can be. I'd also say unless there's something odd about the way SenderBackupProvider works, it's not performance you need to worry about, but how it's designed to be used. – Dylan Nicholson Dec 06 '17 at 03:46
  • You can pick one of several ways to implement a singleton pattern in C# by looking at [Jon Skeet's "C# In Depth" relevant article](http://csharpindepth.com/Articles/General/Singleton.aspx) – Pac0 Dec 06 '17 at 08:37

1 Answers1

0

Like @gtosto mentioned, you're not implementing singletons correctly.

It should be:

    public static SenderBackupProvider GetInstance()
    {            
        if (oInstance == null)
        {
            oInstance = new SenderBackupProvider(CommFunction.GetLogNumber(CommonConst.APPID));
        }

        return oInstance;
    }

Check out Implementing the Singleton Pattern in C#.

EDIT:

Since OP doesn't believe that the oInstance variable should be null on first time run, here's a screenshot.

enter image description here

Upon a closer look at your code, you are using a web application. The life cycle of static variables differ from one platform to the other. You might need to do an app domain restart in IIS. See more information in Lifetime of ASP.NET Static Variable.

jegtugado
  • 5,081
  • 1
  • 12
  • 35
  • @DylanNicholson there are different ways of implementing singleton patterns and I gave the simplest one. Added a reference link for OP. – jegtugado Dec 06 '17 at 03:50
  • Yes, that is my way to create getInstance object, but when I debug the code, oInstance is never null. How can this happened? Then, I need to assign lognumber in every DB shot. I have change my code to be like this : – toha Dec 06 '17 at 04:08
  • public static SenderBackupProvider GetInstance() { if (oInstance == null) oInstance = new SenderBackupProvider(CommFunction.GetLogNumber(CommonConst.APPID)); else oInstance.LogNumber = CommFunction.GetLogNumber(CommonConst.APPID); if (oInstance == null) { return oInstance; } else { return oInstance; } } – toha Dec 06 '17 at 04:08
  • @toha it is null until the first call to `SenderBackupProvider.GetInstance()` because all succeeding requests will have an instance since the first one instantiated it. – jegtugado Dec 06 '17 at 06:07
  • I said, I have implement your code before, but oInstance is never null. I debug it, and it is not null even I call getInstance for first time. – toha Dec 06 '17 at 07:36
  • @toha how did you test it? did you place a breakpoint on `if (oInstance == null)` before launching the application and the first call to `SenderBackupProvider.GetInstance()`? Because if you did, then it should be null as you never instantiated `oInstance`. – jegtugado Dec 06 '17 at 07:43
  • I have put a breakpoint on if (oInstance == null) before launching the application and the first call to SenderBackupProvider.GetInstance(). And then oInstance is not null. But it is an new model with null property. But the instance is not null. That is why I change the code like that above on the main post. – toha Dec 06 '17 at 08:11
  • You could try restarting IIS. Check out https://stackoverflow.com/a/8919336/6138713 – jegtugado Dec 06 '17 at 08:26