Page 1 of 1

You think you have got it bad?

Posted: Tue Oct 18, 2005 3:20 pm
by Peijen
Here is some code I was just looking at. Why do people feel a need to nest if statments when they dont have to? What's wrong with using 'return' at the end of if block? Why the hell am I still in Arkansas?

Code: Select all

			//Description Field
			if ((t1.Text.Trim () == ""))
			{
				t1.BorderColor = Color.Red;
				t1.Text = null;
				xMenu.SetError("Please Enter Description.");
         }
			else
			{
				t1.BorderColor = Color.White;
				// Account Number
				if((t7.Text.Trim () == "" || ! validateAccountNumber(t7.Text)))
				{	
					t7.BorderColor = Color.Red;
					t7.Text = null;
					xMenu.SetError("Please Enter a Vaild Account Number.");
            }
				else
				{
					t7.BorderColor = Color.White;
					//Agent Field
					if ((t7.Text == "1200" || t7.Text == "1201") && ((t3.Text.Trim () == "") || IsNumeric(t3.Text) == false))
					{
						t3.BorderColor = Color.Red;
						t3.Text = null;
						xMenu.SetError("Please Enter Agent Name.");
               }
					else
					{
						t3.BorderColor = Color.White;
						//Agent Account Field
						if (t3.Text.Trim ()!="" && ((t4.Text.Trim () == "") || t4.Text.Length != 2 || IsNumeric(t4.Text) == false))
						{
							t4.BorderColor = Color.Red;
							t4.Text = null;
							xMenu.SetError("Both Agent and Valid Agent Account is required.");
                  }
						else
						{
							t4.BorderColor = Color.White;
							//Misc Codes Field
							if((t5.Text.Trim () == "" || !validateAgentAccountMiscTypeCodes(t5.Text)))
							{	
								t5.BorderColor = Color.Red;
								t5.Text = null;
								xMenu.SetError("Please Enter a Vaild MISC Type.");
                     }
							else
							{
								t5.BorderColor = Color.White;
								// Amount
								if((t8.Text.Trim () == "") || IsNumeric(t8.Text) == false || !(Double.Parse(t8.Text) > 0.0))
								{
									t8.BorderColor = Color.Red;
									t8.Text = null;
									GiveMsg ("Please Enter a Valid Amount (> 0.00).",0); 	}
								else
								{
									t8.BorderColor = Color.White;
									// Policy
									if((t5.Text == "01" || t5.Text == "02") && (t6.Text.Trim () == ""))
									{
										t6.BorderColor = Color.Red;
										t6.Text = null;
										GiveMsg ("Policy Number Cannot be Null when Pay Type Code is 01 or 02.",0); 	}
									else
									{
										t6.BorderColor = Color.White;

										ArrayList upvalues = new ArrayList ();
										upvalues.Add (e.Item.Cells [0].Text);  //Voucher
										upvalues.Add (e.Item.Cells [1].Text);  //Item No
										upvalues.Add (e.Item.Cells [2].Text); //Entry
										upvalues.Add (t1.Text); //Description
										upvalues.Add (t2.Text); //Center
										upvalues.Add (t3.Text); //Agent
										upvalues.Add (t4.Text); //Agt
										upvalues.Add (t5.Text); //MType
										upvalues.Add (t6.Text); //Policy
										upvalues.Add (dd1.SelectedValue); //D/C
										upvalues.Add (t7.Text); //Account
										upvalues.Add (Double.Parse (t8.Text).ToString("0.00")); //Amount
										upvalues.Add (e.Item.Cells [15].Text); //Type
										upvalues.Add (e.Item.Cells [16].Text); // Com
										upvalues.Add (e.Item.Cells [17].Text); // Dept
										upvalues.Add (e.Item.Cells [14].Text); // Name
			
										SessionVaribles.AppGlobal.DGEditMode = "F";
										DataGrid1.EditItemIndex = -1;
										System.Web.UI.WebControls.LinkButton lblStat = (System.Web.UI.WebControls.LinkButton)e.Item.Cells[18].Controls[0];
										if(lblStat.Text=="Insert")
										{ act = "add"; }
										else
										{ act = "updt"; }

										if (e.Item.Cells [15].Text == "99" || e.Item.Cells[15].Text == "99" || SessionVaribles.VoucherSystem.status == "3")
										{
											GiveMsg ("History Vouchers Cannot be Updated.",0);
											DataGrid1.CurrentPageIndex = 0;
											DataGrid1.DataSource = SessionVaribles.VoucherSystem.VoucherEntries;
											DataGrid1.DataBind ();
										}
										else
										{
											try
											{
												TWM.Common.DataServices.VOUCHER.RECORDHEADER upvrec = changeVDesc ( act,"ORMVOUVF","99", upvalues);
												if (Convert.ToInt32 (upvrec.ReturnCode) == 0)
												{
													GiveMsg ("Record Successfully Added.",1);
													SessionVaribles.VoucherSystem.VoucherEntries = VoucherTable (bfwdVDesc ("bfwd","ORMVOUVF","99", e.Item.Cells [16].Text, "-1") , "-1");
													DataGrid1.CurrentPageIndex = 0;
													DataGrid1.DataSource = SessionVaribles.VoucherSystem.VoucherEntries;
													DataGrid1.DataBind ();
													ddlStatus.SelectedValue = "1";
												}
												else
												{
													GiveMsg(upvrec.Message,0);
												}
											}
											catch (Exception ee)
											{
												GiveMsg (ee.Message,0);
											}
										}

									} } } } } } 
			}

Posted: Tue Oct 18, 2005 3:50 pm
by VLSmooth
I've actually written something similar once, however it used a retVal for an error code.

I usually see this pattern when a single exit point is desired, usually for clean-up code in the event of a failure. Error codes are propagated via the retVal local variable to further assist debugging.

Ways to mitigate extreme nesting are macros and breaking code into multiple functions, both with their obvious advantages and disadvantages.

However, from you provided, that seems to be a void method with no clean-up...

Posted: Tue Oct 18, 2005 3:57 pm
by Peijen
What's wrong with (see code). Because they are functionally the same and you don't have 5000 level of nested ifs.

Code: Select all

if (conditionA)
{
  // do stuff
  return A;  // or retVal = A;
}

if (conditionB)
{
  // do stuff
  return B; // or retVal += B;
}

//etc

Posted: Tue Oct 18, 2005 4:43 pm
by VLSmooth
Note: My situation was different than yours.

Multiple exit points require redundant code for clean-up of structures / etc. In the following code, clean-up code exists in one place regardless of nesting and is easy to modify.

Example:

Code: Select all

ERROR method_name(type1 arg1,
                  type2 arg2)
{
  if (conditionA)
  {
    retVal = codeA();
    if (retVal == SUCCESS)
    {
      retVal = codeB();
      if (retVal == SUCCESS)
      {
        retVal = codeC();
      }
    }
  }

  // clean-up methods
  cleanUp();

  // clean up non-null pointers
  if (!attribute1 ||
      !attribute2 ||
      !attribute3)
  {
    // note: delete on null is effectively a no-op
    delete attribute1;
    delete attribute2;
    delete attribute3;
  }

  return retVal;
}

Posted: Tue Oct 18, 2005 5:01 pm
by Peijen
That is why .NET/Java has exception handling and people are encourage to use them over error code.

While your situation is different and makes more sense than the code I posted I would still argue code with heavy nestes if is designed badly.

Posted: Tue Oct 18, 2005 5:03 pm
by VLSmooth
Relevant tidbit from here.
Kaz Kylheku wrote:With multiple exit points, the cleanup code must be diligently
reproduced before each return. Forgetting but one leads to
an error. The larger the function and the more exit points
it has, the more error prone it is to maintain it (until it
becomes so large and error prone, that it's error prone-ness
becomes legendary and every programmer in the organization
treats it with caution!)

The problem exists because C doesn't have any way of specifying
a common block of cleanup code to be done when a function
terminates. The closest approximation is to replace the
return statements with goto statements that branch forward
to some cleanup code at the end of the function.
Of course, it's just a guideline. If the readability gained from multiple exit points outweights the downsides of redundant code (if any), use it by all means. Well, unless your company has a strong coding standard.

Posted: Tue Oct 18, 2005 5:06 pm
by VLSmooth
Agreed exceptions help a lot. Unfortunately not all platforms have them, which can be constraining. Single exit points are apparently an artifact from Dijkstra's structured programming; back in the GOTO days.

Posted: Tue Oct 18, 2005 5:09 pm
by Peijen
Here I came up with clean code for you in your situation.

Code: Select all

ERROR method_name(args)
{
  retVal = codeA();
  if (retVal != SUCCESS) { return retVal; }

  retVal = codeB();
  if (retVal != SUCCESS) { return retVal; }

  // more shit
}

ERROR safe_wrapper(args)
{
  retVal = method_name(args);

  // cleanup code

  return retVal;
}
of course if you code in .NET you can do

Code: Select all

try
{
  retVal = codeA();
  if (retVal != SUCCESS) { return retVal; }

  retVal = codeB();
  if (retVal != SUCCESS) { return retVal; }

  // more shit
}
finally
{
  // clean up code
}

Posted: Tue Oct 18, 2005 5:42 pm
by Jonathan

Code: Select all

} else { if {
You're right, that is retarded.

Code: Select all

} else if {
That is a gajillion times better.

Posted: Mon Mar 27, 2006 3:01 pm
by Dave

Code: Select all

if [coding] {Sleep}

Posted: Thu Mar 30, 2006 12:44 am
by quantus
Kaz Kylheku wrote:The larger the function and the more exit points
it has, the more error prone it is to maintain it (until it
becomes so large and error prone, that it's error prone-ness
becomes legendary and every programmer in the organization
treats it with caution!)
I think my new goal while working here will be to come legendary by way of writing legendary code. That's kinda what the guy before me did and we still talk about him...

Posted: Thu Mar 30, 2006 1:14 am
by George
Yeah, we had a contractor who worked on the program for a year and a half. During that time, he produced no useable code. Unfortunately, all of his code has been deleted, so his legend will fade and die. The trick is to make your horrible code indispensible so that none dare disturb it. That is the path to immortality.

Posted: Thu Mar 30, 2006 2:50 am
by quantus
Oh, that's easy here. Anyone could do it.

Hrmmm, if it weren't for y2k, a whole lot of people who wrote cobol would be immortal. Or maybe y2k just brought them out of obscurity?