I need to add a range of numbers to a list based on specific conditions. The method recieves a String, and based on value of the String, specific list of values should be added to the list.
Name variable has name of the selected product and numbers are serial number of products that are similar to the selected product.
I am wondering if there is any better approach to do it.
ArrayList numbers = new ArrayList<Integer>();
switch (name) {
case "A":
for (int i = 1; i <= 100; i++) {
if ((i >= 46 && i <= 50) || (i >= 60 && i <= 73) || i == 91
|| i == 96 || i == 97) {
continue;
} else {
numbers.add(i);
}
}
break;
case "B":
for (int i = 1; i <= 100; i++) {
if ((i >= 50 && i <= 55) || (i >= 65 && i <= 88) || i == 90
|| i == 98 || i == 99) {
continue;
} else {
numbers.add(i);
}
}
break;
}
return numbers;
-
2\$\begingroup\$ Also, a bit of context about why you are doing this, and where those excluded ranges come from, would be helpful. \$\endgroup\$200_success– 200_success2016年07月29日 01:29:54 +00:00Commented Jul 29, 2016 at 1:29
1 Answer 1
It's always good to ask questions and try to find potentially better ways to do things. You've come to the right place.
So here's what I'm thinking regarding your question:
Yes, I think there is a better way to handle this that will benefit you in the long term, especially with respect to maintenance efforts. Right now you've hard-coded all the various ranges and values that you're excluding from your results. Let me first suggest that this kind of task is better probably handled by a database and a query to that database rather than coding rules in this manner. If you have all of your products and serial numbers in a database you can maintain the data in one place using a system that is designed for data management and you won't have to update your code every time your number scheme changes.
The basic idea is that you're coding data rather than generic logic and structures to support the use of the data. In order to illustrate this I have put together some Java code, but did not implement any database queries as that would take additional time and you can probably research this more on your own without too much difficulty.
So, the first thing I notice is you have very complex if
statements that contain hard-coded ranges. If you create a generic Range
class and use it to create Range
objects to represent the various ranges of values you need you can make your code more generalized, reliable, and maintainable. Here is the Range
class I created:
public class Range {
private boolean isUpperBoundExclusive = true, isLowerBoundExclusive = true;
private int upperBound, lowerBound;
public Range(int value){
isUpperBoundExclusive = false;
isLowerBoundExclusive = false;
upperBound = value;
lowerBound = value;
}
public Range(int lower, int upper){
lowerBound = lower;
upperBound = upper;
}
public void setUpperBoundExclusive(boolean isUpperBoundStrict) {
this.isUpperBoundExclusive = isUpperBoundStrict;
}
public void setLowerBoundExclusive(boolean isLowerBoundStrict) {
this.isLowerBoundExclusive = isLowerBoundStrict;
}
public boolean isWithinRange(int check){
return (isLowerBoundExclusive ? lowerBound < check : lowerBound <= check)
&& (isUpperBoundExclusive ? upperBound > check : upperBound >= check);
}
}
Notice how this class represents data, but does not hard-code any data within it. I can configure an instance of this class to represent a single value using the first constructor or a range of values that is inclusive or exclusive at either end. Notice there is also a method to determine whether a value is within the range called isWithinRange(int check)
. Please forgive the lack of Javadoc comments, but this is something I just created and I'm preparing to go to bed soon.
By using the Range
class we can test the general logic and then we can have some certainty that it works as expected and that if we see errors while using it we're probably using it wrong in some way. So I wrote a quick JUnit test class to test the Range
class, here it is:
import static org.junit.Assert.*;
import org.junit.Test;
public class RangeTests {
@Test
public void test() {
Range range = new Range(10,20);
assertFalse(range.isWithinRange(21));
assertFalse(range.isWithinRange(9));
assertFalse(range.isWithinRange(-1));
assertFalse(range.isWithinRange(0));
assertFalse(range.isWithinRange(10));
assertFalse(range.isWithinRange(20));
assertTrue(range.isWithinRange(11));
assertTrue(range.isWithinRange(19));
assertTrue(range.isWithinRange(15));
}
@Test
public void test2() {
Range range = new Range(10,20);
range.setLowerBoundExclusive(false);
assertTrue(range.isWithinRange(10));
assertFalse(range.isWithinRange(21));
assertFalse(range.isWithinRange(9));
assertFalse(range.isWithinRange(-1));
assertFalse(range.isWithinRange(0));
assertFalse(range.isWithinRange(20));
assertTrue(range.isWithinRange(11));
assertTrue(range.isWithinRange(19));
assertTrue(range.isWithinRange(15));
}
@Test
public void test3() {
Range range = new Range(10,20);
range.setLowerBoundExclusive(false);
range.setUpperBoundExclusive(false);
assertTrue(range.isWithinRange(10));
assertTrue(range.isWithinRange(20));
assertFalse(range.isWithinRange(21));
assertFalse(range.isWithinRange(9));
assertFalse(range.isWithinRange(-1));
assertFalse(range.isWithinRange(0));
assertTrue(range.isWithinRange(11));
assertTrue(range.isWithinRange(19));
assertTrue(range.isWithinRange(15));
}
@Test
public void test4() {
Range range = new Range(10,20);
range.setUpperBoundExclusive(false);
assertFalse(range.isWithinRange(10));
assertTrue(range.isWithinRange(20));
assertFalse(range.isWithinRange(21));
assertFalse(range.isWithinRange(9));
assertFalse(range.isWithinRange(-1));
assertFalse(range.isWithinRange(0));
assertTrue(range.isWithinRange(11));
assertTrue(range.isWithinRange(19));
assertTrue(range.isWithinRange(15));
}
@Test
public void test5() {
Range range = new Range(10);
assertFalse(range.isWithinRange(11));
assertFalse(range.isWithinRange(9));
assertTrue(range.isWithinRange(10));
}
}
Next, if we had a list of products we could use inside our main logic it would help simplify the code. I'm not sure whether that's possible since you didn't really specify what sort of data and objects you have to work with, but here is what I'm thinking:
import java.util.ArrayList;
import java.util.List;
public class Product {
private String name;
private List<Range> serialNumExcludedRanges;
public Product(String name){
this.name = name;
serialNumExcludedRanges = new ArrayList<Range>();
}
public String getName(){
return name;
}
public void addExcludedSerialNumRange(Range rangeToAdd){
serialNumExcludedRanges.add(rangeToAdd);
}
public boolean isSerialNumExcluded(int check){
for(Range excludedRange: serialNumExcludedRanges){
if(excludedRange.isWithinRange(check))
return true;
}
return false;
}
}
Notice how the Product
itself contains a list of excluded ranges that it then uses when the isSerialNumExcluded
method is called. At that time it uses these ranges in order to determine whether the given input is excluded or not and returns a boolean
value accordingly.
Finally, going back to the main logic here's what we're now able to write:
public static List<Integer> getSerialNums(String name) {
ArrayList<Integer> numbers = new ArrayList<Integer>();
for (Product product : productList) {
// Note: should handle possible null here - what if the "name" param
// is null?
if (name.equals(product.getName())) {
for (int i = 1; i <= 100; i++) {
if (!product.isSerialNumExcluded(i)) {
numbers.add(i);
}
}
}
}
return numbers;
}
Remember, this assumes that we have already defined our list of products prior to calling this method. In this example I created a static variable and added a simple main method to initialize them, but this is where a database query would really become useful. Rather than initialize them by hard-coding the values (for completeness I have provided the initialization logic below) you could store the data in a database then query for the values you want and populate your data structures.
private static List<Product> productList = new ArrayList<Product>();
public static void main(String[] args) {
//Define product A below:
Product productA = new Product("A");
Range r1 = new Range(46, 50);
r1.setLowerBoundExclusive(false);
r1.setUpperBoundExclusive(false);
productA.addExcludedSerialNumRange(r1);
Range r2 = new Range(60, 73);
r2.setLowerBoundExclusive(false);
r2.setUpperBoundExclusive(false);
productA.addExcludedSerialNumRange(r2);
Range r3 = new Range(91);
productA.addExcludedSerialNumRange(r3);
Range r4 = new Range(96);
productA.addExcludedSerialNumRange(r4);
Range r5 = new Range(97);
productA.addExcludedSerialNumRange(r5);
productList.add(productA);
//Define product B below:
Product productB = new Product("B");
Range r6 = new Range(50, 55);
r6.setLowerBoundExclusive(false);
r6.setUpperBoundExclusive(false);
productB.addExcludedSerialNumRange(r6);
Range r7 = new Range(65, 88);
r7.setLowerBoundExclusive(false);
r7.setUpperBoundExclusive(false);
productB.addExcludedSerialNumRange(r7);
Range r8 = new Range(90);
productB.addExcludedSerialNumRange(r8);
Range r9 = new Range(98);
productB.addExcludedSerialNumRange(r9);
Range r10 = new Range(99);
productB.addExcludedSerialNumRange(r10);
productList.add(productB);
}
I hope my code has helped to illustrate the difference between data, business logic, and data structure. Consider what happens when you need to update your code - you risk introducing bugs every time you change something. Keeping the data separate from the logic allows you to isolate the cause of problems. If the code didn't change, odds are it's a data problem. Once again, I just want to restate that I'm not really suggesting you store the ranges of values and so forth in a database - I suggest you store the product information itself and then use that data rather than generate a list of it with a loop. Hope this helps and best of luck!
-
1\$\begingroup\$ If you were not aware of it, Google Guava provides a nice Range class that is rather robust. You can find info here. It might be easier than coding your own , though it would require dependencies. \$\endgroup\$Himself12794– Himself127942016年07月29日 13:47:15 +00:00Commented Jul 29, 2016 at 13:47