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.

Let's compare the following code:

1)

public class MyClass
{
    private object syncRoot;

    public MyClass()
    {
        this.syncRoot = new object();
    }

    ...
}

2)

public class MyClass
{
    private object syncRoot = new object();

    public MyClass()
    {
    }

    ...
}

What do you prefer and what are the advantages/disadvantages of these solutions? To me I prefer the first one because its obvious seeing constructor what is getting intialized, however the second can be covering multiple constructors better.

Edit: I am pasting actual real code to make the question valid, adding some description: The class handles execution of powershell, there are some dependencies of class injected to constructor, and some fields initialized locally (syncRoot is used for thread-safety, logger is log4net for logging).

/// <summary>
/// VCenter handler which creates the tasks and execute them
/// requirement: PowerCLI must be installed
/// </summary>
public class VCenterPowerCLI : IDisposable
{
    /// <summary>log4net logger</summary>
    private readonly ILog logger = LogManager.GetLogger(typeof(VCenterPowerCLI));

    /// <summary>factory of powershell runtimes</summary>
    private readonly IPowershellRuntimeFactory powershellRuntimeFactory;

    /// <summary>vcenter handler configuration</summary>
    private readonly VCenterConfiguration configuration;

    /// <summary>Thread-safe sync</summary>
    private object syncRoot = new object();

    /// <summary>
    /// Initializes a new instance of the VCenterPowerCLI class.
    /// </summary>
    /// <param name="configuration">vcenter handler configuration</param>
    /// <param name="powershellRuntimeFactory">factory of powershell runtimes</param>
    public VCenterPowerCLI(
        VCenterConfiguration configuration,
        IPowershellRuntimeFactory powershellRuntimeFactory)
    {
        this.configuration = configuration;
        this.powershellRuntimeFactory = powershellRuntimeFactory;
    }

    /// <summary>
    /// Immediate task - takes snapshot under the machine 
    /// </summary>
    /// <param name="machineId">machine id</param>
    /// <param name="snapshotName">snapshot name</param>
    public void TakeSnapshot(string machineId, string snapshotName)
    {
        string task = this.CreateTakeSnapshotTask(machineId, snapshotName);
        this.ExecuteTask(task);
    }

    /// <summary>
    /// Creates take snapshot task to be executed in the pipeline later
    /// </summary>
    /// <param name="machineId">machine id</param>
    /// <param name="snapshotName">snapshot name</param>
    /// <returns>task to be executed</returns>
    public string CreateTakeSnapshotTask(string machineId, string snapshotName)
    {
        StringBuilder code = new StringBuilder();
        code.AppendLine("Add-PSSnapin \"VMware.VimAutomation.Core\" | Out-Null;");
        code.AppendLine(string.Format("$global:Server = Connect-VIServer \"{0}\" -User \"{1}\" -Password \"{2}\"", this.configuration.Uri, this.configuration.User, this.configuration.Password));
        code.AppendLine(string.Format("$cluster = Get-Cluster \"{0}\";", this.configuration.ClusterName));
        code.AppendLine(string.Format("$vm = $cluster | Get-VM \"{0}\";", machineId));

        code.AppendLine(string.Format("New-Snapshot -VM $vm -Name \"{0}\" -Quiesce -Memory | Out-Null", snapshotName));
        code.AppendLine("Disconnect-VIServer -Server $global:Server -Confirm:$false");

        return code.ToString();
    }

    /// <summary>
    /// Executes the task - wait for finishing
    /// </summary>
    /// <param name="code">task to be executed</param>
    public void ExecuteTask(string code)
    {
        lock (this.syncRoot)
        {
            using (IPowershellRuntime runtime = this.powershellRuntimeFactory.Create())
            {
                try
                {
                    runtime.ExecuteCode(code);
                }
                catch (Exception e)
                {
                    if (e.Message.Contains("Unsufficient permissions. You need 'System.Read' privileges to complete the requested operation"))
                    {
                        // skip, the operation with this exception is successfully done
                    }
                    else
                    {
                        throw;
                    }
                }
            }
        }
    }
}
share|improve this question
    
That logger is not consistent with the other fields. Even if you don't inject it (which is a mistake!), since the others are initialized in the constructor, it should also be. syncRoot isn't as bad, because it has no data of any kind, but I'd still move it for consistency. –  Magus Apr 11 at 18:53
    
Magus: Consistency is good point as mentioned by Jeroen, and as long as there are dependencies it seems that only way how to make it all consistent is to initialize all the locals in constructor as well –  Tomas Panik Apr 11 at 19:12
1  
@TomasPanik Thanks for adding some real code. I have voted to reopen the question and I've also asked folks in the chat room to do so as well. Once again, welcome to Code Review. –  Simon André Forsberg Apr 11 at 19:36
    
syncRoot should be set to readonly. Bad things happen when the variable you are locking on starts referencing a new instance. –  unholysampler Apr 11 at 19:48
add comment

2 Answers 2

up vote 8 down vote accepted

There are no differences between your two options. There are a few remarks we can make about though:

Consistency

Keep everything consistent. There are a few reasons to change it up (for example: you can do inline initialization all the time and only initialize those in the constructor that take a constructor argument), but that's because these two situations have a significant difference.

If there is no difference (for example: initializing a field to its default value) then you have to be consistent.

Multiple constructors

If you initialize your fields in your constructor and you add a second one, you might forget to chain them.


All things considered, I believe inline initializing to be better. There is no functional difference but it doesn't have the possibility of the constructor chaining overlook.

Aside from that, it also keeps the declaration and the initialization together which improves readability.

Comments

When I look at a variable called IPowershellRuntimeFactory, a comment that says factory of powershell runtimes doesn't add any value.

Comments like this are a lot more important:

/// VCenter handler which creates the tasks and execute them
/// requirement: PowerCLI must be installed

It is hard to deduct that PowerCLI must be installed if you omit it, so that's actually adding valuable information.

share|improve this answer
add comment

I would not use any of them. Use inversion of control and constructor injection instead:

public class MyClass
{
    private readonly object syncRoot;

    public MyClass(object syncRoot)
    {
        this.syncRoot = syncRoot;
    }

    ...

}

And once you understood that IoC and DI are good practices to follow, Nikola Malovic's 4th law of IoC becomes clear:

Every constructor of a class being resolved should not have any implementation other then accepting a set of its own dependencies.

In other words, do not do anything substantial in constructor. Creating an object is substantial.

UPDATE for guys fighting with locks:

public class MyClass
{
    private readonly object syncRoot;

    /// <param name="syncRoot">
    ///     Just instantiated object, please
    ///     
    ///     And no locks, offcourse!
    /// </param>
    public MyClass(object syncRoot)
    {
        this.syncRoot = syncRoot;
    }
}

public class MyClassFactory
{
    MyClass Create()
    {
        return new MyClass(new object());
    }
}
share|improve this answer
5  
This may be a slightly too strong point. I'd consider things like creating an empty list to be safe (and rather pointless to inject). If the object has some initial value though, sure. If it's really just a new object, it's messier to inject it. Nothing outside needs to know. –  Magus Apr 11 at 18:50
5  
That unnecessarily exposes a private variable to meddling from others. For example, some other code could then take a lock on syncRoot. –  200_success Apr 11 at 19:41
    
@Magus Sometimes it's very useful to write own lists with own interfaces; at least you can have own interface, and wrap an usual list to think about it later. Passing correct arguments to the constructor is factories' job; empty object can be perfect correct argument, look at it's interface - it provides some useful values. –  astef Apr 11 at 20:31
2  
syncRoot does not look like a dependency, therefore I would not use dependency injection on it. –  Matthew Apr 11 at 20:48
1  
Or you just new the syncLock object in the ctor and not waste hours of time debugging based on some unthinking religious belief, leaving you with one less problem. –  Andy Apr 11 at 21:49
show 8 more comments

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.