Get rid of ugly claims

Question

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 multiple if statements?

Answer 1

 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 the best for your business.

If you wish, you should choose the Switch Case option, where ever possible

Update: If you have analyzed the value of "v", in most cases it is in a lower range (<10) 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 change the sequence of conditions according to your analysis. If you know that most of the values ​​are less than 10, and then, secondly, most of the values ​​are between 68-117, you can change the sequence of conditions accordingly.

Changes:

 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; 

Answer 2

How about this 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; } 

---

Functional: (shown 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 } 

Answer 3

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(); 

Answer 4

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

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

Of course, you will not get the exact result, but if you can tolerate some deviations, this is a very effective alternative. Since the “leave unchanged” behavior of the original function for values ​​where v<10 cannot be modeled using a polynomial, I took the liberty of accepting zero-order hold interpolation for this area.

For a polynomial of 45 degrees with the following ratios

 -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 will get a beautifully fitted curve:

And, as you can see, you get a 1-dimensional error of only 1.73 in the entire range from 0 to 200 *!

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

Answer 5

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

Answer 6

The original code seems fine to me, but if you don't mind a few 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 multiple return or no discussion in org.life.java post .

Answer 7

There are tons of answers and suggestions, but I honestly don’t see any of them “more beautiful” or “more elegant” than the original method.

If you had dozens or hundreds of iterations to check, then I could easily see how someone goes in cycles, but to be honest, for a few comparisons that you had, stick with if and move. This is not so ugly.

Answer 8

 return (v-173) / -27; 

Answer 9

Here is my picture ...

Update: fixed. The previous solution gave incorrect answers for exact values ​​(10,22,51 ...). This default value is 6 for if <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; } 

Answer 10

I have another version for you. I don’t think this is the best because it adds unnecessary complexity in the name of “performance” when I am 100% sure that this function will never be a productive pig (unless someone calculates the size in a tight cycle a million times. ..).

But I only introduce it because I thought hard binary search was hard coded as interesting . It does not look very binary, because there are not enough elements to go very deep, but it has the virtue that it returns the result from no more than three tests, and not from 6 , as in the original post. The return statements are also ordered by size, which helps 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; } } 

Answer 11

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

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

Edit: Initially, I did not understand that this question was about Java. This expression is not valid in Java, but it is valid in C / C ++. I will leave an answer, as some users find it useful.

Answer 12

Here is an object-oriented solution, a class called Mapper<S,T> , which displays values ​​of any type that implements targets comparable to any 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] 

The 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: the version that is looking for partitions will still be faster for large arrays, but sorry: I'm too lazy.

If you think this is too bloated, think about it:

• It contains a builder that allows you to create a mapper using the varargs syntax. I would say that it is mandatory for ease of use
• It contains both a single element and a collection matching method.
• It is a constant and therefore safe flow.

Of course, all these functions can be easily removed, but the code will be less complete, less useful or less stable.

Answer 13

My ability to comment is not included, I hope no one will say “rightfully” based on my answer ...

Satisfying ugly code can / should be defined as an attempt to achieve:

• Readability (OK, indicating the obvious - unnecessary question)
• Performance is optimal at best, at worst it's not a big leak.
• Pragmatism is far from the way most people do, given the usual problem, which does not need an elegant or unique solution, changing it later should be a natural effort that does not need much memory.

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

Looking back at all the comments on this issue, at the time of writing, it turned out that only org.life.java raised the issue of performance (and perhaps mfloryan also states that something will be “longer”). Of course, in most situations, and given this example, it should not have a noticeable slowdown, but you write it.

However, your nested conditions and the optimal order of conditions can increase productivity [worth it, especially if it was looped).

All that is said, the conditions for nesting and ordering (more complex than your example), obtained by defining to achieve the fastest possible execution, often create less readable code and code that is more difficult to change. I again refer to No. 3, pragmatism ... balancing needs.

Answer 14

Is there a basic mathematical rule for this? If so, you should use this: but only if it comes from the problem domain, and not just some formula that is suitable for cases.

Answer 15

 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; 

Answer 16

You can rewrite it in ARM code. These are just 7 worst-case cycles and thin 164 bytes. Hope this helps. (note: this is not verified)

 ; 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 

Answer 17

Just for completeness, let me suggest you install a SIZES array with 145 elements so that the answer can be returned directly as SIZES [v]. Forgive me for not writing all this. Of course, you had to make sure that v is in range.

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

Answer 18

In fact, if the sizes are likely to change, doing this in the database may be a good alternative 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 the 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 

Answer 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 liner is always better. Returns 7 for undefined, where v <= 10.

Answer 20

why didn't someone suggest a switch statement. it is much better if there was another staircase.

 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; } 

Answer 21

This is my sample code using SortedSet. You initialize the borders 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 afterwards for multiple v values ​​(and initialized size)

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

Answer 22

If you really want a quick solution to the complexity time with a large O for this particular answer, this is a constant search.

 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 

then

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

What we are doing here means all possible results v within the range and where they fall, and then we only need to check the boundary conditions.

The problem is that it uses more memory and, of course, if maxBoundary is much larger, it will be very inefficient (and also take more time to initialize).

Sometimes this may be the best solution.

Answer 23

Interestingly, there are many excellent answers to a simple ugly question. I like mfloryan's answer best, however I would push it further by deleting 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; } 

Now it becomes more flexible and can process any given array in descending order, and the method will find the index where the value of 'v' belongs.

PS. I can not comment on the answers yet.

Answer 24

  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 only execute the necessary if statements.

Answer 25

Another variation (less pronounced than George's answer)

  //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;