1
\$\begingroup\$

I have a WCF service that work with databse. The service should handle incoming message and store messagId into database. Please suggest me how can I improve it?

public class DBWorker
{
    static DBWorker instance;
    private string connectionString;
    private SqlConnection connection;

    protected DBWorker()
    {
        connectionString = GetConncetionString();
        connection = new SqlConnection(connectionString);
        connection.Open();
    }

    public static DBWorker Instance()
    {
        if (instance == null)
            instance = new DBWorker();                
        return instance;
    }

    private string GetConncetionString()
    {
        var conn = ConfigurationManager.ConnectionStrings["FsConnectionString"];
        if (conn == null | string.IsNullOrEmpty(conn.ConnectionString))
          throw new Exception("Couln't find connection string in app.config.");
        return conn.ConnectionString;
    }

    public void StoreMessage(string messageId)
    {
        SqlCommand cmd = new SqlCommand("insert into test (MessageId) values (@MessageId)", connection);
        cmd.Parameters.AddWithValue("@MessageId", messageId);
        cmd.ExecuteNonQuery();
    }

}

My WCF:

public class HostTest : IHostTest
{
    private DBWorker dbWorker;

    public HostTest()
    {
        dbWorker = DBWorker.Instance();
    }

    public Stream HandleIncomingMessage()
    {            
        var currentContext = WebOperationContext.Current;
        var request = currentContext.IncomingRequest;
        var response = currentContext.OutgoingResponse;

        string messageId = request.Headers["X-Message-Id"];

        response.StatusCode = HttpStatusCode.OK;
        response.ContentType = "text/plain";

        if (request.ContentLength == 0 )
        {
            response.StatusCode = HttpStatusCode.NoContent;
        }
        else
        {
            try
            {
                dbWorker.StoreMessage(messageId);
            }
            catch(Exception e)
            {
                response.StatusCode = HttpStatusCode.InternalServerError;
            }
        }

        return new MemoryStream(UTF8Encoding.Default.GetBytes(messageId)); ;
    }
}
\$\endgroup\$
  • \$\begingroup\$ You know this code doesn't compile? conn == null | string.IsNullOrEmpty(conn.ConnectionString) doesn't work. \$\endgroup\$ – IEatBagels Nov 13 '15 at 14:55
  • \$\begingroup\$ @TopinFrassi No, everything compile and works fine \$\endgroup\$ – Roman Marusyk Nov 13 '15 at 15:01
  • \$\begingroup\$ Hmm, your right! I didn't know you could do this. Sorry! :) \$\endgroup\$ – IEatBagels Nov 13 '15 at 15:03
  • \$\begingroup\$ @TopinFrassi No problem =).What do you think about the naming of class, methods? \$\endgroup\$ – Roman Marusyk Nov 13 '15 at 15:07
2
\$\begingroup\$

You keep your SqlConnection opened for the whole application lifecycle which is waayyy too long. BCdotWEB already covered the using part, which you should apply! :)

You should never throw Exception. That's mean. There is a ConfigurationException that exists for a good reason! When you want to throw an exception that's not related to your business logic, assume the framework already has an exception for this.

Your DbWorker might have concurrency issues. That's your call, but if you have a little load, it would be possible for two clients to initialize your singleton at the same time. To deal with that, you need to make the Singleton initialization thread-safe. We'll use the Lazy<> class.

public class DBWorker
{
    private static Lazy<DBWorker> instance = new Lazy<DBWorker>(() => new DbWorker(), LazyThreadSafetyMode.ExecutionAndPublication);

    protected DBWorker()
    {
        connectionString = GetConncetionString();
        connection = new SqlConnection(connectionString);
        connection.Open();
    }

    public static DBWorker Instance()
    {
         return instance.Value;
    }
}

The Lazy<> class will be executed once, in a thread-safe manner. And it'll then return the Value defined in the expression. If you use C#6, your Instance method could be a one-liner this way :

public static DBWorker Instance() => return instance.Value;

Here : if (conn == null | string.IsNullOrEmpty(conn.ConnectionString))

At first, I didn't think you could do this, but eh, you can! But I read on this, and there's a difference to consider between | and || operator. Read it here. Basically, using the || operator, if the first condition is true it won't compute the second condition, which is more performant. Using the | operator, each conditions are evaluated, and then we'll verify if it's true or false. There's a useless overhead! Meaning you should use || operator.

I don't think your constructor should be protected. After all, what's the use? You don't really want to do inheritance on a singleton class, it's just very weird. If you plan on using inheritance, consider adding an interface IDBWorker and implement the interface on different classes.

Your naming is a little off I think. DBWorker doesn't tell much except from the fact that it works with the Database. Its responsability is to store messages. So maybe MessageRepository (If you don't know the repository pattern, I recommend you read about it!) and HostTest is bad. Is it a test? I'd guess not. Ask yourself what is the responsability of your class and then you could name it easily! You can use this list to help you find good names!

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

SqlConnection, SqlCommand, etc. all implement IDisposable and thus they should be encapsulated in using blocks. Instead you tie SqlConnection to a singleton class, which is the opposite of that.

Here's how they should be used:

using (SqlConnection con = new SqlConnection(connectionString))
{
    con.Open();

    using (SqlCommand command = new SqlCommand("SELECT TOP 2 * FROM Dogs1", con))
    using (SqlDataReader reader = command.ExecuteReader())
    {
        while (reader.Read())
        {
            Console.WriteLine("{0} {1} {2}",
            reader.GetInt32(0), reader.GetString(1), reader.GetString(2));
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. I want to create one connect when the service started. How can I use using in that case? \$\endgroup\$ – Roman Marusyk Nov 13 '15 at 11:29
  • \$\begingroup\$ @MegaTron You shouldn't. Use a connection etc. when you need it and dispose of it as soon as possible. Please read stackoverflow.com/a/814606/648075 . \$\endgroup\$ – BCdotWEB Nov 13 '15 at 12:20
2
\$\begingroup\$

You have a lot of "magic if blocks":

if (instance == null)
    instance = new DBWorker();

Best practice is to always enclose if-then statements in braces:

if (instance == null)
{
    instance = new DBWorker();
}

That way if you ever need to add additional code (e.g. instance.init()), you'll be sure to have it in the right scope.

Another point is that your code isn't very testable; see Make WCF Service testable for some hints on that front.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 for mentioning testability. Making that all testable will ease life of OP. \$\endgroup\$ – kayess Nov 13 '15 at 18:42

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.