Is there any way to make this long if statement more concise?

Question

Here is my long if statement:

   if i % 2 == 1 or i == 0:
       if x1 > x:
           ny = y + ceil(shift * (i / 2))
           ny1 = y1 + ceil(shift * (i / 2))
       elif x1 == x:
           ny = y
           ny1 = y1
       elif x1 < x:
           ny = y - ceil(shift * (i / 2))
           ny1 = y1 - ceil(shift * (i / 2))
       if y1 > y:
           nx = x - ceil(shift * (i / 2))
           nx1 = x1 - ceil(shift * (i / 2))
       elif y1 == y:
           nx = x
           nx1 = x1
       elif y1 < y:
           nx = x + ceil(shift * (i / 2))
           nx1 = x1 + ceil(shift * (i / 2))
   else:
       if x1 > x:
           ny = y - ceil(shift * (i / 2))
           ny1 = y1 - ceil(shift * (i / 2))
       elif x1 == x:
           ny = y
           ny1 = y1
       elif x1 < x:
           ny = y + ceil(shift * (i / 2))
           ny1 = y1 + ceil(shift * (i / 2))
       if y1 > y:
           nx = x + ceil(shift * (i / 2))
           nx1 = x1 + ceil(shift * (i / 2))
       elif y1 == y:
           nx = x
           nx1 = x1
       elif y1 < y:
           nx = x - ceil(shift * (i / 2))
           nx1 = x1 - ceil(shift * (i / 2))
   return nx, ny, nx1, ny1

I'm not sure if it's possible to just convert this whole thing into a simple equation. However, if not, is there any possible way to only have the first large if statement and make the sign in ny = y + ceil(shift * (i / 2)) be plus if i % 2 == 1 or i == 0 and minus else?

What exactly is the intended logic of it? Why do you think you need all these special cases? What do the `i`, `x`, `y`, `x1`, `y1` values represent? Why are you using the `ceil` function plus a bunch of special cases? Are you familiar with the `//` operator? Do you see how it might help you? — Karl Knechtel, Oct 27 '21 at 00:32
can i suggest you break it down by functions? having that long of an if statement is never recommended as it's most prone to edge cases — vladthelad, Oct 27 '21 at 00:40
you can also try out "if not obj < obj2" which should alleviate some of the ongoing if statement — vladthelad, Oct 27 '21 at 00:41
`ceil(shift * (i / 2))` appears many times. Assign that to a variable instead of repeating it. — Barmar, Oct 27 '21 at 00:46

enzo · Accepted Answer · 2021-10-27T01:15:37.293

This guy is your friend. Comparing the main if with its else, we'll see this:

This means that only the ceiling sign changes between branches. We can fix this by adding a new variable:

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1

if x1 > x:
    ny = y + ceil_sign * ceil(shift * (i / 2))
    ny1 = y1 + ceil_sign * ceil(shift * (i / 2))

elif x1 == x:
    ny = y
    ny1 = y1

elif x1 < x:
    ny = y - ceil_sign * ceil(shift * (i / 2))
    ny1 = y1 - ceil_sign * ceil(shift * (i / 2))

if y1 > y:
    nx = x - ceil_sign * ceil(shift * (i / 2))
    nx1 = x1 - ceil_sign * ceil(shift * (i / 2))

elif y1 == y:
    nx = x
    nx1 = x1

elif y1 < y:
    nx = x + ceil_sign * ceil(shift * (i / 2))
    nx1 = x1 + ceil_sign * ceil(shift * (i / 2))

You can also move branches around:

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1

if x1 > x:
    ny = y + ceil_sign * ceil(shift * (i / 2))
    ny1 = y1 + ceil_sign * ceil(shift * (i / 2))

elif x1 < x:
    ny = y - ceil_sign * ceil(shift * (i / 2))
    ny1 = y1 - ceil_sign * ceil(shift * (i / 2))

else:
    ny = y
    ny1 = y1

if y1 > y:
    nx = x - ceil_sign * ceil(shift * (i / 2))
    nx1 = x1 - ceil_sign * ceil(shift * (i / 2))

elif y1 < y:
    nx = x + ceil_sign * ceil(shift * (i / 2))
    nx1 = x1 + ceil_sign * ceil(shift * (i / 2))

else:
    nx = x
    nx1 = x1

This refactoring allow us to see even more differences: the first if and its elif also only changes the ceiling sign. Let's add another variable:

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1

if x1 == x:
    ny = y
    ny1 = y1

else:
    x_ceil_sign = 1 if x1 > x else -1
    ny = y + x_ceil_sign * ceil_sign * ceil(shift * (i / 2))
    ny1 = y1 + x_ceil_sign * ceil_sign * ceil(shift * (i / 2))

if y1 == y:
    nx = x
    nx1 = x1

else:
    y_ceil_sign = 1 if y1 < y else -1
    nx = x + y_ceil_sign * ceil_sign * ceil(shift * (i / 2))
    nx1 = x1 + y_ceil_sign * ceil_sign * ceil(shift * (i / 2))

As @Barmar suggested, note that ceil_sign always appears with the ceil call. You can combine them just once:

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1
ceil_value = ceil_sign * ceil(shift * (i / 2))

if x1 == x:
    ny = y
    ny1 = y1

else:
    x_ceil_sign = 1 if x1 > x else -1
    ny = y + x_ceil_sign * ceil_value
    ny1 = y1 + x_ceil_sign * ceil_value

if y1 == y:
    nx = x
    nx1 = x1

else:
    y_ceil_sign = 1 if y1 < y else -1
    nx = x + y_ceil_sign * ceil_value
    nx1 = x1 + y_ceil_sign * ceil_value

You can do the same move with the internal ceil_signs:

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1
ceil_value = ceil_sign * ceil(shift * (i / 2))

if x1 == x:
    ny = y
    ny1 = y1

else:
    x_ceil_sign = (1 if x1 > x else -1) * ceil_value
    ny = y + x_ceil_sign
    ny1 = y1 + x_ceil_sign

if y1 == y:
    nx = x
    nx1 = x1

else:
    y_ceil_sign = (1 if y1 < y else -1) * ceil_value
    nx = x + y_ceil_sign
    nx1 = x1 + y_ceil_sign

Note that in both clauses, you're adding nothing on the if branch and adding *_ceil_sign in the else branch. Create another variable:

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1
ceil_value = ceil_sign * ceil(shift * (i / 2))

x_factor = 0 if x1 == x else ((1 if x1 > x else -1) * ceil_value)
ny = y + x_factor
ny1 = y1 + x_factor

y_factor = 0 if y1 == y else ((1 if y1 < y else -1) * ceil_value)
nx = x + y_factor
nx1 = x1 + y_factor

As an additional refactoring, note that if a > b, sign(a - b) is equal to 1. You can use this to refactor the *_factor variables:

def sign(x):
    return 0 if x == 0 else 1 if x > 0 else -1

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1
ceil_value = ceil_sign * ceil(shift * (i / 2))

x_factor = 0 if x1 == x else sign(x1 - x) * ceil_value
ny = y + x_factor
ny1 = y1 + x_factor

y_factor = 0 if y1 == y else -sign(y1 - y) * ceil_value
nx = x + y_factor
nx1 = x1 + y_factor

Also note that, if a == b, sign(a - b) is equal to 0, which defines both *_factors:

def sign(x):
    return 0 if x == 0 else 1 if x > 0 else -1

ceil_sign = 1 if i % 2 == 1 or i == 0 else -1
ceil_value = ceil_sign * ceil(shift * (i / 2))

x_factor = sign(x1 - x) * ceil_value
ny = y + x_factor
ny1 = y1 + x_factor

y_factor = -sign(y1 - y) * ceil_value
nx = x + y_factor
nx1 = x1 + y_factor

Can't you also combine `ceil_sign` with the `ceil()` call? – Barmar Oct 27 '21 at 00:50 — Barmar, Oct 27 '21 at 00:50
You're correct, didn't notice that – enzo Oct 27 '21 at 00:54 — enzo, Oct 27 '21 at 00:54

score 2 · Answer 2 · answered Oct 27 '21 at 01:05

Here are some hints on how to approach problems like this generally:

Look for commonly repeated expressions, and name them.

Here, ceil(shift * (i / 2)) happens a lot. Let's name that:

offset = ceil(shift * (i / 2))
if i % 2 == 1 or i == 0:
    if x1 > x:
        ny = y + offset
        ny1 = y1 + offset
    elif x1 == x:
        ny = y
        ny1 = y1
    elif x1 < x:
        ny = y - offset
        ny1 = y1 - offset
    if y1 > y:
        nx = x - offset
        nx1 = x1 - offset
    elif y1 == y:
        nx = x
        nx1 = x1
    elif y1 < y:
        nx = x + offset
        nx1 = x1 + offset
else:
    if x1 > x:
        ny = y - offset
        ny1 = y1 - offset
    elif x1 == x:
        ny = y
        ny1 = y1
    elif x1 < x:
        ny = y + offset
        ny1 = y1 + offset
    if y1 > y:
        nx = x + offset
        nx1 = x1 + offset
    elif y1 == y:
        nx = x
        nx1 = x1
    elif y1 < y:
        nx = x - offset
        nx1 = x1 - offset
return nx, ny, nx1, ny1

Use Python's simultaneous assignment to group related assignments.

This is pretty straightforward:

offset = ceil(shift * (i / 2))
if i % 2 == 1 or i == 0:
    if x1 > x:
        ny, ny1 = y + offset, y1 + offset
    elif x1 == x:
        ny, ny1 = y, y1
    elif x1 < x:
        ny, ny1 = y - offset, y1 - offset
    if y1 > y:
        nx, nx1 = x - offset, x1 - offset
    elif y1 == y:
        nx, nx1 = x, x1
    elif y1 < y:
        nx, nx1 = x + offset, x1 + offset
else:
    if x1 > x:
        ny, ny1 = y - offset, y1 - offset
    elif x1 == x:
        ny, ny1 = y, y1
    elif x1 < x:
        ny, ny1 = y + offset, y1 + offset
    if y1 > y:
        nx, nx1 = x + offset, x1 + offset
    elif y1 == y:
        nx, nx1 = x, x1
    elif y1 < y:
        nx, nx1 = x - offset, x1 - offset
return nx, ny, nx1, ny1

Exploit symmetries and parallels. (The reason I make simple syntax tweaks like the simultaneous assignment first is that, by reducing the line count, we can bring symmetrical things visually closer together, and thus notice symmetry more easily.)

What I mean by a "symmetry" or "parallel" is that we do fundamentally the same thing with different variables (or a related thing with some small, patterned alteration). There are two of these here: first, there's the way that the code depends on the i % 2 == 1 or i == 0. We can quickly see that the effect of this - propagated through all the if/elif logic - is that when the condition is false, offset is subtracted where it would otherwise be added, and vice versa. We can use elementary algebra to simplify that, by simply replacing offset with -offset when the condition isn't met. Thus:

offset = ceil(shift * (i / 2))
if not(i % 2 == 1 or i == 0):
    offset = -offset
if x1 > x:
    ny, ny1 = y + offset, y1 + offset
elif x1 == x:
    ny, ny1 = y, y1
elif x1 < x:
    ny, ny1 = y - offset, y1 - offset
if y1 > y:
    nx, nx1 = x - offset, x1 - offset
elif y1 == y:
    nx, nx1 = x, x1
elif y1 < y:
    nx, nx1 = x + offset, x1 + offset
return nx, ny, nx1, ny1

Make algebraic simplifications.

Pattern recognition is useful here. We are doing three-way comparisons, and depending on that comparison we either add, subtract or don't use a given value. We could represent that "add, subtract or don't use" by instead multiplying our offset by 1, -1 or 0 respectively. If we extract that idea:

def threeway_compare(a1, a):
    if a1 > a:
        return 1
    elif a1 < a:
        return -1
    return 0 # a1 == a

then we find the hidden symmetry (i.e., we did that kind of three-way comparison twice):

offset = ceil(shift * (i / 2))
if not(i % 2 == 1 or i == 0):
    offset = -offset
y_offset = threeway_compare(x1, x)
ny, ny1 = y + y_offset, y1 + y_offset
# Since the changes made to the nx/nx1 values were reversed,
# we negate the comparison result before using it this way.
x_offset = -threeway_compare(y1, y)
nx, nx1 = x + x_offset, x1 + x_offset
return nx, ny, nx1, ny1

Scrap everything and try a different approach.

Sometimes this is called for. In this particular case, using ceil in order to calculate an "adjustment" is a warning sign that something is going wrong. It might have been better to compute the nx etc. values directly, rather than computing x etc. and then having to adjust them in this way. Perhaps this indicates something wrong about the original question.

Is there any way to make this long if statement more concise?

2 Answers2