1

I have written the following code for finding pageCount of comics that lie within certain budget.

At first I tried to come up with code that would have architecture like this:

  • Stream gives price of a MarvelComic object.
  • I sum the price of the MarvelComic object from stream with prices of the previous comics that came down this stream and check if it's < BUDGET
  • If Yes, then I sum the pageCount of the MarvelComic object with the pageCount sum of previous MarvelComic objects that came down the stream.
  • If yes, then onNext of the subscriber is called.

Since I couldn't devise a way to write code like I mentioned in the steps above, I resorted to mashing imperative programming with Reactive Programming. As a result I wrote the following code:

Observable.fromIterable(getMarvelComicsList()).
                map(new Function<MarvelComic, HashMap<String, Double>>() {
                    @Override
                    public HashMap<String, Double> apply(@NonNull MarvelComic marvelComic) throws Exception {
                        HashMap<String, Double> map = new HashMap<String, Double>();
                        map.put("price", Double.valueOf(marvelComic.getPrice()));
                        map.put("pageCount", Double.valueOf(marvelComic.getPageCount()));
                        map.put("comicCount", Double.valueOf(marvelComic.getPageCount()));
                        return map;
                    }
                })
                .scan(new HashMap<String, Double>(), new BiFunction<HashMap<String, Double>, HashMap<String, Double>, HashMap<String, Double>>() {
                    @Override
                    public HashMap<String, Double> apply(@NonNull HashMap<String, Double> inputMap, @NonNull HashMap<String, Double> newValueMap) throws Exception {
                        double sum = inputMap.get("price")+newValueMap.get("price");
                        double count = inputMap.get("pageCount")+newValueMap.get("pageCount");
                        double comicCount = inputMap.get("comicCount")+newValueMap.get("comicCount");

                        HashMap<String, Double> map = new HashMap<String, Double>();
                        map.put("price", sum);
                        map.put("pageCount", count);
                        map.put("comicCount", comicCount);

                        return map;
                    }
                })
                .takeWhile(new Predicate<HashMap<String, Double>>() {
                    @Override
                    public boolean test(@NonNull HashMap<String, Double> stringDoubleHashMap) throws Exception {
                        return stringDoubleHashMap.get("price") < budget;
                    }
                })
                .subscribe(new DisposableObserver<HashMap<String, Double>>() {
                    @Override
                    public void onNext(HashMap<String, Double> stringDoubleHashMap) {
                        double sum = stringDoubleHashMap.get("price");
                        double pageCount = stringDoubleHashMap.get("pageCount");
                        double comicCount = stringDoubleHashMap.get("comicCount");
                        Timber.e("sum %s  pageCount %s  ComicCount: %s", sum, pageCount, comicCount);
                    }

                    @Override
                    public void onError(Throwable e) {
                        Timber.e("onError %s", e.fillInStackTrace());
                    }

                    @Override
                    public void onComplete() {
                        Timber.e("onComplete");
                    }
                });

My Reservations:

  1. Is it good idea to create a new Hashmap every time inside map(), scan()?
  2. How can I improve this code further?

Issues:

This code gives NullPointerException in onError because map.get("price") returns null in scan(). I'm not really sure of the reason.

Error:

 onError java.lang.NullPointerException: Attempt to invoke virtual method 'double java.lang.Double.doubleValue()' on a null object reference

NOTE:

HashMap isn't null, the double field is being returned as NULL for some reason. I'm trying to figure out how.

Umer Farooq
  • 7,356
  • 7
  • 42
  • 67
  • Why not use a class with a `double` field and two `int` fields instead of that `HashMap`? – Dawood ibn Kareem Apr 25 '17 at 10:52
  • Yes that came to my mind but I thought instead of creating a separate class, resorting to a collection will be a cleaner solution. Even if I created another class, I would have to create an object of that class in the mentinoed methods – Umer Farooq Apr 25 '17 at 10:54
  • @DharmbirSingh Kindly Post a question and stop commenting about stuff that's not related to this question. – Umer Farooq Apr 25 '17 at 10:59
  • I wouldn't consider this use of a `HashMap` to be particularly clean. And if you follow my suggestion, you won't have problems with null pointer exceptions. – Dawood ibn Kareem Apr 25 '17 at 11:07
  • In terms of solving your immediate problem though, the best thing you can do is to step through this code with a debugger to find out exactly where the unexpected null is coming from. – Dawood ibn Kareem Apr 25 '17 at 11:08
  • @DawoodibnKareem Thanks for the suggestion. For instance let's keep the NPE asside, can you please tell me why you prefer a model class instead of collection for this particular case. The reason I'm trying to avoid creating class is, if I have multiple cases similar to the one in OP, I will have to create multiple helper classes to do so. That's just going to add noise in the project IMO – Umer Farooq Apr 25 '17 at 11:10
  • Not as much noise as manipulating a HashMap every time you want to use one of these objects. – Dawood ibn Kareem Apr 25 '17 at 11:14

4 Answers4

2

The problem is likely that you have an empty initial map due to

.scan(new HashMap<String, Double>(), ...)

and when the first real map arrives from upstream, you are trying to get values from that empty initial map:

double sum = inputMap.get("price")+newValueMap.get("price");

I assume you want to do a running aggregate of the properties by the use of scan so you should try scan(BiFunction) that emits the first upstream value as is and then start combining the previous with the new upstream value.

Alternatively, you could pre-initialize the new HashMap<>() with default values and avoid the NPE as well:

HashMap<String, Double> initialMap = new HashMap<String, Double>();
initialMap.put("price", 0.0d);
initialMap.put("pageCount", 0.0d);
initialMap.put("comicCount", 0.0d);

Observable.fromIterable(getMarvelComicsList()).
            map(new Function<MarvelComic, HashMap<String, Double>>() {
                @Override
                public HashMap<String, Double> apply(@NonNull MarvelComic marvelComic) {
                    HashMap<String, Double> map = new HashMap<String, Double>();
                    map.put("price", Double.valueOf(marvelComic.getPrice()));
                    map.put("pageCount", Double.valueOf(marvelComic.getPageCount()));
                    map.put("comicCount", Double.valueOf(marvelComic.getPageCount()));
                    return map;
                }
            })
            .scan(initialMap, 
            new BiFunction<HashMap<String, Double>, 
                    HashMap<String, Double>, HashMap<String, Double>>() {
                @Override
                public HashMap<String, Double> apply(
                         @NonNull HashMap<String, Double> inputMap, 
                         @NonNull HashMap<String, Double> newValueMap) {
                    double sum = inputMap.get("price")+newValueMap.get("price");
                    double count = inputMap.get("pageCount")
                        +newValueMap.get("pageCount");
                    double comicCount = inputMap.get("comicCount")
                        +newValueMap.get("comicCount");

                    HashMap<String, Double> map = new HashMap<String, Double>();
                    map.put("price", sum);
                    map.put("pageCount", count);
                    map.put("comicCount", comicCount);

                    return map;
                }
            })
            // etc.
akarnokd
  • 69,132
  • 14
  • 157
  • 192
  • Thanks so much. This is exactly I was doing wrong. However can you please tell me under what circumstances a person would use `scan(initiailvalue, new BiFunc() ...)`? Another question is, is the way of using collections to perform action on multiple fields good? or should I write separate observables for all the properties and zip them? I don't know if that zip() thing will work or not (still newbie) but IMO that'll inefficient cuz I'll have N observables & I'll be going over them N times for N fields. – Umer Farooq Apr 25 '17 at 12:37
  • 1
    When you need that initial value even if the upstream is empty. Values belonging together should travel together, not much reason to split each into its own stream when they are available together. You should also consider using a proper value class instead of HashMap as Hans showed. – akarnokd Apr 25 '17 at 12:55
  • @Thanks I have created a separate Model class for this filtering process. I was and still am kinda reluctant to create separate model classes for every process rather then using collections. Additional model classes just create noise in my opinion but then considering my limited knowledge, may be creating model classes is the best path. – Umer Farooq Apr 25 '17 at 13:35
1

I tried to solve your problem with a different approach, which does not throw any NPE.

Please do not use HashMaps as datastructures. It is intransparent what is going on. You should create classes which are meaningful.

Furthermore the subscriber should not do any businesslogic. The subscriber actually should just use the result and do side-effects like changing the view.

I hope I did understand your question properly.

@Test
void name() {
    ArrayList<MarvelComic> marvelComics = Lists.newArrayList(new MarvelComic(10, 200), new MarvelComic(3, 133), new MarvelComic(5, 555), new MarvelComic(32, 392));

    final double BUDGET = 20.0;

    Observable<Result> resultObservable = Observable.fromIterable(marvelComics)
            .scan(Result.IDENTITY, (result, marvelComic) -> {
                double priceSum = result.sumPrice + marvelComic.getPrice();

                if (priceSum <= BUDGET) {
                    int pageCount = result.sumPageCount + marvelComic.getPageCount();
                    int comicCount = result.comicCount + 1;
                    return new Result(pageCount, priceSum, comicCount);
                }

                return Result.IDENTITY;
            })
            .skip(1) // because first Value would be Result.IDENTITY
            .takeWhile(result -> result != Result.IDENTITY);

    TestObserver<Result> test = resultObservable.test().assertValueCount(3);

    Result result1 = test.values()
            .stream()
            .reduce((result, result2) -> result2)
            .get();

    assertThat(result1.comicCount).isEqualTo(3);
    assertThat(result1.sumPageCount).isEqualTo(888);
    assertThat(result1.sumPrice).isEqualTo(18);
}

class MarvelComic {
    private final double price;
    private final int pageCount;

    MarvelComic(double price, int pageCount) {
        this.price = price;
        this.pageCount = pageCount;
    }

    public double getPrice() {
        return price;
    }

    public int getPageCount() {
        return pageCount;
    }
}

static class Result {
    private final int sumPageCount;

    private final double sumPrice;

    private final int comicCount;

    Result(int sumPageCount, double sumPrice, int comicCount) {
        this.sumPageCount = sumPageCount;
        this.sumPrice = sumPrice;
        this.comicCount = comicCount;
    }

    static Result IDENTITY = new Result(0, 0, 0);
}
Sergej Isbrecht
  • 3,842
  • 18
  • 27
  • Can you please tell me why does HashMap returns null in place of the values even though I put the values in it myself and I double checked it? – Umer Farooq Apr 25 '17 at 12:11
  • Look at the answer from @akarnokd. Scan pushes empty hashMap to subscriber. Get will return null if element not in hashMap. You try to invoke a method on null return. Please note the description of get method: Returns: the value to which the specified key is mapped, or null if this map contains no mapping for the key – Sergej Isbrecht Apr 25 '17 at 12:19
0

i think you can get null or blank value of getPrice(),getPageCount() methods 

 map.put("price", Double.valueOf(marvelComic.getPrice()));
                            map.put("pageCount", Double.valueOf(marvelComic.getPageCount()));
                            map.put("comicCount", Double.valueOf(marvelComic.getPageCount()));

or you can use Double.parseDouble(); method

Bhupat Bheda
  • 1,968
  • 1
  • 8
  • 13
0

you have used doubleValue() Function 3 times here,

map.put("price", Double.valueOf(marvelComic.getPrice()));
map.put("pageCount", Double.valueOf(marvelComic.getPageCount()));
map.put("comicCount", Double.valueOf(marvelComic.getPageCount()));

confirm that marvelComic has the values for Price And Page Count, and I think you are missing comicCount as you are adding pageCount as comicCount in map,

I would suggest to use try catch and print the error to get to know the root cause

SwapnilKumbhar
  • 347
  • 2
  • 5
  • 17