2
\$\begingroup\$

Is it a bad practice to use getters and setters this way?

Below is my code:

public struct Settings
{
    public static int workMinutes
    {
        get
        {
            return Properties.Settings.Default.workMinutes;
        }
        set
        {
            Properties.Settings.Default.workMinutes = value;
            save();
        }
    }

    public static int restMinutes
    {
        get
        {
            return Properties.Settings.Default.restMinutes;
        }
        set
        {
            Properties.Settings.Default.restMinutes = value;
            save();
        }
    }

    public static void init()
    {
        if(Properties.Settings.Default.workMinutes == 0)
            Properties.Settings.Default.workMinutes = 5;

        if(Properties.Settings.Default.restMinutes == 0)
            Properties.Settings.Default.restMinutes = 5;

        save();
    }

    private static void save()
    {
        Properties.Settings.Default.Save();
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review. I know you are eager to have your code reviewed. In CR, we expect a summary of what your code does ? \$\endgroup\$ – Siobhan Oct 23 '16 at 20:36
  • 1
    \$\begingroup\$ Making this a struct makes little sense, a value type is a value, not a façade for some static object; there is no immutable data to encapsulate here - IMO this should be a class, if it needs to exist at all. \$\endgroup\$ – Mathieu Guindon Oct 23 '16 at 21:16
3
\$\begingroup\$

I don't think it's a bad practice at all. What you did is called encapsulation. It enables you to exchange the underlying configuration provider (currently the Properties.Settings) to something else without breaking your application because it doesn't rely directly on it.

It's also called the Dependency Inversion Principle the higher module (your application) relies on an abstraction (your Settings) and not on the low level module (Properties.Settings). Usually it's build upon abstract types (or interfaces) but a lite version of this could be your settings layer.

\$\endgroup\$
1
\$\begingroup\$

I don't see what you've done here as 'bad practice'. But I can suggest some improvements. My comments are below each snippet of your code:

public static int workMinutes
{
    get
    {
        return Properties.Settings.Default.workMinutes;
    }
    set
    {
        Properties.Settings.Default.workMinutes = value;
        save();
    }
}

Can the wrapped code possibly throw? If yes, can you include code that guards against those exceptions from throwing? If no, please document your getter/setter with the list of exceptions that could possibly throw, having a brief explanation as to what conditions can cause the exceptions.

public static void init()
{
    if(Properties.Settings.Default.workMinutes == 0)
        Properties.Settings.Default.workMinutes = 5;

    if(Properties.Settings.Default.restMinutes == 0)
        Properties.Settings.Default.restMinutes = 5;

    save();
}

You've created properties, but then in client code (the init method), you don't use the properties, but rather you directly access the thing being wrapped. I suggest that you use your properties in all client code.

private static void save()
{
    Properties.Settings.Default.Save();
}

I see no good reason to wrap the call to Properties.Settings.Default.Save inside of this static method. Your wrapper is private, and is consumed only by members of the same struct. I suggest to remove your custom save method, and making direct calls to Properties.Settings.Default.Save within the other two struct members.

Lemme know what you think about my critique.

\$\endgroup\$
1
\$\begingroup\$

Naming Conventions

  • Avoid java naming conventions void save. Use C# conventions instead void Save.
  • Favor verbs Initialise over abbreviations Init for method names.

Type Conventions

  • Don't use struct for mutable objects. Use class instead.

Design Considerations

Others have already pointed out Settings could be used as a facade for your application, splitting the interface from the internal implementation. Ideally, you would want to create an interface for your settings providers and have one implementation that uses the auto-generated Properties.Settings class. For the sake of limiting our scope, let's work directly on the auto-generated settings.

 public class Settings
    {
        private Properties.Settings settings;

        Settings(Properties.Settings settings)
        {
            this.settings = settings ?? throw new ArgumentNullException(nameof(settings));
        }

        // ..
    }

We could then write cleaner code with much less redudancy.

        public int WorkMinutes
        {
            get => settings.WorkMinutes;
            set
            {
                settings.WorkMinutes = value;
                Save();
            }
        }

        public int RestMinutes
        {
            get => settings.RestMinutes;
            set
            {
                settings.RestMinutes = value;
                Save();
            }
        }

You no longer have a static class, but perhaps you would like a shared instance available throughout your application. Let's forsee a shared instance.

private static Settings shared;
public static Settings Default => shared ?? (shared = new Settings(Properties.Settings.Default));

Perhaps we could auto-initialise our settings in the constructor.

 Settings(Properties.Settings settings)
        {
            this.settings = settings ?? throw new ArgumentNullException(nameof(settings));
            Initialise();
        }

Extended Design

In a real application, it's likely Settings would end containing dozens of seperate settings properties. Perhaps the settings would also be editable at runtime. In addition, Initialise would have to call Save on each individual property. We could use a batch update on demand to optimize our class.

Initialise would look like this:

 public void Initialise()
 {
     using (var batch = new Batch(this))
     {
         if (WorkMinutes == 0)
         {
             WorkMinutes = 5;
         }

         if (RestMinutes == 0)
         {
             RestMinutes = 5;
         }

         // other settings ..

     } // <- this calls Save() once
 }

The pattern can be implemented as follows:

        private void Save()
        {
            if (!deferred)
            {
                settings.Save();
            }
        }

        private bool deferred;

        public IDisposable StartBatch()
        {
            return new Batch(this);
        }

        private class Batch : IDisposable
        {
            Settings source;

            public Batch(Settings source)
            {
                Debug.Assert(source != null);
                this.source = source;
                this.source.deferred = true;
            }

            public void Dispose()
            {
                source.deferred = false;
                source.Save();
            }
        }
\$\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.