Most obvious potential for error is if there is a second call made to log an entry to the same file as in the first call, and the caller of the first call is still writing to the log, an error will be thrown in the second call since the file is still locked as a result of the first call.

Oh, and since we're "creating" a file each time, any subsequent calls to the GetLogger will completely obliterate any previously written logs to the same file name (unless the CreateText call will not actually create the file if it already exists and will instead return a stream to append, but I would have to check the documentation to be sure on that)

Threading issue if 2 threads create a new Log with the same path?

Directory.Exists() could return false for both, then they will both try to create it, and the second one will get an exception.

Shane,

Assume single threaded access and proper use of using.

I updated the code to include open/ create as needed, because that wasn't the point of the bug

You need to call Log(string path) first to set the path field before you can get your logger, else it should throw an exception.

If path refers to a file, ex c:\foo where foo is a text file, Directory.Exists("C:\foo") will return false, but then Directory.CreateDirectory("c:\foo") will throw an exception.

ok, assuming a single thread model (single app?), my first assertion is not valid.

I stand by my second assertion that subsequent calls will delete the contents of previously written logs. I checked the documentation this time :)

"If the file does exist, its contents are overwritten."

So, if (assuming that) the intention was to append a log entry to existing contents, then there is a subtle bug here as previously written entries will be overwritten since the call to "CreateText" recreates the file as new if it already exists.

You could pass in a relative path that does not exist to the constructor - say "MyLogs\ThisProgram". The constructor would then create the new directory based on the current directory. After this, the path is stored by the class in the relative path form. SO... if the current directory changes and then GetLogger is called, Path.Combine is going to use the new current directory + the relative directory which is (not likely) not going to exist.

meant to type -->

which is (most likely) not going to exist.

Here's a wild stab in the dark:

File.CreateText returns a StreamWriter which implements TextWriter.

File.OpenText returns a StreamReader which implements TextReader.

ergo

when you OpenText, you do so to read, not to write?!

So if the file doesnt exist, then booom, exception time.

ding ding ding.... I think Markk got it.

Markk has definitely found a bug, but I don't think it is the original bug Oren intended for us to find.

That part of the code was not in the original code written for us to solve ( I think in Oren's haste to modify the original text to remove the possibility of overwriting existing logs he mistakenly chose OpenText instead of the proper AppendText)

Hmm... Does this even compile? One return a StreamWriter one return a StreamReader...

mendicant,

Interesting, I haven't thought of that.

That is not what I was thinking about, however

Tetsuo ,

Precisely! And a something that is not obvious (at least to me) until you give it some thought.

This is a class of problem that can be classed as "assuming fixed context"

Mark,

That was a bug when I updated the code. Shane is correct, AppendText was supposed to be there (and is now)

Oren, you also have a potential long path issue. If the combination of path and name overflow the maximum length of a path in Windows (I believe in the .NET APIs, the default max length is 255 chars), then the File.Create will fail (I'm thinking the File.Append would too).

I used to run into this all the time when writing extraction software that might extract an archive that contains an archive that contains an archive. It wasn't "uncommon" to run into the maximum length issue in Windows.

The problem with Directory.Create:

Directory.Exists("C:\NotExists") = false

Directory.Exists("C:\NotExists\OfcourseNotExists") = false

Directory.Create("C:\NotExists\OfcourseNotExists") = exception

Anyhow, I would stay away from Path.Combine when using input provided by some user/caller (i.e. you are not in control of the contents submitted to .Combine). Take a look at the description of Path.Combine and it will scare you - talk about random results... :-)

I just checked Path.Combine again.

Can you explain what you are objecting to?

Can I assume that the path argument in the constructor is always non-null, not empty and does not contain illegal characters (? * etc.)?

Yes, you can.

I got it:

Even within a single thread, multiple calls to get the same logger can be made. Thus, more than one StreamWriter must be opened to the same file, which will fail.

example:

public class Foo

{

public static void Main() {

StreamWriter logger = new Log(temp).GetLogger("default.log");

//...

new Bar.Work();

}

}

public class Bar

{

public void Work() {

// cannot open two StreamWriters to the same file

StreamWriter logger = new Log(temp).GetLogger("default.log");

}

}

Yeah... that is an issue.

The bug I was looking for was changing the current directory, those breaking the implicit assumption in the code

@Ngoc Van

Nope, Directory.Create will correctly handle that situation, creating all needed subdirectories.

Also, note that the Directory.Exist call is unnecessary as Directory.Create handles that.

If changing the current dir was an issue, wwfDev is right regarding Path.Combine.

From msdn doc:

"Return Value

Type: System..::.String

A string containing the combined paths. If one of the specified paths is a zero-length string, this method returns the other path. If path2 contains an absolute path, this method returns path2."

I'm glad I thought of most of the potential issues mentioned here before reading the comments (multi-thread, relative path, path.combine, combined path too long). Made me feel like I am a better dev than I really am. :)

James, That stuff always happen when using somebody library (Checking for something else that will be check again) But it's good programming practice anyway especially when using somebody else lib, unless the name is CreateIfNotExist.

Oren, ahh... the beauty of relative path :) I see the bug now. I've encounter this bug before I end up caching the first current directory and lock the user to that path your requirement might be different.

System.IO.PathTooLongException, too.

(Folder name is valid, file name is valid, combination is big kablooie).