Is this bad practice?

[Nick Z.]

Consider the following code:

class Log
{
private static StreamWriter w = null;
private static XmlTextWriter tw = null;

private void InitWriter()
{
w = new StreamWriter("log.xml", true, Encoding.Unicode);
tw = new XmlTextWriter(w);
}

private void Finalize()
{
if(tw!= null)
{
tw.Flush();
tw.Close();
}
}

public static void Report(string message)
{
if(tw == null)
InitWriter();

tw.WriteStartElement("message");
tw.WriteString(message);
tw.WriteEndElement();
}
}

Is it "wrong" to have a static stream open throughout the lifetime of a
class, and what are the implications of using the Finalize method? The
Finalize method is a serious performance hit as far as I know, but it
seems that its the right way to go here or not?

Opening the stream every time the Report method is called is much much
slower.

Thanks,
Nick Z.

 

[Nicholas Paldino [.NET/C# MVP]]

Nick,

It's not such a bad idea, but depending on how you open the file (and
from the looks of this, it will be an exclusive lock), you won't be able to
open access the file while the app is running.

If you are going to store the StreamWriter and the XmlTextWriter in
static fields, you don't need the finalizer (as the CLR will clean those up
when it shuts down). Additionally, you use the tilde (~) syntax for the
finalizer, not a method named Finalizer (I don't believe that the compiler
lets you override Finalizer, you are shadowing it).

Instead of performing the check in every call to report, it would be
better to create a static initializer ("static Log()") which would create
the StreamWriter and the XmlTextWriter. This way, you save yourself a check
every time Report is called.

Finally, you would have to make this thread safe. If Report was called
from multiple threads at the same time, it would mess with the state of the
XmlTextWriter.

Hope this helps.

 

[Nick Z.]

Nick,

It's not such a bad idea, but depending on how you open the file (and
from the looks of this, it will be an exclusive lock), you won't be able to
open access the file while the app is running.

Thats one issue I have to work with, because I need the ability to open
the log. Maybe it is possible to open the stream with FileShare.Read and
FileAccess.ReadWrite?

If you are going to store the StreamWriter and the XmlTextWriter in
static fields, you don't need the finalizer (as the CLR will clean those up
when it shuts down).

Yes, but will it flush w.e is still in the stream?

Additionally, you use the tilde (~) syntax for the
finalizer, not a method named Finalizer (I don't believe that the compiler
lets you override Finalizer, you are shadowing it).

Thanks, I did not know this, I never used the finalizer before.

Instead of performing the check in every call to report, it would be
better to create a static initializer ("static Log()") which would create
the StreamWriter and the XmlTextWriter. This way, you save yourself a check
every time Report is called.

Thats actually what I'm doing I re-typed the code when I was posting.
Thanks for pointing this out.

Finally, you would have to make this thread safe. If Report was called
from multiple threads at the same time, it would mess with the state of the
XmlTextWriter.

Hmmm... I see this being a major problem, but how would I go about
fixing this? I'm not entirely sure.

Hope this helps.

Very much appreciated, thank you.
Nick Z.

 

[Nick Z.]

I think if I implement a message Queue and open the stream every time
there are messages to process in another background thread that will
solve my problems.

I'll give this a try.
Nick Z.