Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

There has got to be a better way to do this. I have a method which returns either your standard "01/01/2014" Date or "January 1st, 2014" Date of the assembly file write time. Any suggestions on streamlining would be much appreciated.

// Grab the last date of revision of the assembly file write time
// You may pass any value 1-2
// 1 will return format MM/dd/YYYY
// 2 will return format MonthName Day-nth, Year
public static string RevisionDate(int id = 0)
{
    System.IO.DirectoryInfo Root = new System.IO.DirectoryInfo(HttpContext.Current.Server.MapPath("~"));
    Assembly Assy = Assembly.GetExecutingAssembly();
    string[] AssyFullName = Assy.FullName.Split(',');
    string Title = AssyFullName[0];
    string Path = null;

    if (Root.ToString().Length - 1 != ('/'))
    {
        Path = Root.ToString() + "\\bin\\" + Title + ".dll";
    }
    else
    {
        Path = Root.ToString() + "bin\\" + Title + ".dll";
    }

    var ModifiedDate = System.IO.File.GetLastWriteTime(Path);

    switch (id)
    {
        case 1:
            return Convert.ToDateTime(ModifiedDate).ToString("MM/dd/yyyy");
        case 2:
            string MonthName = Convert.ToDateTime(ModifiedDate).ToString("MMMM", CultureInfo.InvariantCulture);
            string Day = Convert.ToDateTime(ModifiedDate).ToString("dd");
            string Year = Convert.ToDateTime(ModifiedDate).ToString("yyyy");

            //eliminate leading zeros from Day
            if (Day[0] == '0')
            {
                Day = Day[Day.Length - 1].ToString();
            }

            // Beautify String
            string nth = string.Empty;
            switch (Day[Day.Length - 1]) 
            {
                case '1':
                    nth = "st";
                    break;
                case '2':
                    nth = "nd";
                    break;
                case '3':
                    nth = "rd";
                    break;
                default:
                    nth = "th";
                    break;
            }
            return MonthName + " " + Day + nth + ", " + Year;
        default:
            return "You must provide a format value 1 or 2";
    }
}
share|improve this question
add comment

3 Answers 3

up vote 5 down vote accepted

What will RevisionDate(5) do? I can't tell from the code. Methods should be expressed as actions: [Action][Context]; since you are returning a string I suppose GetRevisionDateById might be more appropriate (you can omit the ById part if there are no other ways of retrieving it).


Use using statements so you don't have to fully quality System.IO.DirectoryInfo but can instead use DirectoryInfo.


Local variables are written in lowerCamelCase.


You never use AssyFullName so you might as well just do this:

string assemblyName = Assembly.GetExecutingAssembly().FullName.Split(',');

This is some very obscure code:

 if (Root.ToString().Length - 1 != ('/'))

So you get a directory, take the length, substract one and compare it to the ASCII value of /? What? Unless I am reading it incorrectly, this doesn't do what you think it does. Can you give an example of a Root value so I can propose something better?


What exactly is id? I would connect it to a uniqueness factor of an object but here it is.. some way to describe what action to take? How do I know what 1 and 2 do?

Use an enum to describe the purpose:

enum DateStyle {
    ShortHand,
    FullBlownDateBaby
}

You do Convert.ToDateTime on ModifiedDate which is the result of File.GetLastWriteTime which already returns a DateTime object so that entirely unnecessary.


I am not great with datetime styling but I am 105% sure you are reinventing the wheel. Take a look here (standard options) and here (custom options) for all the different ways you can create your own date and time representation.


You're returning a string in the default statement of your switch. I would use an exception (like ArgumentException) to clearly signify that exceptional (and incorrect) input has occurred.


As I noted in the comments above: indenting. Braces are written on newlines (but not indented themselves) so an if statement looks like this:

if(condition) 
{
    action();
}
share|improve this answer
    
The root bit is to ensure that I am at the end of a directory. I was having issue where in my dev environment (which is on a local machine) would handle Path = Root.ToString() + "bin\\" + Title + ".dll"; just fine but the server the apps are actually hosted on (which is on a machine somewhere in Arizona) needed Path = Root.ToString() + "\\bin\\" + Title + ".dll"; On the server it returns a path like D:\Websites\AppName on the sever and `C:\Documents and Settings\User\My Documents\VS 2010\AppName` on my local machine. –  aaronmallen Jun 5 at 23:12
1  
@aaronmallen: I think it'd be easier if you used Root.ToString().TrimEnd(new[] {'\'}) + "\\bin\\" + Title + ".dll";. This will remove the trailing occurrences of the backslash if there are any. –  Jeroen Vannevel Jun 5 at 23:36
    
And I just realized that's the wrong slash lol. –  aaronmallen Jun 5 at 23:42
    
Do I create the enum before RevisionDate() or inside it? –  aaronmallen Jun 5 at 23:47
1  
Before. An enum should be outside your class (it's at the same level of hierarchy as a class and interface). –  Jeroen Vannevel Jun 5 at 23:48
show 2 more comments

Three points to make beyond what was already said (and I strongly recommend you follow what was already said as well).

First, you ask for a day with a leading zero, then remove the leading zero. Better to just ask for the day as you want it in the first place:

string Day = Convert.ToDateTime(ModifiedDate).ToString("%d");

Now we can drop the part where we have to trim off that leading zero.


Second, you can DRY your code out a little by not repeating Convert.ToDateTime(ModifiedDate) multiple times. Instead, do it once and reference that:

DateTime ModifiedDate = Convert.ToDateTime(System.IO.File.GetLastWriteTime(Path));

switch (id)
{
    case 1:
        return ModifiedDate.ToString("MM/dd/yyyy");
    case 2:
        string MonthName = ModifiedDate.ToString("MMMM", CultureInfo.InvariantCulture);
        string Day = ModifiedDate.ToString("%d");
        string Year = ModifiedDate.ToString("yyyy");

Third, you are right at the threshold of needing to extract out at least one helper method as this is growing a little long. Case in point, your ordinal logic (adding the "st", "nd", "rd", "th") is fairly long, yet still insufficient. For the 12th day of the month, it would print "12nd". This area of code is perfect for extraction. It would be nice if C# had something to do this built in, but unfortunately it appears they do not. Fortunately, though, you aren't the first to have this problem, and someone else has already done the work for you.

share|improve this answer
    
I've changed the switch for day to switch (day) { case "1": case"21": case"31": nth = "st"; break; case "2":, case "22": nth = "nd"; break; case "3": case "23": nth = "rd"; break; default: nth = "th"; break;} do you recommend a different approach? –  aaronmallen Jun 9 at 19:54
    
also using modifiedDate.ToString("d"); returns the entire date value with the day missing leading zeros not just the day. –  aaronmallen Jun 9 at 20:00
1  
Corrected modifiedDate.ToString. Single custom identifiers need a special character (%) to distinguish them. See msdn.microsoft.com/en-us/library/… for details. As for the switch statement, your approach seems fine (though less general than the solution linked to), but you need to be able to know for yourself if it truly works through testing. Extracting this logic into a helper method would go a long way in making that easier, but a test of a whole method could also tell you. –  cbojar Jun 10 at 0:29
add comment

I'll preface this with "I'm just learning C# too and I might have gotten some of the syntax wrong, but the principals are sound."

First, fix your indenting.

if (Root.ToString().Length - 1 != ('/'))
    {
Path = Root.ToString() + "\\bin\\" + Title + ".dll";
    }

Should look more like this

if (Root.ToString().Length - 1 != ('/'))
{
    Path = Root.ToString() + "\\bin\\" + Title + ".dll";
}

One problem I see is that you're using magic numbers for your argument. Set up an enum instead.

enum ArgOptions {Option1,Option2,Option3}

Then you can call your function with a meaningful argument and use that for your switch.

public static string RevisionDate(ArgOptions id = Option1) 'I'm not positive of the syntax here. I don't know c# very well.
//
// 
    switch(id)
    {
        case Option1:
            return Convert.ToDateTime(ModifiedDate).ToString("MM/dd/yyyy");
        case Option2:
            string MonthName = Convert.ToDateTime(ModifiedDate).ToString("MMMM", CultureInfo.InvariantCulture);
            string Day = Convert.ToDateTime(ModifiedDate).ToString("dd");
            string Year = Convert.ToDateTime(ModifiedDate).ToString("yyyy");

You should also throw new argument error here instead of returning a string from your function if it hits the default case.

Instead of

  default:
      return "You must provide a format value 1 or 2";

Try

throw new ArgumentException("You must provide a valid format value");
share|improve this answer
1  
There may be, but you've also just wrote it. This is exactly what abstraction is for. Put the part of your code that "makes it pretty" into a function in a Dates class of your own. Then you could use it anywhere in your code. –  ckuhn203 Jun 5 at 21:47
1  
This explains it pretty well. stackoverflow.com/a/47890/3198973 –  ckuhn203 Jun 5 at 22:20
1  
@ckuhn203: the braces should not be indented; they're at the same level as an if statement. Likewise: they have to be on a newline at your switch statement –  Jeroen Vannevel Jun 5 at 22:23
2  
@aaronmallen: his indentation in his answer is still incorrect. I'll add it to my own answer (which I'm writing now) –  Jeroen Vannevel Jun 5 at 22:42
1  
Sidenote: you don't have to cast the enum to an int in a switch statement. –  Jeroen Vannevel Jun 5 at 22:58
show 4 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.