CODEWITHSUNDEEP
javacoding-style
kofucii
kofucii

Reputation: 7653

Get rid of ugly if statements

I have this ugly code:

if ( v > 10 ) size = 6;
if ( v > 22 ) size = 5;
if ( v > 51 ) size = 4;
if ( v > 68 ) size = 3;
if ( v > 117 ) size = 2;
if ( v > 145 ) size = 1;
return size;

How can I get rid of the multiple if statements?

Upvotes: 60

Views: 13347

Answers (25)

Sarev of Aona
Sarev of Aona

Reputation: 153

You could rewrite it in ARM code. It's only 7 cycles worst case and a slender 164 bytes. Hope that helps. (note: this is untested)

; On entry
;   r0 - undefined
;   r1 - value to test
;   lr - return address
; On exit
;   r0 - new value or preserved
;   r1 - corrupted
;
wtf
        SUBS    r1, r1, #10
        MOVLE   pc, lr
        CMP     r1, #135
        MOVGT   r0, #1
        ADRLE   r0, lut
        LDRLEB  r0, [r0, r1]
        MOV     pc, lr
;
; Look-up-table
lut
        DCB     0   ; padding
        DCB     6   ; r1 = 11 on entry
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     6
        DCB     5   ; r1 = 23 on entry
        DCB     5
        ...
        ALIGN

Upvotes: 3

George
George

Reputation: 3845

7 - (x>10 + x>22 + x>51 + x>68 + x>117 + x>145)

where 7 is the default value (x <= 10).

Edit: Initially I didn't realize this question is about Java. This expression is not valid in Java, but is valid in C/C++. I will leave the answer, as some users found it helpful.

Upvotes: 7

RTA
RTA

Reputation: 1299

why somebody have not suggested switch statement. it is far better then if else ladder.

public int getSize(int input)
    {
        int size = 0;
        switch(input)
        {
        case 10:
            size = 6;
            break;

        case 22:
            size = 5;
            break;


        case 51:
            size = 4;
            break;

        case 68:
            size = 3;
            break;

        case 117:
            size = 2;
            break;

        case 145:
            size = 1;
            break;
        }

        return size;
    }

Upvotes: 0

Jigar Joshi
Jigar Joshi

Reputation: 240900

if ( v > 145 ) size = 1;
else if ( v > 117 ) size = 2;
else if ( v > 68 ) size = 3;
else if ( v > 51 ) size = 4;
else if ( v > 22 ) size = 5;
else if ( v > 10 ) size = 6;

return size;

This is better for your case.

Optionally you should choose Switch Case where ever possible

Update: If you have analyzed the value of 'v' generally resides in lower range(<10) in most of the cases than you can add this.

if(v < 10)           size = SOME_DEFAULT_VALUE;
else if ( v > 145 )  size = 1;
else if ( v > 117 )  size = 2;
else if ( v > 68 )   size = 3;
else if ( v > 51 )   size = 4;
else if ( v > 22 )   size = 5;
else if ( v > 10 )   size = 6;

further : You can also alter the condition sequence, according to your analysis. If you know that most of the values are less than 10 and then in the second place most of values lie between 68-117, you can alter the condition sequence accordingly.

Edits:

if(v < 10)           return SOME_DEFAULT_VALUE;
else if ( v > 145 )  return 1;
else if ( v > 117 )  return 2;
else if ( v > 68 )   return 3;
else if ( v > 51 )   return 4;
else if ( v > 22 )   return 5;
else if ( v > 10 )   return 6;

Upvotes: 75

Yauhen Yakimovich
Yauhen Yakimovich

Reputation: 14211

Yet another variation (less pronounced than the answer by George)

  //int v = 9;
  int[] arr = {145, 117, 68, 51, 22, 10};
  int size = 7; for(;7 - size < arr.length && v - arr[size - 2] > 0; size--) {};
  return size;

Upvotes: -1

Vikram
Vikram

Reputation: 4106

            if (v <= 10)
                return size;
            else {
                size = 1;

                if (v > 145)
                    return size;
                else if (v > 117)
                    return ++size;
                else if (v > 68)
                    return (size+2);
                else if (v > 51)
                    return (size+3);
                else if (v > 22)
                    return (size+4);
                else if (v > 10)
                    return (size+5);
            }

This will execute the necessary if statements only.

Upvotes: -1

mfloryan
mfloryan

Reputation: 7685

How about such approach:

int getSize(int v) {
    int[] thresholds = {145, 117, 68, 51, 22, 10};

    for (int i = 0; i < thresholds.length; i++) {
        if (v > thresholds[i]) return i+1;
    }
    return 1;
}

Functionally: (Demonstrated in Scala)

def getSize(v: Int): Int = {
  val thresholds = Vector(145, 117, 68, 51, 22, 10)
  thresholds.zipWithIndex.find(v > _._1).map(_._2).getOrElse(0) + 1
}

Upvotes: 160

Sean Patrick Floyd
Sean Patrick Floyd

Reputation: 298898

Here is an object-oriented solution, a class called Mapper<S,T> that maps values from any type that implements comparable to any target type.

Syntax:

Mapper<String, Integer> mapper = Mapper.from("a","b","c").to(1,2,3);

// Map a single value
System.out.println(mapper.map("beef")); // 2

// Map a Collection of values
System.out.println(mapper.mapAll(
    Arrays.asList("apples","beef","lobster"))); // [1, 2, 3]

Code:

public class Mapper<S extends Comparable<S>, T> {

    private final S[] source;
    private final T[] target;

    // Builder to enable from... to... syntax and
    // to make Mapper immutable
    public static class Builder<S2 extends Comparable<S2>> {
        private final S2[] data;
        private Builder(final S2[] data){
            this.data = data;
        }
        public <T2> Mapper<S2, T2> to(final T2... target){
            return new Mapper<S2, T2>(this.data, target);
        }
    }


    private Mapper(final S[] source, final T[] target){
        final S[] copy = Arrays.copyOf(source, source.length);
        Arrays.sort(copy);
        this.source = copy;
        this.target = Arrays.copyOf(target, target.length);
    }

    // Factory method to get builder
    public static <U extends Comparable<U>, V> Builder<U> from(final U... items){
        return new Builder<U>(items);
    }

    // Map a collection of items
    public Collection<T> mapAll(final Collection<? extends S> input){
        final Collection<T> output = new ArrayList<T>(input.size());
        for(final S s : input){
            output.add(this.map(s));
        }
        return output;
    }

    // map a single item
    public T map(final S input){
        final int sourceOffset = Arrays.binarySearch(this.source, input);
        return this.target[
            Math.min(
                this.target.length-1,
                sourceOffset < 0 ? Math.abs(sourceOffset)-2:sourceOffset
            )
        ];
    }
}

Edit: finally replaced the map() method with a more efficient (and shorter) version. I know: a version that searches partitions would still be faster for large arrays, but sorry: I'm too lazy.

If you think this is too bloated, consider this:

  1. It contains a builder that lets you create the Mapper using varargs syntax. I'd say that's a must-have for usability
  2. It contains both a single item and a collection mapping method
  3. It's immutable and hence thread safe

Sure, all of these features could be easily removed, but the code would be less complete, less usable or less stable.

Upvotes: 5

CashCow
CashCow

Reputation: 31435

If you really want the fastest big-O complexity time solution for this particular answer this one is constant lookup.

final int minBoundary = 10;
final int maxBoundary = 145;
final int maxSize = 6;
Vector<Integer> index = new Vector<Integer>(maxBoundary);
    // run through once and set the values in your index

subsequently

if( v > minBoundary )
{
   size = (v > maxBoundary ) ? maxSize : index[v];
}

What we are doing here is marking all the possible results of v within the range and where they fall, and then we only need to test for boundary conditions.

The issue with this is that it uses more memory and of course if maxBoundary is a lot bigger it will be very space inefficient (as well as take a longer time to initialise).

This may sometimes be the best solution for the situation.

Upvotes: 0

Zutty
Zutty

Reputation: 19

The obvious answer is to use Groovy:

def size = { v -> [145,117,68,51,22,10].inject(1) { s, t -> v > t ? s : s + 1 } }

One liners are always better. Returns 7 for the undefined case where v <= 10.

Upvotes: 1

ErikE
ErikE

Reputation: 50211

I have one more version for you. I don't really think it's the best one because it adds unnecessary complexity in the name of "performance" when I'm 100% sure this function will never be a performance hog (unless someone is calculating size in a tight loop a million times ...).

But I present it just because I thought performing a hard-coded binary search to be sort of interesting. It doesn't look very binary-y because there aren't enough elements to go very deep, but it does have the virtue that it returns a result in no more than 3 tests rather than 6 as in the original post. The return statements are also in order by size which would help with understanding and/or modification.

if (v > 68) {
   if (v > 145) {
      return 1
   } else if (v > 117) {
      return 2;
   } else {
      return 3;
   }
} else {
   if (v > 51) {
      return 4;
   } else if (v > 22) {
      return 5;
   } else {
      return 6;
   }
}

Upvotes: 11

ErikE
ErikE

Reputation: 50211

Actually, if the sizes are likely to change, doing it in the database could be a good alternate strategy:

CREATE TABLE VSize (
   LowerBound int NOT NULL CONSTRAINT PK_VSize PRIMARY KEY CLUSTERED,
   Size int NOT NULL
)
INSERT VSize VALUES (10, 6)
INSERT VSize VALUES (22, 5)
INSERT VSize VALUES (51, 4)
INSERT VSize VALUES (68, 3)
INSERT VSize VALUES (117, 2)
INSERT VSize VALUES (145, 1)

And a stored procedure or function:

CREATE PROCEDURE VSizeLookup
   @V int,
   @Size int OUT
AS
SELECT TOP 1 @Size = Size
FROM VSize
WHERE @V > LowerBound
ORDER BY LowerBound

Upvotes: 2

Adrian M
Adrian M

Reputation: 7435

It is interesting that there are plenty of beautiful answers for a simple "ugly" question. I like mfloryan's answer best, however I would push it further by removing the hard-coded array inside the method. Something like,

int getIndex(int v, int[] descArray) {
    for(int i = 0; i < descArray.length; i++)
        if(v > descArray[i]) return i + 1;
    return 0;
}

It now becomes more flexible and can process any given array in descending order and the method will find the index where the value 'v' belongs.

PS. I cannot comment yet on the answers.

Upvotes: 0

Alexander Torstling
Alexander Torstling

Reputation: 18898

The most obvious problem with the OPs solution is branching, so I would suggest a polynomial regression. This will result in a nice branchless expression on the form

size = round(k_0 + k_1 * v + k_2 * v^2 + ...)

You will of course not get an exact result, but if you can tolerate some deviance it's a very performant alternative. Since the 'leave unmodified' behavior of to original function for values where v<10 is impossible to model with a polynomial, I took the liberty of assuming a zero-order hold interpolation for this region.

For a 45-degree polynomial with the following coefficients,

-9.1504e-91 1.1986e-87 -5.8366e-85 1.1130e-82 -2.8724e-81 3.3401e-78 -3.3185e-75  9.4624e-73 -1.1591e-70 4.1474e-69 3.7433e-67 2.2460e-65 -6.2386e-62 2.9843e-59 -7.7533e-57 7.7714e-55 1.1791e-52 -2.2370e-50 -4.7642e-48 3.3892e-46 3.8656e-43 -6.0030e-41 9.4243e-41 -1.9050e-36 8.3042e-34 -6.2687e-32 -1.6659e-29 3.0013e-27 1.5633e-25 -8.7156e-23  6.3913e-21 1.0435e-18 -3.0354e-16 3.8195e-14 -3.1282e-12 1.8382e-10 -8.0482e-09 2.6660e-07 -6.6944e-06 1.2605e-04 -1.7321e-03 1.6538e-02 -1.0173e-01 8.3042e-34 -6.2687e-32 -1.6659e-29 3.0013e-27 1.5633e-25 -8.7156e-23 6.3913e-21 1.0435e-18 -3.0354e-16 3.8195e-14 -3.1282e-12 1.8382e-10 -8.0482e-09 2.6660e-07 -6.6944e-06 1.2605e-04 -1.7321e-03 1.6538e-02 -1.0173e-01 3.6100e-01 -6.2117e-01 6.3657e+00

, you get a beautifully fitted curve:

alt text

And as you can see, you get an 1-norm error of just 1.73 across the whole range from 0 to 200*!

*Results for v∉[0,200] may vary.

Upvotes: 82

phv3773
phv3773

Reputation: 497

Just for completeness, let me suggest that you could set up an array SIZES with 145 elements so the answer could be returned directly as SIZES[v]. Pardon me for not writing the whole thing out. You would have to make sure v was in range, of course.

The only reason I can think of for doing it that way would be if you were going to create the array once and use it thousands of time in an application that had to be really fast. I mention it as an example of a trade-off between memory and speed (not the problem it once was), and also between setup time and speed.

Upvotes: 1

CashCow
CashCow

Reputation: 31435

This is my code sample, using SortedSet. You initialise boundaries once.

SortedSet<Integer> boundaries = new SortedSet<Integer>;

boundaries.add(10);

boundaries.add(22);

boundaries.add(51);

boundaries.add(68);

boundaries.add(117);

boundaries.add(145);

Then use it subsequently this way for multiple values of v (and initialised size)

SortedSet<Integer> subset =  boundaries.tailSet(v);
if( subset.size() != boundaries.size() )
  size = subset.size() + 1;

Upvotes: 0

mhaller
mhaller

Reputation: 14232

return (v-173) / -27;

Upvotes: 14

st0le
st0le

Reputation: 33545

Here's my shot at it...

Update: Fixed. Previous Solution gave incorrect answers for exact values (10,22,51...). This one defaults to 6 for the if val < 10

   static int Foo(int val)
    {
                          //6, 5, 4, 3, 2 ,1
        int[] v = new int[]{10,22,51,68,117,145};
        int pos = Arrays.binarySearch(v, val-1);
        if ( pos < 0) pos = ~pos;
        if ( pos > 0) pos --;
        return 6-pos;
    }

Upvotes: 12

Jim Ferrans
Jim Ferrans

Reputation: 31012

The original code seems fine to me, but if you don't mind multiple returns you might prefer a more tabular approach:

if ( v > 145 ) return 1;
if ( v > 117 ) return 2;
if ( v >  68 ) return 3;
if ( v >  51 ) return 4;
if ( v >  22 ) return 5;
if ( v >  10 ) return 6;
return ...;     // The <= 10 case isn't handled in the original code snippet.

See the multiple return or not discussion in org.life.java's answer.

Upvotes: 23

user207421
user207421

Reputation: 310893

Is there an underlying mathematical rule to this? If so you should use that: but only if it comes from the problem domain, not just some formula that happens to fit the cases.

Upvotes: 4

Chris Adragna
Chris Adragna

Reputation: 632

My commenting ability isn't turned on yet, hopefully no one will say "rightfully" based on my answer...

Pretty-ing up the ugly code could/should be defined as trying to achieve:

  1. Readability (OK, stating the obvious -- redundant to the question perhaps)
  2. Performance -- at best seeking optimal, at worst it's not a big drain
  3. Pragmatism -- it's not far off the way most people do things, given an ordinary problem that's not in need of an elegant or unique solution, changing it later on should be a natural effort, not in need of much recollection.

IMO the answer given by org.life.java was the prettiest and extremely easy to read. I also liked the order in which the conditions were written, for reasons of reading and performance.

Looking over all the comments on this subject, at the time of my writing, it appears that only org.life.java raised the issue of performance (and maybe mfloryan, too, stating something would be "longer"). Certainly in most situations, and given this example it shouldn't bear a noticeable slowdown however you write it.

However, by nesting your conditions and optimally ordering the conditions can improve performance [worthwhile, particularly if this were looped].

All that being said, nesting and ordering conditions (that are more complex than your example) brought on by determination to achieve as fast as possible execution will often produce less readable code, and code that's harder to change. I refer again to #3, pragmatism... balancing the needs.

Upvotes: 5

cbmeeks
cbmeeks

Reputation: 11420

There are a ton of answers and suggestions here but I honestly don't see any of them "prettier" or "more elegant" than the original method.

If you had dozens or HUNDREDS of iterations to check then I could easily see going to some for loop but honestly, for the handful of comparisons you had, stick with the if's and move on. It's not that ugly.

Upvotes: 17

dhblah
dhblah

Reputation: 10151

return v > 145 ? 1 
     : v > 117 ? 2 
     : v > 68 ? 3 
     : v > 51 ? 4 
     : v > 22 ? 5 
     : v > 10 ? 6 
     : "put inital size value here";

Upvotes: 51

barjak
barjak

Reputation: 11270

Using the NavigableMap API :

NavigableMap<Integer, Integer> s = new TreeMap<Integer, Integer>();
s.put(10, 6);
s.put(22, 5);
s.put(51, 4);
s.put(68, 3);
s.put(117, 2);
s.put(145, 1);

return s.lowerEntry(v).getValue();

Upvotes: 88

Ani
Ani

Reputation: 113402

int[] arr = new int[] {145, 117, 68, 51, 22, 10};
for(int index = 0; index < arr.length; index++)
{
  if(v > arr[index]) return 1 + index; 
}

return defaultValue;

Upvotes: 3

Related Questions