A couple years back we had this programmer.  New to us, but not a newbie.  Understood software reasonably, but not as good with the hardware side of things.  Let's call him D.

Well, we are a hardware company.  We make pressure sensors and the corresponding software.  We also have to test the hardware, so we have a test rig and testing software that exercises the electronics.

For some background, our sensors are thin flexible sheets that insert into a handle which contains the electronics.  Of course, the handle has to check for proper alignment.  Two contacts on the sensor are wired together.  If the electronics in the handle detect continuity, we assume good sensor insertion.

Of course, the test rig can close or open the circuit between these contacts to verify the handle under test detects both aligned and unaligned states.

So on to our story.
D was tasked with changing the test application to work with a new set of electronics.  The test procedure itself was nearly unchanged.  He asks me to explain a piece of code:

bool TestAlignment()
{
  bool ok = true;
  for (int i = 0; i < 2; ++i)
  {
    testRig->SetAlignment(i);
    ok = ok && handle->IsAligned() == i;
  }
  return ok;
}

D was confused as to why we would use a for loop.  It took quite a few minutes to convey the idea that there are two states to check.

I might have found myself questioning the for loop as well. It just seems weird having it for that small bit of code, especially without any sort of context. If the 2 was maybe represented as NUMBER_OF_STATES it would have clicked a lot faster (at least for me).

slight optimization

for (int i = 0; (i < 2) && ok; ++i)
  {
    testRig->SetAlignment(i);
    ok &= (handle->IsAligned() == i);
  }

If you fail the first alignment, you might as well not check the second.

I'm confused as to why you would use a for loop here, too, assuming the hard-coded value of 2 is not likely to change in the future. For a value of 2, I would unroll that loop and produce what, in my opinion, is much more readable code:

testRig->SetAlignment(0);
bool ok = handle->IsAligned() == 0;
testRig->SetAlignment(1);
return ok && handle->IsAligned() == 1;

dgvid:

I'm confused as to why you would use a for loop here, too, assuming the hard-coded value of 2 is not likely to change in the future. For a value of 2, I would unroll that loop and produce what, in my opinion, is much more readable code:

testRig->SetAlignment(0);
bool ok = handle->IsAligned() == 0;
testRig->SetAlignment(1);
return ok && handle->IsAligned() == 1;

 

 seconded.

pitchingchris:

..

If you fail the first alignment, you might as well not check the second.

True, but I shortened the example for posting.  The actual code returns the correctness of both checks.  It's useful for production personnel to know just how it failed - it indicates whether it's likely a short or open.

dgvid:

unroll loop

 I agree that, for the example I listed, unrolling the loops makes some sense.  However, the actual is a little bit longer and the duplication is a pain. There is error checking for communicating with the test rig, delay to make sure the handle under test has had time to check the alignment state itself.

 I tried getting to the essence, but maybe lost a little too much.

Vechni:

dgvid:

I'm confused as to why you would use a for loop here, too, assuming the hard-coded value of 2 is not likely to change in the future. For a value of 2, I would unroll that loop and produce what, in my opinion, is much more readable code:

testRig->SetAlignment(0);
bool ok = handle->IsAligned() == 0;
testRig->SetAlignment(1);
return ok && handle->IsAligned() == 1;

 

 seconded.

 

Thirded.  This is a test case after all, not optimized code. 

pitchingchris:

for (int i = 0; (i < 2) && ok; ++i)
  {
    testRig->SetAlignment(i);
    ok &= (handle->IsAligned() == i);
  }

If you fail the first alignment, you might as well not check the second.

Yes. Not like you're doing it though.

 

DogmaBites:

ok = ok && handle->IsAligned() == i;

It's been a long while since I touched C++, but was is this supposed to do?

ok = ok && handle->IsAligned();

I can understand, but what does the "== i" do? 

billhead:

DogmaBites:

ok = ok && handle->IsAligned() == i;

It's been a long while since I touched C++, but was is this supposed to do?

ok = ok && handle->IsAligned();

I can understand, but what does the "== i" do? 

 

 It is a comparison operator that returns a boolean, which is then checked against ok and that boolean is then assigned to the variable.

billhead:

DogmaBites:

ok = ok && handle->IsAligned() == i;

It's been a long while since I touched C++, but was is this supposed to do?

ok = ok && handle->IsAligned();

I can understand, but what does the "== i" do? 

My guess is IsAligned() returns an integer value, probably the get counterpart to SetAlignment (since he passes i to that). Thus he has to compare to an integer value, and he compares it to the value he just assigned to it. Why it's not GetAlignment(), I don't know.

aquanight:

billhead:

DogmaBites:

ok = ok && handle->IsAligned() == i;

It's been a long while since I touched C++, but was is this supposed to do?

ok = ok && handle->IsAligned();

I can understand, but what does the "== i" do? 

My guess is IsAligned() returns an integer value, probably the get counterpart to SetAlignment (since he passes i to that). Thus he has to compare to an integer value, and he compares it to the value he just assigned to it. Why it's not GetAlignment(), I don't know.

 

 It probably returns a boolean.  Assuming of course that something is either aligned or not.

I think that the question is more or less valid.

Resorting to unusual techniques like this should flag that the structure is wrong.

I would favour the following:

bool TestAlignment()
{
  bool ok = TestAlignment(0);
  ok &&= TestAlignment(1);
  return ok;
}

bool TestAlignment(int i)
{
    testRig->SetAlignment(i);
    return (handle->IsAligned() == (bool)i);
}

GettinSadda:

I would favour the following:

bool TestAlignment()
{
  bool ok = TestAlignment(0);
  ok &&= TestAlignment(1);
  return ok;
}

 

Now get rid of the useless variable, and we are done.

bool TestAlignment()
{
  return TestAlignment(0) && TestAlignment(1);
}

 Funroll Loops!

DogmaBites:

 I agree that, for the example I listed, unrolling the loops makes some sense.  However, the actual is a little bit longer and the duplication is a pain. There is error checking for communicating with the test rig, delay to make sure the handle under test has had time to check the alignment state itself.

 

And your immediate reaction was to write a hardcoded 2-iteration loop instead of a subroutine?

ammoQ:

GettinSadda:

I would favour the following:

bool TestAlignment()
{
  bool ok = TestAlignment(0);
  ok &&= TestAlignment(1);
  return ok;
}

 

Now get rid of the useless variable, and we are done.

bool TestAlignment()
{
  return TestAlignment(0) && TestAlignment(1);
}

For the love of God you people have over complicated this.. I'm not sure if you are joking or not, but here we go..

 

bool TestAlignmen()
{
    return testRig->SetAlignment(true) && handle->IsAligned() && !testRig->SetAlignment(false) && !handle->IsAligned();
}

ammoQ:

GettinSadda:

I would favour the following:

bool TestAlignment()
{
  bool ok = TestAlignment(0);
  ok &&= TestAlignment(1);
  return ok;
}

 

Now get rid of the useless variable, and we are done.

bool TestAlignment()
{
  return TestAlignment(0) && TestAlignment(1);
}

 

Yeah, but I left that in so that it could be extended to values of (2) (3) (4) etc.

 

morbiuswilters:

bool TestAlignmen()
{
    return testRig->SetAlignment(true) && handle->IsAligned() && !testRig->SetAlignment(false) && !handle->IsAligned();
}

 

Here you are assuming that testRig->SetAlignment returns the parameter. The OP gives no indication that this assumptions holds.

ammoQ:

morbiuswilters:

bool TestAlignmen()
{
    return testRig->SetAlignment(true) && handle->IsAligned() && !testRig->SetAlignment(false) && !handle->IsAligned();
}

 

Here you are assuming that testRig->SetAlignment returns the parameter. The OP gives no indication that this assumptions holds.

True, but it's not like it's that difficult to make it do that.  This is the more difficult case because if it returns true on successful setting or void then it's easy to just anticipate that value and use it. 

ammoQ:

Now get rid of the useless variable, and we are done.

bool TestAlignment()
{
  return TestAlignment(0) && TestAlignment(1);
} 

Nice code; the only problem is that it isn't correct. In the original post, the SetAlignment() procedure was called for both 0 and 1; your code may skip the second call.

IMil:

ammoQ:

Now get rid of the useless variable, and we are done.

bool TestAlignment()
{
  return TestAlignment(0) && TestAlignment(1);
} 

Nice code; the only problem is that it isn't correct. In the original post, the SetAlignment() procedure was called for both 0 and 1; your code may skip the second call.

 

That can be corrected easily:

 bool TestAlignment(int i)
{
    static bool ok;
    bool result;
    testRig->SetAlignment(i);
    result = (handle->IsAligned() == (bool)i);
    if (i==1) {
        result &&=ok;
    }
    else {
        ok = result;
        result = true;
    }
    return result;
}

 I think that everyone is missing the point of the WTF.

 This all could have been avoid if the code looked like this:

bool TestAlignment()
{
  bool ok = true;

   //there are two states to check, therefore we loop through twice

  for (int i = 0; i < 2; ++i)
  {
    testRig->SetAlignment(i);
    ok = ok && handle->IsAligned() == i;
  }
  return ok;
}

 

Aaron:

DogmaBites:

 I agree that, for the example I listed, unrolling the loops makes some sense.  However, the actual is a little bit longer and the duplication is a pain. There is error checking for communicating with the test rig, delay to make sure the handle under test has had time to check the alignment state itself.

 

And your immediate reaction was to write a hardcoded 2-iteration loop instead of a subroutine?

Why are you assuming I am the original author of the test code?  My purpose of posting the story was the surprise at someone else's difficulty of understanding code that seemed obvious to me.  Nowhere did I state that I wrote it.  If you think the purpose of the code was not obvious, then this may not be a real WTF. 

Come on guys, we're better than this. All I see is terrible assumptions. Can't anybody reformat the code without changing its functionality?

bool TestAlignment()
{
	testRig->SetAlignment(0);
	if (handle->IsAligned() != 0)
			return false;
	
	testRig->SetAlignment(1);
	if (handle->IsAligned() != 1)
			return false;
	
	return true;
}

Or the loop variety:

const int ALIGNMENTS = 2;

bool TestAlignement()
{
	for (i = 0; i < ALIGNMENTS; i++)
	{
		testRig->SetAlignment(i);
		if (handle->IsAligned() != i)
			return false;
	}
	return true;
}

Faxmachinen:

Come on guys, we're better than this. All I see is terrible assumptions. Can't anybody reformat the code without changing its functionality?

 

Apparantly, not even you can do that. Unlike the original post, both of your versions never call testRig->SetAlignment(1) if the first test fails.

DogmaBites:

bool TestAlignment()
{
  bool ok = true;
  for (int i = 0; i < 2; ++i)
  {
    testRig->SetAlignment(i);
    ok = ok && handle->IsAligned() == i;
  }
  return ok;
}

D was confused as to why we would use a for loop.  It took quite a few minutes to convey the idea that there are two states to check.

 

Well, this wouldn't probably be considered a fairly confusing snippet of code in the software engineering world.  Using 0 and 1 instead of defining constants that represent those states is usually considered bad practice.  1 and 0 are magic numbers that could mean anything, but ALIGNED and UNALIGNED is much more unabigious, or maybe a boolean if it's the sort of thing you're sure will never have more than two states.  It's also kind of a strange use of a for loop, but that might make sense in context.

ammoQ:

Apparantly, not even you can do that.

Indeed. Let's try again.

bool TestAlignment()
{
	testRig->SetAlignment(0);
	if (handle->IsAligned() != 0)
	{
		testRig->SetAlignment(1); // TODO: Check if this is necessary
		return false;
	}

	testRig->SetAlignment(1);
	if (handle->IsAligned() != 1)
			return false;
	
	return true;
}

DogmaBites:

Why are you assuming I am the original author of the test code?  My purpose of posting the story was the surprise at someone else's difficulty of understanding code that seemed obvious to me.  Nowhere did I state that I wrote it.  If you think the purpose of the code was not obvious, then this may not be a real WTF. 

 

Okay, chill - maybe you didn't write it but you implied that it was perfectly logical to use a loop for two essentially discrete operations and had to explain this to someone who found it awkward and/or confusing.  If I'd been handed down this code from a 3rd party and somebody asked me why it used a loop, my response probably would have been "Good point, it does seem pretty silly here."

Faxmachinen:

ammoQ:

Apparantly, not even you can do that.

Indeed. Let's try again.

bool TestAlignment()
{
	testRig->SetAlignment(0);
	if (handle->IsAligned() != 0)
	{
		testRig->SetAlignment(1); // TODO: Check if this is necessary
		return false;
	}

	testRig->SetAlignment(1);
	if (handle->IsAligned() != 1)
			return false;
	
	return true;
}

 

 

Which seems to me far more complicated than the original!

Here's mine, for the record: 

 
bool TestAlignment()
{

    testRig->SetAlignment(0); ok = !handle->isAligned();

    testRig->SetAlignment(1);

    return ok && handle->IsAligned();

}

I see no reason for the loop either in this particular case.

 Using a loop like this should never pass a code review. Write a fucking function asshole.