Printing the max of groups of three random ints

Question

I'm trying to teach myself streaming and lambda expressions. Here's the scenario: I have a collection of 10 Thing objects each containing an int[] of 3 random number. I've written code to print out the largest number in each Thing.

I've already written two statements and an accompanying method that print the result I want, however the code is pretty ropey... improvements and suggestions would be greatly appreciated.

import java.util.Random;
public class Thing {
 private Random r = new Random();
 private int a = r.nextInt(), b = r.nextInt(), c = r.nextInt();
 public int[] getCollectionOfInts() {
 return collectionOfInts;
 }
 private int[] collectionOfInts = new int[]{a, b, c};
}//end of class
//main in separate driver class
public static void main(String[] args) {
 Thing[] things = new Thing[10];
 int a=0,b=0,c=0;
 for (int x =0;x<10;x++){
 things[x] = new Thing();
 for (int y=0;y<things[x].getCollectionOfInts().length;y++)
 {
 if(y==0)
 a = things[x].getCollectionOfInts()[y];
 if(y==1)
 b = things[x].getCollectionOfInts()[y];
 if(y==2)
 c = things[x].getCollectionOfInts()[y];
 }
 //print 3 numbers in each Thing object
 System.out.println(x+": "+a+", "+b+", "+c);
 }
 System.out.println();
 //compare each number and print out largest... too many ternaries 
 Stream.of(things).forEach(Thing->System.out.println(Stream.of(Thing.getCollectionOfInts()).mapToInt(z->z[0]>z[1]&&z[0]>z[2]?z[0]:z[1]>z[2]?z[1]:z[2]).reduce((x,y)->x+y).getAsInt()));
 System.out.println();
 //improper use of .max() or .mapToInt(...)?
 Stream.of(things).forEach(Thing->System.out.println(Stream.of(Thing.getCollectionOfInts()).mapToInt(x->maxInt(x)).max().getAsInt()));
 }
 public static int maxInt(int[] x) {
 int max=x[0];
 for (int y:x) {
 if(y>max)
 max = y;
 }
 return max;
 }

I get the expected results for both streams, but I'm still unhappy with how I get there!

Answer 1

The first thing about your code are the two lines inside your class Thing:

private Random r = new Random();
private int a = r.nextInt(), b = r.nextInt(), c = r.nextInt();

If Random r is used just to initialize the array and not in other methods inside the class , it is better to use it in the costructor of the class :

public Thing() {
 Random r = new Random();
 this.arr = new int[] {r.nextInt(), r.nextInt(), r.nextInt()};
}

You can check I initialize directy here the array of ints instead of defining variables a, b, c. A good thing is also override the String method to print the internal state of a object:

@Override
public String toString() {
 return Arrays.toString(arr);
}

Now you can print the state of your Thing object in this way:

for (int i = 0;i < 10; ++i){
 things[i] = new Thing();
 //print 3 numbers in each Thing object
 System.out.println(i + ": " + things[i]);
}

Your iterations with Stream are ok, but instead of use Stream because you are working with int values you can use instead IntStream simplifyng the code:

Stream.of(things).forEach(t -> System.out.println(Arrays.stream(t.getArr()).max().getAsInt()));

Below the code of class Thing including all my modifies:

package codereview;
import java.util.Arrays;
import java.util.Random;
import java.util.stream.Stream;
public class Thing {
 private int[] arr;
 public Thing() {
 Random r = new Random();
 this.arr = new int[] {r.nextInt(), r.nextInt(), r.nextInt()};
 }
 public int[] getArr() {
 return arr;
 }
 @Override
 public String toString() {
 return Arrays.toString(arr);
 }
 public static void main(String[] args) {
 Thing[] things = new Thing[10];
 for (int i = 0;i <10; ++i){
 things[i] = new Thing();
 //print 3 numbers in each Thing object
 System.out.println(i + ": " + things[i]);
 }
 System.out.println();
 Stream.of(things).forEach(t -> System.out.println(Arrays.stream(t.getArr()).max().getAsInt()));
 } 
}

• Roland Illig: Your code could benefit from being formatted consistently by an IDE. Apart from that, it's nice and simple. I also refactored the OP's code and reached very similar code.
• dariosicily: @RolandIllig Thanks, I'm still messing when I copy code from IDE here.
• Roland Illig: There's a simple recipe against messed up code: just forget about the 4 spaces of indentation and surround the code by ~~~. That's much easier to get right.
• dariosicily: @RolandIllig I didn't know this, I apply it in the next answers.

Answer 2

Advice 1: code packaging

I suggest you put your Thing related code into a package. That way you may practice industrial level programming:

package net.tnm;

Note that the above package name is just an example. Usually, is should be reversed domain name of your company. (For example, package com.oracle.xxx where xxx is the project name.)

Advice 2: code layout You have this:

private int a = r.nextInt(), b = r.nextInt(), c = r.nextInt();

I would suggest

private final int a = r.nextInt(), 
 b = r.nextInt(), 
 c = r.nextInt();

Advice 3: declaring immutable fields final

Once again:

private final int a = r.nextInt(), 
 b = r.nextInt(), 
 c = r.nextInt();

Advice 4: spaces around binary operators

A binary operator is an operator that that takes two operands. You often write, for example, y=0, when the coding conventions dictate y = 0.

Advice 5: bracing

You have for (int x =0;x<10;x++){

when you should write

 for (int x =0;x<10;x++) {
 ^
 space

Also,

for (int y=0;y<things[x].getCollectionOfInts().length;y++)
{

is C/C++ style. In Java, it is customary to write

for (int y=0;y<things[x].getCollectionOfInts().length;y++) { 

Advice 6: lambdas

Stream.of(things).forEach(Thing->System...

Since Thing is a variable in that context and not a type, I would rename it to thing.

Advice 7: max of three

You have this:

z -> z[0] > z[1] && z[0] > z[2] ? z[0] : z[1] > z[2] ? z[1] : z[2]

A shorter way of writing the same is

z -> Math.max(z[0], Math.max(z[1], z[2]))

Advice 8: maxInt

You can write it as

public static int maxInt(int[] x) {
 int max = Integer.MIN_VALUE;
 for (int i : x) {
 max = Math.max(max, i);
 }
 return max;
}

Advice 9: redundant if statements

if(y==0)
 a = things[x].getCollectionOfInts()[y];
if(y==1)
 b = things[x].getCollectionOfInts()[y];
if(y==2)
 c = things[x].getCollectionOfInts()[y];

Only one of those if statements will be executed yet all the condition will be checked. Basically, you can do this:

if (y == 0) { 
 a = ...
} else if (y == 1) {
 b = ...
} else {
 c = ...
}

Advice 10: naked if statements

Once again, you have

if (y==0)
 a = things[x].getCollectionOfInts()[y];

More idiomatic Java is this:

if (y == 0) {
 a = ...
}

(Note the braces.)

Summa summarum

I had this in mind:

Thing.java

package net.tnm;
import java.util.Random;
public class Thing {
 private final Random r = new Random();
 private final int a = r.nextInt(), 
 b = r.nextInt(), 
 c = r.nextInt();
 private int[] collectionOfInts = new int[]{a, b, c};
 public int[] getCollectionOfInts() {
 return collectionOfInts;
 }
}

Driver.java

package net.tnm;
import java.util.stream.Stream;
public class Driver {
 public static void main(String[] args) {
 Thing[] things = new Thing[10];
 int a = 0, b = 0, c = 0;
 for (int x = 0; x < 10; x++) {
 things[x] = new Thing();
 for (int y = 0; y < things[x].getCollectionOfInts().length; y++) {
 switch (y) {
 case 0:
 a = things[x].getCollectionOfInts()[0];
 break;
 case 1:
 b = things[x].getCollectionOfInts()[1];
 break;
 case 2:
 c = things[x].getCollectionOfInts()[2];
 break;
 }
 }
 //print 3 numbers in each Thing object
 System.out.println(x + ": " + a + ", " + b + ", " + c);
 }
 System.out.println();
 Stream.of(things)
 .forEach(thing -> System.out.println(
 Stream.of(thing.getCollectionOfInts())
 .mapToInt(z -> Math.max(z[0], Math.max(z[1], z[2])))
 .reduce((x, y) -> x + y)
 .getAsInt()
 )
 );
 System.out.println();
 Stream.of(things)
 .forEach(thing -> System.out.println(
 Stream.of(thing.getCollectionOfInts())
 .mapToInt(x -> maxInt(x))
 .max()
 .getAsInt()));
 }
 public static int maxInt(int[] x) {
 int max = Integer.MIN_VALUE;
 for (int i : x) {
 max = Math.max(max, i);
 }
 return max;
 }
}

• Roland Illig: Your answer is missing explanations for many of your suggestions. Remember that the OP is a beginner. +++ Why would net.tnm be an appropriate package name? +++ In idiomatic Java code, there's no reason to write your own max helper function. +++ The switch is completely useless and can be replaced by 3 simple variable assignments, it's a pattern that often appears in beginner code.