MARCH 11TH, 2008
By RYAN

Just a taste –

Since I’ve been bitching to Dan lately about the code I inherited, here are a few examples:

System.DateTime answer = today.AddDays(-1);
YesterdayTemp = answer.ToString("dd");
TodayTemp = today.ToString("dd");
string TodayMonth = today.ToString("MM");
string currlogdir = @"C:\Faxserver\Logs\";
int currHour = DateTime.Now.Hour;
int currMinute = DateTime.Now.Minute;
int currSecond = DateTime.Now.Second;
if (currHour == 00 && currMinute == 00 && currSecond == 00)
{
      if (File.Exists(currlogdir + "AutoFaxErrorCountLog_" + TodayTemp + ".txt"))
      {
           DateTime tempcreatetime6 = File.GetCreationTime(currlogdir + "AutoFaxErrorCountLog_" + TodayTemp + ".txt");
           string tempday6 = tempcreatetime6.ToString("dd");
           string tempmonth6 = tempcreatetime6.ToString("MM");
           if (TodayTemp != tempday6 || TodayMonth != tempmonth6)
           {
                File.Delete(currlogdir + "AutoFaxErrorCountLog_" + TodayTemp + ".txt");
           }
      }
      FaxCountLog(YesterdayTemp, lblFaxesTodayDisplay.Text);
      FaxErrorCountLog(YesterdayTemp, lblErrorsTodayDisplay.Text);
      lblFaxesYesterdayDisplay.Text = lblFaxesTodayDisplay.Text;
      lblErrorsYesterdayDisplay.Text = lblErrorsTodayDisplay.Text;
      TextlblDisplayTodayAdd("0");
      TextlblDisplayTodayErrorAdd("0");
      TextlblDisplayHourAdd("0");
      TextlblDisplayHourErrorAdd("0");
      FaxesHour = 0;
      ErrorsHour = 0;
      FaxesToday = 0;
      ErrorsToday = 0;
}
if (currHour != 00 && currMinute == 00 && currSecond == 00)
{
     FaxCountLog(TodayTemp, lblFaxesTodayDisplay.Text);
     FaxErrorCountLog(TodayTemp, lblErrorsTodayDisplay.Text);
     TextlblDisplayHourAdd("0");
     TextlblDisplayHourErrorAdd("0");
     FaxesHour = 0;
     ErrorsHour = 0;
}
if (ElapsedHours == 23 && ElapsedMinutes == 59)
{
     ElapsedDays += 1;
     ElapsedHours = 00;
     ElapsedMinutes = 00;
     ElapsedSeconds = 00;
}
if (ElapsedMinutes == 59)
     {
     ElapsedHours += 1;
     ElapsedMinutes = 00;
}
if (ElapsedSeconds == 59)
{
     ElapsedMinutes += 1;
     ElapsedSeconds = 00;
}
else
{
     ElapsedSeconds += 1;
}
if (ElapsedHours >= 0 && ElapsedHours <= 9)
{
     ElapsedHoursTemp = "0" + ElapsedHours;
}
else
{
     ElapsedHoursTemp = ElapsedHours.ToString();
}
if (ElapsedMinutes >= 0 && ElapsedMinutes <= 9)
{
     ElapsedMinutesTemp = "0" + ElapsedMinutes;
}
else
{
     ElapsedMinutesTemp = ElapsedMinutes.ToString();
}
if (ElapsedSeconds >= 0 && ElapsedSeconds <= 9)
{
     ElapsedSecondsTemp = "0" + ElapsedSeconds;
}
else
{
     ElapsedSecondsTemp = ElapsedSeconds.ToString();
}
blElapsedTimeDisplay.Text = ElapsedDays + " Day(s), " + ElapsedHoursTemp + ":" + ElapsedMinutesTemp + ":" + ElapsedSecondsTemp;
}

Replaced with:

int totalSeconds = DateTime.Today.TotalSeconds;
Stopwatch elapsedTime = new Stopwatch();
elapsedTime.Start();
public class Log {
     public void Rotate (FileInfo logFile)
      {
          if ((logFile.Exists) && (logFile.LastAccessTime.Date != DateTime.Today.Date))
          {
               logFile.Delete()
          }
      }
      /* snip */
}
public class Reset {
     public void Labels(bool day, bool hour)
          {
                lblFaxesYesterdayDisplay.Text = lblFaxesTodayDisplay.Txt;
           /*snip*/
           }
     public void Counts(bool day, bool hour)
     {
                /*snip*/
     }
}
Reset reset = new reset;
FileInfo fileErrorLog = new FileInfo(logdir + "AutoFaxErrorCountLog_" + DateTime.Today.Day + ".txt");
if (totalSeconds % 3600 == 0)
{
     if (totalSeconds == 0)
     {
          log.Rotate(fileErrorLog);
          reset.Labels(true, true);
          reset.Counts(true, true);
     }
     else 
     {
          reset.Labels(false, true);
          reset.Labels(false, true);
     }
reset.Dispose();
}
TimeSpan ts = elapsedTime.Elapsed;
lblElapsedTimeDisplay.Text = String.Format("{0} Day(s), {1:00}:{2:00}:{3:00}", ts.Days, ts.Hours, ts.Minutes, ts.Seconds);
}

No, I’m not using Delgates until I can decouple this:
AutoFax.Form1.btnCleanErrors_Click(object, System.EventArgs) AutoFax.Form1.btnRestartPrintService_Click(object, System.EventArgs)
AutoFax.Form1.btnStart_Click(object, System.EventArgs)
AutoFax.Form1.CheckRdy() AutoFax.Form1.CheckSaved()
AutoFax.Form1.ChooseThreads(int)
AutoFax.Form1.CreateNotAndRdy()
AutoFax.Form1.Dispose(bool)
AutoFax.Form1.ErrorOutSecondsNumberGet()
AutoFax.Form1.ErrorOutSecondsNumberSet(int)
AutoFax.Form1.FaxCountLog(string, string)
AutoFax.Form1.FaxErrorCountLog(string, string)
AutoFax.Form1.FaxLog(string, bool)
AutoFax.Form1.Form1()
AutoFax.Form1.Form1_Exit(object, System.EventArgs)
AutoFax.Form1.GetFaxCount()
AutoFax.Form1.InitializeComponent()
AutoFax.Form1.Main()
AutoFax.Form1.menuItemAbout_Click(object, System.EventArgs)
AutoFax.Form1.menuItemShutDown_Click(object, System.EventArgs)
AutoFax.Form1.OnChanged(object, System.IO.FileSystemEventArgs)
AutoFax.Form1.ProcessFile(string, bool)
AutoFax.Form1.ProgressIncrement(int)
AutoFax.Form1.Rdy()
AutoFax.Form1.read_ini_settings()
AutoFax.Form1.SendAlert(string)
AutoFax.Form1.SendEmail(string)
AutoFax.Form1.SendFax()
AutoFax.Form1.SendFaxToProgram(string, string)
AutoFax.Form1.Textbox1Add(string)
AutoFax.Form1.Textbox1Append(string)
AutoFax.Form1.TextbtnStartadd(string)
AutoFax.Form1.TextlblDisplayHourAdd(string)
AutoFax.Form1.TextlblDisplayHourErrorAdd(string)
AutoFax.Form1.TextlblDisplayTodayAdd(string)
AutoFax.Form1.TextlblDisplayTodayErrorAdd(string)
AutoFax.Form1.TextlblFaxesAdd(string)
AutoFax.Form1.TextlblQueAdd(string)
AutoFax.Form1.TextlblSendAdd(string)
AutoFax.Form1.timer1_Tick_1(object, System.EventArgs)
AutoFax.Form1.timer2_Tick(object, System.EventArgs)
AutoFax.Form1.timerHBT_Tick(object, System.EventArgs)

Who needs classes? That Form1 class can just get every method known to man.
More joy:

String rdydir = @"C:\RDY\INCOMING\";
String notdir = @"G:\NOT\";
String str1 = path.ToUpper();
String delim = rdydir;
String delim1 = "NOT.RDY";
String str2 = str1.Trim(delim.ToCharArray());
if ((File.GetAttributes(path) & FileAttributes.Normal) == FileAttributes.Hidden)
{
    File.Delete(path);
    FaxLog("deleted file: [" + path + "]", false);
}
else
{
File.SetAttributes(path, FileAttributes.Normal);
File.Delete(path);
FaxLog("deleted file: [" + path + "]", false);
}
String str3 = str2.Trim(delim1.ToCharArray());
String NotFile = str3 + ".NOT";
TextlblQueAdd("looking for: [" + NotFile + "]");

Replacement:

 string notdir = @"G:\NOT\";
string NotFile = Regex.Replace(path, @".*\\(\w+\d+).*", "$1" + ".NOT").ToUpper();

This is just…

string templine = sr.ReadLine();
sr.Close();
if (templine != null)
{
      int test = templine.IndexOf("[");
      int test2 = templine.IndexOf("]");
      string test3 = templine.Substring(test + 1, test2 - 1 - test);
      if (test3 != null)
      {
            Regex reDateTime = new Regex(@"^(\d{2})/(\d{2})/(\d{4}) (\d{2}):(\d{2}):(\d{2})$");
            if (reDateTime.IsMatch(test3))
            {
                  int test4 = test3.IndexOf(" ");
                  string test5 = test3.Substring(0, test4);
                  if (test5 == TodayMonthTempStr + "/" + TodayDayTempStr + "/" + TodayYearTemp)
                  {
                        filedatetimeisthesame = true;
                        // file exists and it has today's date, so just append to file
                        FileStream fs2 = new FileStream(templogpath + BeginLogName + MiddleLogName + TodayDayTempStr + ".txt", FileMode.Append, FileAccess.Write);
                        StreamWriter m_streamWriter2 = new StreamWriter(fs2);
                        string FaxLogDate2 = DateTime.UtcNow.ToUniversalTime().ToShortDateString();
                        string FaxLogTime2 = DateTime.UtcNow.ToUniversalTime().ToLongTimeString();
                        m_streamWriter2.WriteLine("[" + FaxLogDate2 + " " + FaxLogTime2 + "], " + FaxLog);
                        m_streamWriter2.Flush();
                        m_streamWriter2.Close();
                        fs2.Close();
                   }
              }
         }
    }
}

Replace:

if (fileAutoFaxLog.LastWriteTime.Date == DateTime.Today.Date) 
{  
      filedatetimeisthesame = true;
       // file exists and it has today's date, so just append to file
       FileStream fs2 = new FileStream(@"C:\Faxserver\Logs\AutofaxLog_" + DateTime.Now.ToString("dd") + ".txt", FileMode.Append, FileAccess.Write);
       StreamWriter m_streamWriter2 = new StreamWriter(fs2);
       m_streamWriter2.WriteLine("[" + DateTime.Now.ToString() + "], " + FaxLog);
       m_streamWriter2.Flush();
       m_streamWriter2.Close();
       m_streamWriter2.Dispose();
       fs2.Close();
       fs2.Dispose();
}

Lots of these (15 or so) in the code:

DateTime hbtTime = DateTime.UtcNow.ToUniversalTime();
string hbtMonth = hbtTime.Month.ToString();
if (hbtMonth.Length == 1)
{
     hbtMonth = "0" + hbtMonth;
}
string hbtDay = hbtTime.Day.ToString();
if (hbtDay.Length == 1)
{
     hbtDay = "0" + hbtDay;
}
string hbtYear = hbtTime.Year.ToString();
string hbtHour = hbtTime.Hour.ToString();
if (hbtHour.Length == 1)
{
     hbtHour = "0" + hbtHour;
}
string hbtMinute = hbtTime.Minute.ToString();
if (hbtMinute.Length == 1)
{
     hbtMinute = "0" + hbtMinute;
}
     string hbtSecond = hbtTime.Second.ToString();
if (hbtSecond.Length == 1)
{
      hbtSecond = "0" + hbtSecond;
}
string strHBTtime = hbtMonth + "/" + hbtDay + "/" + hbtYear + ", " + hbtHour + ":" + hbtMinute + ":" + hbtSecond;

Which is really:

string strHBTtime = DateTime.Now.ToString("MM/dd/yy HH:mm:ss");

This isn’t mentioning all the other weird shit in the code, like queues are always:

object whythefuck = queue.Peek();
string eh = whythefuck.ToString();
queue.Dequeue();

Rather than:

string morereadablelesswaste = queue.Dequeue().ToString();

Flags being set in 1,000 places to do the same thing. Every catch is handled the same way, but lots of copy+paste code rather than passing the Message to a method to handle it (better to just repeat ad-nauseum). Re-inventing booleans with lower performance by setting strings to “true” or “false” then comparing those (they never have any other value). No Dipose() or Finalize(). Ever. No comments. Pointless variable names that mean nothing, like tmpStr14. That also brings up: Hungarian notation. Gods, why use it in a strongly-typed language?

What started off as a 20 minute “add a method to email me” is going to be a “rework 4500 lines of code into (probably) 2000, by cleaning up the trash everywhere then adding classes and delegates.” Joy.

Tags: , ,

categories Bug, General

3 Comments

  • By Dan, March 12, 2008 @ 11:27 am

    Commenting on this in segments –

    First, make namespaces and call them with using. Your first replacement block has reset.Dispose(), but you don’t need to (and shouldn’t, by best practices) call anything.Dispose(). If you call a using statement inside your method call/class, the class called by that statement will automatically be disposed when the call ends. Then again, you say memory management is not a real issue with this.

    Also, because I don’t see a /*snip*/ here, I’m assuming this is a complete if statement?

    if (totalSeconds % 3600 == 0)
    {
    if (totalSeconds == 0)
    {
    log.Rotate(fileErrorLog);
    reset.Labels(true, true);
    reset.Counts(true, true);
    }
    else
    {
    reset.Labels(false, true);
    reset.Labels(false, true);
    }
    reset.Dispose();
    }

    If it is, you can use && (as you used before) to save a bit on code complexity.

    The various lblXXXX and txtXXXX methods in Form1 would probably be worthy of inclusion on the Daily WTF, because he’s yet again reinventing the wheel. TextBox1Add(string), really? Not TextBox1.Text = TextBos1.Text.ToString() + string? I guess he doesn’t know what boxing is, as we talked about yesterday.

  • By Ryan, March 12, 2008 @ 12:37 pm

    It’s my understanding from MSDN that

    using (
}

    Is essentially the same as:

    try {
}
finally {
thing.dispose();
}

    I’ve heard numerous complaints about Stream(Writer/Reader), SqlConnection, MailMessage, and a few others slowly hogging memory unless you explicitly dispose() them, since they’re unmanaged objects.  

    In any case, I don’t see that it needs a new namespace, but then, I haven’t read C# Best Practices. By Python/C++ reasoning, since there ought not to be any duplication of method/class/variable names in the class, a new namespace isn’t necessary. Any particular reason why I should use a namespace for it? 

    Yes, that’s a complete if. I could use &&, but it seems more readable without. 

    All the lblXXXX and txtXXXX methods are fucking delegates. Actually, the only delegates he had in the code.

    public delegate void TextlblDisplayTodayAddCallback(string textdisplaytodayadd);
private void TextlblDisplayTodayAdd(string textdisplaytodayadd)
{
     try
     {
          if (lblFaxesTodayDisplay.InvokeRequired)
          {
               TextlblDisplayTodayAddCallback d = new
               TextlblDisplayTodayAddCallback(TextlblDisplayTodayAdd);
     Invoke(d, new object[] { textdisplaytodayadd });
          }
          else
          {
               lblFaxesTodayDisplay.Text = textdisplaytodayadd;
          }
     }
     catch (Exception f)
     {
     MessageBox.Show("Error : " + f.Message);
     }
}

    Presumably this avoids deadlock conditions or… something. I can’t even tell what the fuck the point of it is. A look on MSDN tells me that Windows forms are not threadsafe, and this is supposed to help that (since these are sometimes called from outside threads), but it seems kludgy. I haven’t looked into replacing these yet. Still about halfway through the code trying to de-spaghettify it. 

    Tons of these around, too:

    if (textBox1.Text == "")
{
     TextbtnStartadd("The program is missing the ini file.  You are not allowed to start faxing.");
}
else
{
     Textbox1Append("\r\nThe program is missing the ini file.  You are not allowed to start faxing.");
}

    Some with slightly varying syntax, but they’re all just a waste of code.

  • By Missy, June 23, 2008 @ 12:42 pm

    WTB more blogs please. And in English.

Other Links to this Post

RSS feed for comments on this post. TrackBack URI

Leave a comment

You must be logged in to post a comment.