FAQ
overflow

Great Answers to
Questions About Everything

QUESTION

I have a class that listens to a file event constantly and reads that json file once it's created and makes of it a mail report and an excel file.

Now, I made the properties that are used for both these tasks static so I could determine their values once from the json and easily access them from all methods:

public class Reporter
{
    public static bool ShouldNotify = false;
    public static string CSVFilePath;
    private static string HTMLReport;
    private string aSerializedObject;
    ...
    public Reporter(string jsonFilePath)
    {
      //Determine static params values. 
    }
    ...
    public string BuildHtmlReport() { ... }

    public string WriteCsvFile() { ... }
}

There are a couple of things I feel that aren't best practices:

  1. Is it bad practice to combine static and non static params in the same non static class? Eventually it's a lot like using local and global variables but maybe I should encapsulate them.
  2. Initializing all necessary params in the constructor - I always call the constructor with the jsonFilePath and don't use the default constructor.
  3. I don't dispose the instance when finished since most properties are static and I don't really need to (functionality wise), should I?
{ asked by Moshisho }

ANSWER

  1. Not only is it bad practice, I'd say it's horrible practice. Save yourself, or others which will be dealing with your code, some future headache and use as few static variables as possible, preferably none! (The reason why I say this is horrible could have something to do with the code I have to deal with at work, anyways, it is still not good practice...)

  2. I'd say that not using the default constructor is perfectly fine, if all your Reporter objects needs a jsonFilePath then it's good to pass it in the constructor. Also, if this value shouldn't change then you can make it readonly.

  3. If by disposing you mean "clearing it's state" or something, then no you don't need to. Garbage collector will take care of that. However, if you keep using the static variables then you might run into problems if you want to create another one.

So, how to get rid of these static variables?

It's quite easy actually: Tell, don't ask.

You haven't given much code here so I'll have to try to explain how it generally works:

In your json class (which I will call Foo from now on since I don't know it's actual name), or whatever you have, instead of using Reporter.ShouldNotify, create an instance variable of the Reporter type. Then if you call/create your Foo class from your Reporter class, you tell the Foo object about it's Reporter. For example:

// In your reporter class
Foo foo = new Foo();
foo.setReporter(this); // sorry for the Java naming conventions here, but I hope you get the idea

In Foo you can then use your Reporter object which you retreived from the setReporter method to use that particular object.

Note that one can take the "Tell, don't ask" thing even further, but with the code in your question this is what I think you need for now.

{ answered by Simon André Forsberg }
Tweet