5
\$\begingroup\$

I'm writing GUI controls and there are many places where there are many nested ifs checking for some result.

function TMyObject.GetCursor: TCursor;
begin
  if CanDragX then
  begin
    if CanDragY then
      Result := crSizeAll
    else
      Result := crSizeWE;
  end
  else if CanDragY then
    Result := crSizeNS
  else
  if CanClick then
    Result := crHandPoint
  else
    Result := crArrow;
end;

How would you format/rewrite this code?

\$\endgroup\$
5
\$\begingroup\$

I would do this:

function TMyObject.GetCursor: TCursor;
begin
  if CanDragX and CanDragY then
    Result := crSizeAll
  else if CanDragX then
    Result := crSizeWE
  else if CanDragY then
    Result := crSizeNS
  else
    if CanClick then
      Result := crHandPoint
    else
      Result := crArrow;
end;

But really it is a matter of style and what is most readable to you (and the person who will maintain it).

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I know this is a trivial case, but maybe I'm overlooking something, thats why I asked. \$\endgroup\$ – Kromster says support Monica Aug 8 '11 at 8:03
  • \$\begingroup\$ @Krom The only change I'd suggest to this answer is to put the "if CanClick then" on the same line with the preceeding "else" and adjust the following indentation accordingly. Like Martin said, it's just a matter of style. \$\endgroup\$ – jimreed Aug 8 '11 at 12:27
4
\$\begingroup\$

I'm not sure of what your language is. Instead of assigning to Result, I prefer mid-function returns in a case like this; then I don't need else.

function TMyObject.GetCursor: TCursor;
begin
  if CanDragX && CanDragY then return crSizeAll;
  if CanDragX then return crSizeWE;
  if CanDragY then return crSizeNS;
  if CanClick then return crHandPoint;
  return crArrow;
end;

If that works in your language (VB?), I think that's much cleaner.

\$\endgroup\$
  • 3
    \$\begingroup\$ It's Delphi, unfortunately there was no Delphi tag prior my question, I should have added that. In Delphi2009+ it would be "Exit (crSizeAll)" instead of "return crSizeAll". Thats a nice idea, unfortunately it does not work in Delphi7. +1 \$\endgroup\$ – Kromster says support Monica Aug 9 '11 at 5:11
2
\$\begingroup\$

As there are only 8 possible results, you can put them into mapping array. Then function itself will be one-liner only.

const
  CursorMap: array [boolean, boolean, boolean] // CanClick, CanDragX, CanDragY
    of TCursor =               // see CanClick, CanDragX indexes values below:
    (((crArrow, crSizeNS),     // false, false
      (crSizeWE, crSizeAll)),  // false, true
     ((crHandPoint, crSizeNS), // true, false
      (crSizeWE, crSizeAll))); // true, true


function TMyObject.GetCursor: TCursor;
begin
  Result:= CursorMap[CanClick, CanDragX, CanDragY];
end;
\$\endgroup\$
  • \$\begingroup\$ Clever way to make the function itself a single line. But you've moved all the complexity into CursorMap, and the need for comments should be proof enough that this is a bad idea. As you add more variables, the problem simply gets worse. \$\endgroup\$ – Disillusioned Jun 23 '12 at 11:08
  • 2
    \$\begingroup\$ @CraigYoung, I just told that there is such way also. May it'll be useful for somebody. My personal favorite answer is with returns, by @CarlManaster \$\endgroup\$ – Serhii Kheilyk Jun 23 '12 at 15:20
1
\$\begingroup\$

In a method like this I like to make it clear up front that the default Result is crArrow and the rest of the routine is simply considering situations in which the default would be insufficient.

function TMyObject.GetCursor: TCursor;
begin
  Result := crArrow;

  if CanDragX and CanDragY then Result := crSizeAll
  else if CanDragX then Result := crSizeWE
  else if CanDragY then Result := crSizeNS
  else if CanClick then Result := crHandPoint;
end;

This is somewhat similar to Carl's answer, but compensating for the fact that Delphi does not provide a single operation to both set Result and return.

A key difference to note here is that it is very important to be aware of the impact of else if vs simple if. For example, if you reverse the order of the checks, then you have to implement as follows:

if CanClick then Result := crHandPoint;
if CanDragY then Result := crSizeNS;
if CanDragX then Result := crSizeWE;
if CanDragX and CanDragY then Result := crSizeAll;

A more direct translation of Carl's answer might prove a little less error prone, albeit more verbose:

if CanDragX and CanDragY then
begin
  Result := crSizeAll;
  Exit;
end;
if CanDragX then
begin
  Result := crSizeWE;
  Exit;
end;
...

Even then I suspect mistakes would be easy to make.
Although I dislike both Sunny's and Sergiy's answers because they need comments for clarification, there's a lot to be said for explicitly mapping each permutation. Consider the following pseudo-code:

case [DragX, DragY, Click] of
  [CanDragX, CanDragY, CanClick] : Result := crSizeAll;
  [CanDragX, CanDragY, NotClick] : Result := crSizeAll;
  [CanDragX, NotDragY, CanClick] : Result := crSizeWE;
  [CanDragX, NotDragY, NotClick] : Result := crSizeWE;
  [NotDragX, CanDragY, CanClick] : Result := crSizeNS;
  [NotDragX, CanDragY, NotClick] : Result := crSizeNS;
  [NotDragX, NotDragY, CanClick] : Result := crHandPoint;
  [NotDragX, NotDragY, NotClick] : Result := crArrow;
end;

It would be wonderful if Delphi's enums and sets supported such syntax, but similar can be achieved using a self-documenting variation of Sunny's answer:

const
  cCannotDo = 0;
  cCanDragX = 1;
  cCanDragY = 2;
  cCanClick = 4;
var
  LMouseOptions: Integer;
begin
  LMouseOptions :=  Ord(CanDragX)*cCanDragX +
                    Ord(CanDragY)*cCanDragY +
                    Ord(CanClick)*cCanClick;
  case LMouseOptions of
    cCanDragX + cCanDragY + cCanClick : Result := crSizeAll;
    cCanDragX + cCanDragY + cCannotDo : Result := crSizeAll;
    cCanDragX + cCannotDo + cCanClick : Result := crSizeWE;
    cCanDragX + cCannotDo + cCannotDo : Result := crSizeWE;
    cCannotDo + cCanDragY + cCanClick : Result := crSizeNS;
    cCannotDo + cCanDragY + cCannotDo : Result := crSizeNS;
    cCannotDo + cCannotDo + cCanClick : Result := crHandPoint;
    cCannotDo + cCannotDo + cCannotDo : Result := crArrow;
  else
    //Either raise error or return crArrow as default
  end;
end;

However, the permutations can get unwieldy as more flags and even multi-value options are added. So personally I would go for my first option, and implement unit tests to run through all the cases of interest.

\$\endgroup\$
0
\$\begingroup\$

Using bitwise OR, and assuming this is some form of Pascal, but same applies to any languiage:

VAR:
   res : integer

res := (ord(CanClick) * 4) OR (ord(CanDragY) * 2) OR ord(CanDragX);
case res of
   0: Result := crArrow;      // 0 0 0 
   1, 5: Result := crSizeWE;  // 0 0 1 or 1 0 1
   2, 6: Result := crSizeNS;  // 0 1 0  or 1 1 0
   3, 7: Result := crSizeAll; // 0 1 1  or 1 1 1
   4: Result := crHandPoint;  // 1 0 0
end;
\$\endgroup\$
  • 2
    \$\begingroup\$ The need for comments should be an alarm bell screaming: Houston we have a problem! \$\endgroup\$ – Disillusioned Jun 23 '12 at 11:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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