How to improve this code? (too many if)

Question

I want to print the border of the square... It may print only one side, or more sides of the square, so , I write this method

printBorder(N, E, S, W) {
  if (N) {
     square.printBorder(0,0,0,10);
  }
  if (E) {
     square.printBorder(0,10,10,10);
  }
  if (S) {
     square.printBorder(10,0,10,10);
  }
  if (W) {
     square.printBorder(0,0,10,0);
  }
}

It can work fine, but I think it is not so elegant, it is too many if.... and all statement is more or less the same... ...I think it should have a way to simplify this codes, any suggestion?

Answer 1

One way of simplifying it... make calls even if you don't need them, but conditionalise the implementation:

printBorder(N, E, S, W){
  square.printBorder(n, 0,0,0,10);
  square.printBorder(e, 0,10,10,10);
  square.printBorder(s, 10,0,10,10);
  square.printBorder(w, 0,0,10,0);
}

Then in Square (or whatever):

printBorder(condition, top, left, bottom, right) {
  if (!condition) {
    return;
  }
  printBorder(top, left, bottom, right);
}

A similar alternative would be to keep the conditional printBorder with the original function:

printBorder(N, E, S, W){
  printBorder(n, 0,0,0,10);
  printBorder(e, 0,10,10,10);
  printBorder(s, 10,0,10,10);
  printBorder(w, 0,0,10,0);
}

printBorder(condition, top, left, bottom, right) {
  if (!condition) {
    return;
  }
  square.printBorder(top, left, bottom, right);
}

Answer 2

I wouldn't care about the ifs. I'd just make it more readable:

printBorder(N, E, S, W){
  if(N) square.printBorder( 0,  0,  0, 10);
  if(E) square.printBorder( 0, 10, 10, 10);
  if(S) square.printBorder(10,  0, 10, 10);
  if(W) square.printBorder( 0,  0, 10,  0);
}

Answer 3

Personally, I really like binary comparisons.

const uint NORTH = 1;
const uint SOUTH = 2;
const uint EAST = 4;
const uint WEST = 8;

// ... some code ...
printBorder(NORTH + EAST);
// ... some other code ...

printBorder(uint Sides)
{
   if((NORTH & Sides) > 0) square.printBorder(0, 0, 0, 10);
   if((SOUTH & Sides) > 0) square.printBorder(0, 10, 10, 10);
   if((EAST & Sides) > 0) square.printBorder(10, 0, 10, 10);
   if((WEST & Sides) > 0) square.printBorder(0, 0, 10, 0);
}

Some might say that this makes the code inside the function less readable. However, my thinking is there is only a single occurrence of this function whereas you will be calling this function all over the place. If you're running through some code you haven't looked at in awhile which is more readable?

printBorder(true, false, true, true);

or

printBorder(NORTH + SOUTH + EAST);

Just my opinion. :)

Answer 4

First you are doing ok, this does exactly what it expresses, don't worry about the space that you are using, most of the solutions here just muddy the water.

If you really want to 'do' something look if you can't move the Border parameter into the square. you could move the border padding (10 in your example into the square), possibly also the State which border should be shown, and then just call square.printBorders(). That depends a lot on the context where you are using this.

Answer 5

you did not specify which programming language.

if it were java, enums can provide good readable syntax,type safety, as well as take advantage of the efficient bit-fiddling capabilities of the EnumSet implementation.

alternatively you could also provide a varargs method signature, but then you cannot be sure that your mehtod will be called with printBorder(N,N), which does not really make sense. using the EnumSet interface you have this guarantee.

  public class PrintBorder {

    //this is your method without the if's
    public static void printBorder(EnumSet<Sides> sides) {
        for (Sides side : sides) {
            side.print(square);
        }
    }

    //use it like this
    public static void main(String[] args) {
        printBorder(EnumSet.of(N, E)); //static import here
    }

    //declare an enum for the sides.
    public enum Sides {
        N(0, 0, 0, 10),
        E(0, 10, 10, 10),
        S(10, 0, 10, 10),
        W(0, 0, 10, 0);

        private final int x1;
        private final int y1;
        private final int x2;
        private final int y2;

        Sides(int x1, int y1, int x2, int y2) {
            this.x1 = x1;
            this.y1 = y1;
            this.x2 = x2;
            this.y2 = y2;
        }

        //this method could as well be in the Square class, would be cleaner
        public void print(Square s) {
            s.printBorder(x1, y1, x2, y2);
        }

    }

    //boilerplate here
    private static final Square square = new Square();

    private static class Square {
        public void printBorder(int x1, int y1, int x2, int y2) {
            //do something..
        }
    }
}

Answer 6

How about:

square.printBorder(N|E|W?0:10, N|S|W?0:10, N?0:10, N|E|S?10:0);