Refactor DirectoryHelper

[ThomasBarnekow]

Rename TryDeleteDirectory(string, int, int, int) to DeleteDirectory(string, int, int, int) because this method can still throw exceptions and is not fully in line with the try-parse pattern.

Introduce TryDeleteDirectory(string) : bool as a try-parse pattern-compliant option to delete directories recursively without throwing exceptions. It doesn't need an out parameter because the basic method it wraps does not return a value.

Remove DeleteSubdirectories(string) method because it is not used anywhere (housekeeping).

Use var keyword consistently across all methods.

Related to #980.

[Therzok]

I may be picky, but I'd rather not hide types here.

[Therzok]

I don't like those var changes. Hiding types from stuff that's not clear from the right side of the expression is just not my style.

[ThomasBarnekow]

I'm not precious about these changes. Unless anybody else votes for keeping them, I could roll them back.

[ThomasBarnekow]

On the other hand, you could also say the variable names are suboptimal and, therefore, the type helps understand what these really are. For example, would you still disagree with the change if we used more descriptive variable names, like so?

var fileNames = Directory.GetFiles(directoryPath);
var directoryNames = Directory.GetDirectories(directoryPath);

From my point of view, it would make sense to rename these variables even if we roll back the previous change.

@nulltoken What do you think?

[nulltoken]

I don't like those var changes. Hiding types from stuff that's not clear from the right side of the expression is just not my style.

👍 We indeed tend to use var when the right side of the expression is explicit about the type being returned.

int i = 1;
var j = (long)i;
var things = new List<string>()

[ThomasBarnekow]

@Therzok and @nulltoken I've rolled back those changes but renamed the variables to be more explicit about what those really are.

[Therzok]

Why was this moved? :D

[ThomasBarnekow]

To have all fields at the top of the class. The other existing field also did not directly precede the (only) method in which it was used. In other words, consistency ...

[Therzok]

Fine by me.

[ThomasBarnekow]

I'm also a very big fan of code readability. However, this is also driven by layout and redundancy, for example.

This is just a matter of preference, and I tend to swing on the explicit typing side of the wagon.

Explicit is good, unless it is really redundant. Redundancy adds clutter. While that is not the case when you write int vs. var, other situations benefit from the more "tabular" look and feel that using var brings about. For example:

int number = 1;
IEnumerable<string> strings = <something-that-obviously-returns-an-IEnumerable<string>>;

explicitly defines two variables. The following example is less explicit:

var number = 1;
var strings = <something-that-obviously-returns-an-IEnumerable<string>>;

It is clearly a matter of preference or judgement whether or not you would go with the second example. It is certainly nicer on my eyes and I would, therefore, prefer it as long as the righthand side is clearly enough (which is the judgement part) an int (which it is) or IEnumerable<string> (which depends on the concrete example).

Code can change, also. What if you decide to drop the initialization on the same line? You'd have to change it to an explicit type -> noise in diffs.

Dropping the initialization on the same line will add noise no matter what. Changing int amount = 1; to int amount; adds the same noise as going from var amount = 1; to int amount;.

How would you specify which of the values the variable is going to take? Is it going to be unsigned? Is it going to be signed? I know you're going to say - look at the value.

There is a reason why the type of integer literals defaults to int (a signed integer) unless that integer literal can't be represented as an int. It is because, in the vast majority of cases, a signed integer is appropriate and you want just that. Therefore, I'm totally fine with var amount = 1; for signed integers. If I wanted an unsigned integer, that would be an exceptional case in which I would definitely be explicit about my choice and write uint amount = 1;. I would not even write var amount = 1u, because it is so exceptional to me personally.

var amount = 1.5

I would argue here that you probably wanted a float and you'll get lots of warnings downhill about float and double mixing.

As for int being the default type for integer literals, there's a good reason why double is the default type for real literals. Most people in most cases seemingly don't want a float unless they have to use that type, e.g., because some API demands it. In case I had to use float I would again be explicit about it. Since double is the default, I'm fine with var when declaring and initializing a double at the same time, using a real literal (which would have to be suffixed to mean anything else than a double).

But I'd have to scroll to the definition and not look at the type, but at the value.

Wouldn't you also have to scroll to the definition and look at the value "where it is nice on the eyes"?

Again, believe it or not, I'm not precious about var vs. int in this specific example. I actually changed it because ReSharper proposed it and I thought, "hey, that makes sense". I'd really be fine with rolling it back. So, @Therzok and @nulltoken, just tell me what you want me to do.

Personally, I now tend to use var more and more often and consistently. At the same time, I'm trying to use meaningful variable names so that I don't have to rely too much on the types. Wherever I must make assumptions on the characteristics of a collection, for example, I make sure the name is explicit about that (certainly not in the legacy prefix notation sense (e.g., lpstr, though). This way, I don't even have to go back to the declaration, whether it's explicit or implicit, to understand what a variable represents.

[ThomasBarnekow]

Here you go ... I rolled it back and changed for (var attempt = 1; ... into for (int attempt = 1; .... Told you I was not precious about it ;-)

[ThomasBarnekow]

Travis said it was "unable to fetch some archives".

[nulltoken]

Build has been restarted

[nulltoken]

As this method is no longer used, let's just drop it.

[ThomasBarnekow]

Will do.

[nulltoken]

Other than the nitpick above, looks pretty nice to me. Thanks!

[ThomasBarnekow]

Deleted TryDeleteDirectory().

[nulltoken]

Another one in. Thanks! 💖

[ThomasBarnekow]

Thanks!