Is there any logical problems in my code, anything required to optimize?

Question

Is there any logical problems in my code, anything required to optimize?

In this code I am fetching value from registry. Here I created one property. I am fetching that property in different file. Do we need set accessor here though? The value is fixed in the registry.

public class Agent
{
    public string version
    {
        get { return m_version; }
        set { m_version = value; }
    }

    private string m_version = null;

    // constructor
    public Agent()
    {
        string keySpoPath = SpecialRegistry.SpecialAgentRoot;
        RegistryKey regkey = Registry.LocalMachine.OpenSubKey(keySpoPath);
        m_version = 
            (string)regkey.GetValue(SpecialRegistry.regValue_CurrentVersion);
    }
}

Answer 1

Unless you wish to change the value from an external class and later update the Registry, NO, you dont need a setter.

Answer 2

No. If you do not want the value to be updated from the other file, just provide the get accessor.

Answer 3

The question is does the value change and do you need to save it back to the registry? Or is the value from registry always correct and never updated?

For the first instance:

The private backing field:

    private static HorizontalAlignment? _Alignment;

The property:

    public static HorizontalAlignment Alignment
    {
        get
        {
            if (_Alignment == null)
            {
                _Alignment = GetAlignment();
            }

            return _Alignment.Value;
        }
        set
        {
            if (_Alignment != value && SetAlignment(value))
            {
                _Alignment = value;
                OnAlignmentChanged(new AlignmentChangedEventArgs(value));
            }
        }
    }

The "Get" method:

    private static HorizontalAlignment GetAlignment()
    {
        HorizontalAlignment alignmentValue = DEFAULT_ALIGNMENT;

        using (RegistryKey registryKey = Registry.LocalMachine.CreateSubKey(REGISTRYKEY))
        {
            if (registryKey != null)
            {
                string tempAlignment = registryKey.GetValue(ALIGNMENT_KEYNAME, string.Empty).ToString();

                if (!string.IsNullOrEmpty(tempAlignment))
                {
                    try
                    {
                        alignmentValue = (HorizontalAlignment)Enum.Parse(typeof(HorizontalAlignment), tempAlignment, false);
                    }
                    catch (Exception exception)
                    {
                        alignmentValue = DEFAULT_ALIGNMENT;
                        Logging.LogException(exception);
                    }
                }
            }
        }

        return alignmentValue;
    }

The "Set" method:

    private static bool SetAlignment(HorizontalAlignment value)
    {
        bool flag = true;

        using (RegistryKey registryKey = Registry.LocalMachine.CreateSubKey(REGISTRYKEY))
        {
            if (registryKey != null)
            {
                try
                {
                    registryKey.SetValue(ALIGNMENT_KEYNAME, value.ToString(), RegistryValueKind.String);
                }
                catch (Exception exception)
                {
                    Logging.LogException(exception);
                    flag = false;
                }
            }
        }

        return flag;
    }

If your question is "Is it required to implement a Set accessor?" then the answer is no. The following are also valid.

public int MyInt { get { return 1; } }

public int MyInt { get; protected set; }

Answer 4

You could pull out keySpoPath and make it static, or readonly as it appears to be constant for each call to agent. For that matter regkey can be pulled out and done just once as well. I would leave the get in place in case it is changing, otherwise it can be done statically as well.

Answer 5

Dispose RegistryKey after usage. Don't cache unless you need it. Don't afraid to hardcode registry key names - they will most probably be only used in one place.

public class Agent
{
    public string Version
    {
        get
        {
            using (var regkey = Registry.LocalMachine.OpenSubKey(SpecialRegistry.SpecialAgentRoot))
                return (string)regkey.GetValue("CurrentVersion");
        }
    }
}