5
\$\begingroup\$

I have designed a tool that generates some HTML files and a CSS file in a directory, which then feed into a tool called Jutoh that uses these to make eBooks.

However, my output files are of the form 1.htm, 2.htm and so on. Jutoh ranks 10.htm to preceed 2.htm and therefore, I wrote a method to convert 1.htm into 0001.htm. I would like to know your opinion on it and learn where there might be scope for speedup and other improvements.

var inputDir = _cssFile.Directory.FullName + "\\Output\\";
var outputDir = _cssFile.Directory.FullName + "\\Output";
if (!Directory.Exists(outputDir))
    Directory.CreateDirectory(outputDir);
DirectoryInfo di = new DirectoryInfo(inputDir);
foreach (FileInfo fi in di.GetFiles("*.htm"))
{
    string n = fi.Name.Replace(fi.Extension, ""); //remove extension
    int k = Int32.Parse(n); //Convert string to int
    string t = k.ToString("D4"); //convert it into 4 digit
    t = t + fi.Extension; // add extension

    string trgtfile = Path.Combine(fi.DirectoryName, t);
    File.Move(fi.FullName, trgtfile);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! Is there a lot of surrounding code? You might get more/better feedback if you include your entire method's body (and signature). \$\endgroup\$ – Mathieu Guindon Aug 30 '14 at 4:42
  • 2
    \$\begingroup\$ You could gain a lot of simplicity by using the Pad method available to strings, rather than converting the string to an integer and formatting it back into a string. \$\endgroup\$ – tinstaafl Aug 30 '14 at 11:28
  • \$\begingroup\$ @Mat'sMug : Thank you for an interesting question. There isn't much surrounding code. The core logic is to solve the problem of the numbering where 10 precedes 2 . \$\endgroup\$ – ValarMorghulis Jan 2 '18 at 5:33
  • \$\begingroup\$ @tinstaafl : Interesting observation. :) \$\endgroup\$ – ValarMorghulis Jan 2 '18 at 5:34
12
\$\begingroup\$

Here are several tips that might help write better/cleaner code. Note that my tips or help might not be perfect but it gives you an idea on how to write better code.

  • Naming you variables

Using variable names like k and n are very bad practice. Not only will other people not understand your code, if you haven't looked at your code for a while, you might have forgotten what each variable does/stands for. Give your variables a meaningful name. So after applying this, your code looks like this:

string currentName = fi.Name.Replace(fi.Extension, "");
int nameAsNumber = Int32.Parse(currentName);
string newName = nameAsNumber.ToString("D4");
newName = newName + fi.Extension;

string targetFile = Path.Combine(fi.DirectoryName, newName);
File.Move(fi.FullName, targetFile );
  • Over-creating variables

Storing operations/calculations on variables in new variables can be handy and even necessary in certain situations. But in this situation where you take a string, parse it to an int and make it a string again, I suggest you do this in one line. You can even add the extension in the same line:

string currentName = fi.Name.Replace(fi.Extension, "");
string newName = int.Parse(currentName).ToString("D4") + fi.Extension;

This does not mean you have to write all your code in one line, this an example of doing too much in one line:

string targetFile = Path.Combine(fi.DirectoryName, int.Parse(Path.GetFileNameWithoutExtension(fi.FullName)).ToString("D4") + fi.Extension);

The above line does exactly the same but it's not easy to read and/or maintain.

  • Re-inventing the wheel (part 1)

Why create methods/functions/lines of code that do something that's already been made for you. If you want to get the filename without the extension, use the Path.GetFileNameWithoutExtension() method.

string currentName = Path.GetFileNameWithoutExtension(fi.FullName);
  • Re-inventing the wheel (part 2)

Remember, the .NET framework contains classes and methods for almost anything. And this also goes for adding zeros (or other characters) to a string when they do not yet meet a desired length. This is done with the Pad() method, and in your case the String.PadLeft(Int32, Char) method. You simply provide the desired length and the character that will be put in place. Your code will become:

string currentName = Path.GetFileNameWithoutExtension(fi.FullName);
string newName = currentName.PadLeft(4, '0') + fi.Extension;

Your complete inside-loop code will now look like:

string currentName = Path.GetFileNameWithoutExtension(fi.FullName);
string newName = currentName .PadLeft(4, '0') + fi.Extension;
string targetFile = Path.Combine(fi.DirectoryName, newName);
File.Move(fi.FullName, targetFile);

I hope this helps! :)

\$\endgroup\$
  • \$\begingroup\$ This was very helpful. Thank you. Accepting this answer. \$\endgroup\$ – ValarMorghulis Jan 2 '18 at 5:29

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.