2

How can I make this look better:

lblTotalWorldSize.Text = GetDirectorySize(_worldsDirectory + selectedItem) * 1024 + " kb";    // Total world size
if (Directory.Exists(_worldsDirectory + selectedItem + "\\" + selectedItem))                  // World itself
{
    lblWorldSize.Text = GetDirectorySize(_worldsDirectory + selectedItem + "\\" + selectedItem) * 1024 + " kb";
}
else
{
    lblWorldSize.Text = "Couldn't find world.";
}
if (Directory.Exists(_worldsDirectory + selectedItem + "\\" + selectedItem + "_nether"))      // Nether
{
    lblNetherSize.Text = GetDirectorySize(_worldsDirectory + selectedItem + "\\" + selectedItem + "_nether") * 1024 + " kb";
}
else
{
    lblWorldSize.Text = "Couldn't find world.";
}
if (Directory.Exists(_worldsDirectory + selectedItem + "\\" + selectedItem + "_the_end"))     // The End
{
    lblTheEndSize.Text = GetDirectorySize(_worldsDirectory + selectedItem + "\\" + selectedItem + "_the_end") * 1024 + " kb";
}
else
{
    lblWorldSize.Text = "Couldn't find world.";
}

It really looks like a mess and I can't seem to find any questions like this.

1
  • 1
    Is the assignment to lblWorldSize in every else a mistake? Jul 27, 2012 at 18:22

5 Answers 5

7

Well, there are several things that can help here:

  • A helper method (you're performing the same conversion from "path" to "size string" each time)
  • The conditional operator
  • Extracting a common local variable

Something like this:

// Note that _worldsDirectory must be an absolute path)
string prefix = Path.Combine(_worldsDirectory, selectedItem, selectedItem);
lblWorldSize.Text = GetDirectorySizeOrDefault(prefix, "Couldn't find world");
lblNetherSize.Text = GetDirectorySizeOrDefault(prefix + "_nether",
                                               "Couldn't find _nether");
lblTheEndSize.Text = GetDirectorySizeOrDefault(prefix + "_the_end",
                                               "Couldn't find _the_end");

...

static string GetDirectorySizeOrDefault(string directory, string defaultText)
{
    return Directory.Exists(directory)
        ? GetDirectorySize(directory) * 1024 + " kb"
        : defaultText;
}

Note that I've corrected the fact that your original code always assigns to lblWorldSize.Text on error - I assume that wasn't deliberate.

3
  • Like making a function that can check each folder? Jul 27, 2012 at 18:22
  • I see! This sure makes it look better. BTW, if I do it the other way (how I'm doing it right now) will it be slower compared to your way? Jsut curious Jul 27, 2012 at 18:26
  • I'm not sure whether to be mad that you posted essentially the same answer as me, before me, or to be happy that I thought of the same answer as Jon Skeet :)
    – Michael
    Jul 27, 2012 at 18:36
2

Please use Path.Combine for path creation, and not string concatenation like you do.

0

Is it possible that this method does more than 1 things? You could refactor this in a few other methods maybe.

0

I would suggest to use intention and variable substitution to enhance readability:

string dir = "_worldsDirectory + selectedItem";
string dirItemPath = @"_worldsDirectory + selectedItem \\" + selectedItem;

// Total World Size
lblTotalWorldSize.Text = GetDirectorySize(_worldsDirectory + selectedItem) * 1024 + " kb";

// World itself
if (Directory.Exists(dirItemPath)) {

    lblWorldSize.Text = GetDirectorySize(dirItemPath) * 1024 + " kb";
} else {

     lblWorldSize.Text = "Couldn't find world.";
}

// Nether
if (Directory.Exists(dirItemPath + "_nether")) {

    lblNetherSize.Text = GetDirectorySize(dirItemPath + "_nether") * 1024 + " kb";
} else {

    lblWorldSize.Text = "Couldn't find world.";
}

// The End
if (Directory.Exists(dirItemPath + "_the_end")) {

    lblTheEndSize.Text = GetDirectorySize(dirItemPath + "_the_end") * 1024 + " kb";
} else {

    lblWorldSize.Text = "Couldn't find world.";
}
0
lblTotalWorldSize.Text = GetDirectorySize(_worldsDirectory + selectedItem) * 1024 + " kb";    // Total world size

DoOnDirectoryExists(lblWorldSize, "");    
DoOnDirectoryExists(lblNetherSize, "_nether");    
DoOnDirectoryExists(lblWorldSize, "_the_end");    


//Change the name to something more appropriate to your program, same with the LBL type and tempString
public bool DoOnDirectoryExists(LBL lbl, string suffix)
{
    string tempString = _worldsDirectory + selectedItem + "\\" + selectedItem + suffix;

    if (Directory.Exists(tempString))     
    {
         lbl.Text = GetDirectorySize(tempString) * 1024 + " kb";
    }
    else
    {
        lblWorldSize.Text = "Couldn't find world.";
    }
}

Best way to get rid of clutter and make your code more readable - remove code duplication.

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.