13
\$\begingroup\$

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?
\$\endgroup\$
12
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply, I understand 2 and 3 now, but I still don't get why use as few static variables? how it affects whoever deals with the code later? \$\endgroup\$ – Moshisho Feb 6 '14 at 8:43
  • 2
    \$\begingroup\$ @Moshisho Using static variables isn't object oriented, it makes your code less extensible. You won't be able to use multiple objects without them causing conflicts because of the static variables. All object instances share the same static variables. \$\endgroup\$ – Simon Forsberg Feb 6 '14 at 8:56
  • 1
    \$\begingroup\$ In C#, you can use events instead of something like setReporter(this). \$\endgroup\$ – svick Feb 18 '14 at 14:50
  • \$\begingroup\$ for point no 3 GC wont take care of disposing static members. \$\endgroup\$ – Jaydeep Shil Jul 12 at 2:59
  • \$\begingroup\$ @JaydeepShil I'm not entirely familiar with the GC in C# but I doubt that is true. I don't see any reason why it wouldn't be able to deal with static members as well. Do you have a source for that claim? \$\endgroup\$ – Simon Forsberg Jul 12 at 7:56
4
\$\begingroup\$

It's not so much a question of best practices, but rather a question of correctness. From my understanding of the problem you are trying to solve, using static variables for CSVFilePath and HTMLReport would be incorrect, since (I assume) their values depend on the jsonFilePath passed into the Reporter's constructor. If you had multiple Reporters, each monitoring a different JSON file, each Reporter would have a different CSVFilePath and HTMLReport — hence, they should be instance variables, not class variables.

That said, instance methods rarely use class variables, unless the class variables are const or readonly. If your instance method does use a static variable, you should at least rethink what you are doing, and why.

If you only expect to ever create one Reporter in your system, look into making Reporter a singleton.

\$\endgroup\$
2
\$\begingroup\$

Yes, that is a bad idea.

Think of it this way.

Say you want to process 10 items with your reporter. 5 should notify, 5 should not. So what do you do?

Reporter reporter = new Reporter();
reporter.ShouldNotify = true;
...
...
reporter.ShouldNotify = false;
...
...

Okay...so later on you have some more to do, swap em again? not only does this litter the code but it is something easy to forget to change it.

Next, you have to make the task asyncronous...oops. no joy, people keep switching the flag on you!

Any properties like this should be constructor based. In fact my personal favourite solution is to package such things either in an entirely different class or to at least pass in a parameterCollection

so in short something like:

Reporter reporter = new Reporter(ReportTypeOneSettings);

and whats cool if you find yourself doing the same few configurations all the time you can instantiate multiple reporters super quickly.

Reporter reporterNotify = new Reporter(ReportTypeTwoSettings);

Now I can use the appropriate reporter with no fear of effecting anything else. also, if I have to suddenly change something for ALL the reports all I have to do is swap out the settings.

\$\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.