You think you have got it bad?

Posts you want to find years later go here.
Post Reply
Peijen
Minion to the Exalted Pooh-Bah
Posts: 2790
Joined: Fri Jul 18, 2003 2:28 pm
Location: Irvine, CA

You think you have got it bad?

Post 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);
											}
										}

									} } } } } } 
			}

VLSmooth
Tenth Dan Procrastinator
Posts: 3055
Joined: Fri Jul 18, 2003 3:02 am
Location: Varies
Contact:

Post 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...

Peijen
Minion to the Exalted Pooh-Bah
Posts: 2790
Joined: Fri Jul 18, 2003 2:28 pm
Location: Irvine, CA

Post 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

VLSmooth
Tenth Dan Procrastinator
Posts: 3055
Joined: Fri Jul 18, 2003 3:02 am
Location: Varies
Contact:

Post 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;
}

Peijen
Minion to the Exalted Pooh-Bah
Posts: 2790
Joined: Fri Jul 18, 2003 2:28 pm
Location: Irvine, CA

Post 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.

VLSmooth
Tenth Dan Procrastinator
Posts: 3055
Joined: Fri Jul 18, 2003 3:02 am
Location: Varies
Contact:

Post 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.

VLSmooth
Tenth Dan Procrastinator
Posts: 3055
Joined: Fri Jul 18, 2003 3:02 am
Location: Varies
Contact:

Post 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.

Peijen
Minion to the Exalted Pooh-Bah
Posts: 2790
Joined: Fri Jul 18, 2003 2:28 pm
Location: Irvine, CA

Post 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
}

Jonathan
Grand Pooh-Bah
Posts: 6722
Joined: Tue Sep 19, 2006 8:45 pm
Location: Portland, OR
Contact:

Post by Jonathan »

Code: Select all

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

Code: Select all

} else if {
That is a gajillion times better.

Dave
Tenth Dan Procrastinator
Posts: 3483
Joined: Fri Jul 18, 2003 3:40 pm

Post by Dave »

Code: Select all

if [coding] {Sleep}
It takes 43 muscles to frown and 17 to smile, but it doesn't take any to just sit there with a dumb look on your face.

quantus
Tenth Dan Procrastinator
Posts: 4891
Joined: Fri Jul 18, 2003 3:09 am
Location: San Jose, CA

Post 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...
Have you clicked today? Check status, then: People, Jobs or Roads

George
Veteran Doodler
Posts: 1267
Joined: Sun Jul 18, 2004 12:26 am
Location: Arlington, VA

Post 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.
I feel like I just beat a kitten to death... with a bag of puppies.

quantus
Tenth Dan Procrastinator
Posts: 4891
Joined: Fri Jul 18, 2003 3:09 am
Location: San Jose, CA

Post 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?
Have you clicked today? Check status, then: People, Jobs or Roads

Post Reply