General static value required by multiple classes

Question

I have a general value called Seed which is required to multiple class. But those class aren't dependent of the class which declare it.

class Game {
    class Settings {
       public static float Seed = 1337;
    }
}

And some class need a Seed value to work. I take the class Noise as example here.

class Noise {
    private static float _seed;
}

A solution would be to use Game.Settings.Seed to know the value into the class Noise. But what if one day I decide to take all the Noise class to use it in an other project. The code won't compile because there is probably not a Game.Settings.Seed declared in the other project obviously.

A other way would be to set as public field in the Noise class.

class Noise {
    public static float Seed;
}

So in this way, Game which know he need to use Noise class, will set Seed field before doing anything with this class.

But I don't like it too, in this case this is about a float, so that not really a problem, but what about an object that need to be declared, which means Noise class need to implement a verification about itself.

class Noise {

    private static float _seed;
    private static bool _isSeedDeclared;

    public static void SetSeed(int seed){
        _seed = seed;
        _isSeedDeclared = true;
    }

    public int GetValue(int x, int z){
        CheckSettings();
        return computedValue; //to calculate computedValue, I need Seed value
    }

    private void CheckSettings(){
        if(!_isSeedDeclared)
            throw new Exception("You have to define Seed value.");
    }
}

This solution works but I'm not pleased about it. For non-nullable type I have to define an other variable to check, for a nullable class I can compare it to null. But anyway, I don't have only one parameter and I don't have only one class. I'm looking for an better way to implement it.

Answer 1

Okay, so there's lots going on in your question. Let's try and break it down.

But what if one day I decide to take all the Noise class to use it in an other project. The code won't compile because there is probably not a Game.Settings.Seed declared in the other project obviously.

What you're talking about here is decoupling. There are many different types of coupling in software design. You're on the right track, decoupling your classes from each other is a very good thing to do for many reasons, including the one you've described.

In this specific example we're trying to decouple the Noise class from the Game.Settings class.

You've really only got 2 options here. Constructor injection or Property injection. Both options are valid but they have pros and cons depending on your requirements. Any other option is either just a variation of these two (e.g. field injection, service or function injection) or won't really decouple things.

So in this way, Game which know he need to use Noise class, will set Seed field before doing anything with this class.

One of the pros of using constructor injection is to enforce that the Game class sets the seed on the Noise class before it can be used.

_noise = new Noise(Game.Settings.Seed);

A other way would be to set as public field in the Noise class.

This is a form of property injection. The downside is that you can only enforce things by using exceptions at runtime. In some cases it makes sense, but from what I've seen I don't think this is your best choice.

For non-nullable type I have to define an other variable to check, for a nullable class I can compare it to null.

Actually, C# has nullable types for exactly this situation. So you could convert this code:

private static float _seed;
private static bool _isSeedDeclared;

into this:

private static float? _seed;

and when you want to use it you can do something like this:

if(!_seed.HasValue)
    throw new Exception("You have to define Seed value.");

But anyway, I don't have only one parameter and I don't have only one class. I'm looking for an better way to implement it.

I agree, your question is a little more broad than this one class. It's more about the overall design of the system as a whole. The truth is there's no one size fits all answer, it depends.

I believe part of the problem is the overuse of static. It's not really clear why the Noise class needs to use static at all. In particular, you're likely to run into all sorts of issues with the use of global static state as I suspect is the case.

I'd seriously consider removing all the staticness from any classes that don't need it. Especially if they're also holding shared data. As usual, it depends, but you might be surprised how much simpler things become when you let go of that idea.

Answer 2

If the seed parameter is required for the Noise class to work properly, it must go to the constructor as a required parameter. If an invalid seed passed to the constructor, it should throw an ArgumentException explaining what's wrong with the seed and (ideally) what the expected value could look like (e.g. if there are some range rules take place in your domain).

Consumers of the Noise class will have to inject the proper value (likely taking it from the settings).

Answer 3

You're doing Settings all wrong.

I know you don't believe me, but you are. I've been working on a (very) long-term project (3+ years) with a 'settings' type system, and my solution was very bulky, but creates a lot less work later.

I can't even tell where you plan on using Seed in the Settings class, what does it act as a seed for? Everything? You need to group things into categories so that you can create a separation of responsibilities (SRP), and keep things that are related together.

The first thing I did is create a BaseConfiguration class that has the common settings systems in it:

[DataContract]
public class BaseConfiguration
{
    [DataMember]
    public NetworkConfiguration NetworkConfiguration { get; set; }

    [DataMember]
    public LoggingConfiguration LoggingConfiguration { get; set; }

    [XmlIgnore]
    [ScriptIgnore]
    [IgnoreDataMember]
    public static string ConfigurationFile { get; set; } = "Config.xml";

    public void Save<T>()
    {
        Save<T>(this);
    }

    public static void Save<T>(BaseConfiguration config)
    {
        DataContractSerializer dcs = new DataContractSerializer(typeof(T));

        using (FileStream fs = new FileStream(ConfigurationFile, FileMode.Create))
        using (XmlWriter writer = XmlDictionaryWriter.CreateTextWriter(fs))
        {
            dcs.WriteObject(writer, config);
        }
    }

    public static T Load<T>()
    {
        DataContractSerializer dcs = new DataContractSerializer(typeof(T));

        using (FileStream fs = new FileStream(ConfigurationFile, FileMode.OpenOrCreate))
        using (XmlDictionaryReader reader = XmlDictionaryReader.CreateTextReader(fs, new XmlDictionaryReaderQuotas()))
        {
            return (T)dcs.ReadObject(reader);
        }
    }
}

Then, there are two classes that derive from this, a ClientConfiguration and a ServerConfiguration:

[DataContract]
public class ClientConfiguration : BaseConfiguration
{
    [DataMember]
    public GraphicsConfiguration GraphicsConfiguration { get; set; }
}

[DataContract]
public class ServerConfiguration : BaseConfiguration
{
    [DataMember]
    public WorldConfiguration WorldConfiguration { get; set; }

    [DataMember]
    public SqlConfiguration SqlConfiguration { get; set; }
}

Then you have the actual settings in each Configuration type:

[DataContract]
public class WorldConfiguration
{
    /// <summary>
    /// The number of <see cref="Chunk"/>s wide that the <see cref="World"/> is.
    /// </summary>
    [DataMember]
    public ushort WorldChunkWidth { get; set; }

    /// <summary>
    /// The number of <see cref="Chunk"/>s high that the <see cref="World"/> is.
    /// </summary>
    [DataMember]
    public ushort WorldChunkHeight { get; set; }

    /// <summary>
    /// The total number of <see cref="Chunk"/>s in the <see cref="World"/>.
    /// </summary>
    [DataMember]
    public uint WorldChunkSize { get; set; }

    /// <summary>
    /// The number of <see cref="Tiles"/> wide each <see cref="Chunk"/> is.
    /// </summary>
    [DataMember]
    public ushort ChunkWidth { get; set; }

    /// <summary>
    /// The number of <see cref="Tiles"/> high each <see cref="Chunk"/> is.
    /// </summary>
    [DataMember]
    public ushort ChunkHeight { get; set; }

    /// <summary>
    /// The number of pixels wide each of the <see cref="Tiles"/>"/> are.
    /// </summary>
    [DataMember]
    public byte TileWidth { get; set; }

    /// <summary>
    /// The number of pixels high each of the <see cref="Tiles"/>"/> are.
    /// </summary>
    [DataMember]
    public byte TileHeight { get; set; }

    [DataMember]
    public string ChunkFolder { get; set; }

    [DataMember]
    public string PlayerFolder { get; set; }

    [DataMember]
    public string PlayerTable { get; set; }

    [XmlIgnore]
    [ScriptIgnore]
    [IgnoreDataMember]
    public Rectangle TileBounds => new Rectangle(0, 0, WorldChunkWidth * TileBounds.Width, WorldChunkHeight * TileBounds.Height);

    [XmlIgnore]
    [ScriptIgnore]
    [IgnoreDataMember]
    public Rectangle PixelBounds => new Rectangle(0, 0, WorldChunkWidth * PixelBounds.Width, WorldChunkHeight * PixelBounds.Height);

    public WorldConfiguration()
    {
        WorldChunkWidth = 256;
        WorldChunkHeight = 256;
        ChunkWidth = 256;
        ChunkHeight = 256;
        TileWidth = 32;
        TileHeight = 32;

        ChunkFolder = @"World\Chunks";
        PlayerFolder = @"World\Players";
        PlayerTable = @"dbo.Players";
    }
}

[DataContract]
public class LoggingConfiguration
{
    [DataMember]
    public string Folder { get; set; }

    [DataMember]
    public int MaxFileSize { get; set; }
}

[DataContract]
public class GraphicsConfiguration
{
    [DataMember]
    public bool VSync { get; set; } = true;

    [DataMember]
    public bool IsFixedTimeStep { get; set; } = false;

    [DataMember]
    public bool FullScreen { get; set; } = false;

    [DataMember]
    public byte[] ResolutionBytes
    {
        get { return Resolution.GetBytes(); }
        set
        {
            var sz = new Size();
            sz.FromBytes(value);
            Resolution = sz;
        }
    }

    [XmlIgnore]
    [ScriptIgnore]
    [IgnoreDataMember]
    public Size Resolution { get; set; }
}

Etc. I won't go into detail on all of them, but you get the picture. Do note that Resolution is a custom Size type I built myself, not the built-in .NET ones, you can cut that and ResolutionBytes out if you want to try to use this and build your own alternative.

Now eventually we come to the point where we want to use the Configuration types:

BaseConfiguration.ConfigurationFile = @"Config\Config.xml";
ClientConfiguration clientConfiguration = new ClientConfiguration();
clientConfiguration.LoggingConfiguration = new LoggingConfiguration();
clientConfiguration.LoggingConfiguration.Folder = "Log";
clientConfiguration.LoggingConfiguration.MaxFileSize = 16 * 1024 * 1024;
clientConfiguration.NetworkConfiguration = new NetworkConfiguration();
clientConfiguration.NetworkConfiguration.PeerString = "Ancients";
clientConfiguration.NetworkConfiguration.Port = 8093;
clientConfiguration.NetworkConfiguration.IP = "127.0.0.1";
clientConfiguration.GraphicsConfiguration = new GraphicsConfiguration();
clientConfiguration.GraphicsConfiguration.IsFixedTimeStep = false;
clientConfiguration.GraphicsConfiguration.VSync = true;
clientConfiguration.GraphicsConfiguration.FullScreen = false;
clientConfiguration.GraphicsConfiguration.Resolution = new Size(1024, 768);
BaseConfiguration.Save<ClientConfiguration>(clientConfiguration);

(This bit is ugly, but it's for testing purposes only.)

Then to load it:

var config = BaseConfiguration.Load<ClientConfiguration>();

Then finally you pass config or config.LoggingConfiguration, config.GraphicsConfiguration, etc. around as you desire.

---

Finally, we come to Noise:

Why is everything in this static? What happens if I want two Noise variations with different seeds? You cannot do that with your implementation.

Only mark things static that are universal states. That is, something that there is absolutely no reason to want to have different states for.

In your case, Noise does not meet that criteria.

class Noise
{
    private float _seed;

    public Noise(float seed)
    {
        _seed = seed;
    }

    public int GetValue(int x, int z)
    {
        return computedValue; //to calculate computedValue, I need Seed value
    }
}

Even better: make an interface so you can have different types of noise:

public interface INoise
{
    int GetValue(int x, int z);
}

Then implement it:

public class PerlinNoise
{
    private float _seed;

    public PerlinNoise(float seed)
    {
        _seed = seed;
    }

    public int GetValue(int x, int z)
    {
        return calculatedValue;
    }
}

Then any object/method that needs a Noise can just take an INoise, which means you can supply different noise-types if you need to. (I do this very frequently with logging.)

Answer 4

I'm right behind Sergey Shushlyapin's answer of making a constructor with required parameters. In your code, this would mean making seed a required parameter for the Noise class constructor.

To help illustrate this answer, I wanted to contribute by providing you with an update of your own code:

class Noise {

    private static float _seed;

    public Noise(float seed)
    {
        UpdateSeed(seed);
    }

    public void UpdateSeed(float seed){
        if(seed < 0) throw new ArgumentOutOfRangeException(nameof(seed), "cannot be a negative value");

        _seed = seed;
    }

    public int GetValue(int x, int z){
        CheckSettings();
        return computedValue; //to calculate computedValue, I need Seed value
    }
}

This would be used, like so

Noise noise = new Noise(0);   // constructor
noise.UpdateSeed(10);         // UpdateSeed method

---

Use Properties

However, as an improvement upon the code above, I would lean more towards this approach of using properties. As using your existing code doesn't provide way for knowledge of the seed value outside of the Noise class.

public class Noise
{
    private static float _seed;

    public float Seed
    {
        get
        {
            return _seed;
        }
        set
        {
            if (value < 0) throw new ArgumentOutOfRangeException(nameof(value), "must be greater than zero");
            _seed = value;
        }
    }


    public Noise(float seed)
    {
        this.Seed = seed;
    }

    ...
}

This would be used, like so

Noise noise = new Noise(0);   // constructor
noise.Seed = 10;              // set
float seedValue = noise.Seed; // get

---

Taking it a step further

You could also avoid repeating a lot of code, by moving all of the above into a "base" class, I've added the example here:

Note: that the access modifier has been changed to protected, but you can change this behavior if you do not want the inherited class to have direct access.

public class BaseClass
{
    protected float _seed;

    public float Seed
    {
        get
        {
            return _seed;
        }
        set
        {
            if (value < 0) throw new ArgumentOutOfRangeException(nameof(value), "must be greater than zero");
            _seed = value;
        }
    }

    public BaseClass(float seed)
    {
        this.Seed = seed;
    }
}

Then you would derive your Noise class from it:

public class Noise : BaseClass
{
    public Noise(float seed) : base(seed) { }
}

Answer 5

Just put it in class Settings. If you move your class Noise in another project, you will need to do some refactoring anyway.