1
\$\begingroup\$

I wrote a ROW (Race of work) simulation, the idea contains a mathematical bug and, I asked a question about it here.

I would also like to make this code more readable and, this is why I am sharing it here. Please review it and tell me how I can style it better or improve performance.

I posted it in a gist on GitHub.

import java.math.BigDecimal;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.Random;
import java.util.UUID;

/**
 * This is an experimental proof that in
 * <a href="https://confidence-coin.com/race-of-work-blockchain-without-forks/">ROW</a>,
 * Race of work have, your chances to mine a block are equal to your
 * computational power percentage from the entire network, or in other words, the chance to win are equally distributed.
 *
 * @author Ilya Gazman
 */
public class RowTest {

    private MessageDigest sha256;

    private RowTest() throws NoSuchAlgorithmException {
        sha256 = MessageDigest.getInstance("SHA-256");
    }

    public static void main(String... args) throws NoSuchAlgorithmException {
        new RowTest().start();
    }

    private void start() {
        ArrayList<Integer> list = new ArrayList<>();
        Random random = new Random();
        list.add(10);
        while (sum(list) < 100) {
            int value = random.nextInt(100 - sum(list) + 1);
            if (value == 0 || list.contains(value)) {
                continue;
            }
            list.add(value);
        }
        System.out.println(sum(list) + " " + list.size());

        byte[][] players = new byte[list.size()][];
        int[] score = new int[list.size()];
        double[] reword = new double[list.size()];

        String guid = UUID.randomUUID().toString();

        float totalGames = 1_000;
        for (int i = 0; i < totalGames; i++) {
            for (int j = 0; j < list.size(); j++) {
                int player = list.get(j);
                byte[] min = null;
                for (int k = 0; k < player; k++) {
                    byte[] hash = hash(i, guid, k, player);
                    if (hash.length != 32) {
                        throw new Error("Hash error");
                    }
                    if (min == null || compare(hash, min) == -1) {
                        min = hash;
                    }
                }
                players[j] = min;
            }
            score[findWinner(players)]++;

            rewordPlayers(players, reword);
        }

        for (int i = 0; i < score.length; i++) {
            System.out.println(list.get(i) + " won\t" + score[i] / totalGames * 100 + "%\tof the times, he earned\t" + reword[i]);
        }

        double totalReword = 0;
        for (double v : reword) {
            totalReword += v;
        }
        System.out.println("\nTotal reword " + totalReword + " / " + totalGames);
    }

    private void rewordPlayers(byte[][] players, double[] reword) {
        int rounding = BigDecimal.ROUND_CEILING;
        int scale = 32;

        BigDecimal total = BigDecimal.ZERO;
        for (byte[] player : players) {
            BigDecimal playerReword = new BigDecimal(new BigInteger(player));
            total = total.add(BigDecimal.ONE.divide(playerReword, scale, rounding));
        }

        for (int j = 0; j < players.length; j++) {
            BigDecimal playerReword = new BigDecimal(new BigInteger(players[j]));
            BigDecimal a = BigDecimal.ONE.divide(playerReword, scale, rounding);
            BigDecimal b = a.divide(total, scale, rounding);
            reword[j] += b.doubleValue();
        }
    }

    private int findWinner(byte[][] players) {
        byte[] min = null;
        int winner = -1;
        for (int i = 0; i < players.length; i++) {
            byte[] hash = players[i];
            if (min == null || compare(hash, min) == -1) {
                min = hash;
                winner = i;
            }
        }
        return winner;
    }

    /**
     * if a > b return 1 else if a < b return -1 else return 0
     */
    private static int compare(byte[] a, byte[] b) {
        int aLength = a.length;
        int bLength = b.length;

        for (int i = a.length - 1; i >= 0 && a[i] == 0; i--) {
            aLength--;
        }

        for (int i = b.length - 1; i >= 0 && b[i] == 0; i--) {
            bLength--;
        }

        if (aLength > bLength) {
            return 1;
        } else if (bLength > aLength) {
            return -1;
        }

        for (int k = 0; k < aLength; k++) {
            int A = a[k] & 0xff;
            int B = b[k] & 0xff;
            if (A > B) {
                return 1;
            }
            if (A < B) {
                return -1;
            }
        }
        return 0;
    }

    private byte[] hash(int i, String value, int k, int player) {
        value = i + "," + value + "," + k + "," + player;
        return sha256.digest(value.getBytes());
    }

    private int sum(Iterable<Integer> list) {
        int total = 0;
        for (Integer value : list) {
            total += value;
        }
        return total;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ "the idea contains a mathematical bug" Broken code is off-topic, does it work as intended? \$\endgroup\$ – Ludisposed Jul 24 '18 at 12:52
  • \$\begingroup\$ @Ludisposed yeah, I plan to upgrade it in the future but for now, it's doing what it should be doing \$\endgroup\$ – Ilya Gazman Jul 24 '18 at 12:54
0
\$\begingroup\$

Very quick review here, formatting is good, so I focused on good practices, reuse of standard Java libraries (Streams), naming etc.

To have this code more organized I would embrace Objects and create a Player class, maybe a Game class. But given the short-lived nature of these objects and their quantity, it's ok not to.

Here's a reviewed (untested) code (in particular, I have no idea what it is supposed to do, so I don't know if it still works... but it should ^^)

import java.math.BigDecimal;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Random;
import java.util.UUID;

public class RowTest {

    // Make this final, because it effectively is
    private final MessageDigest sha256;

    // This is an internal constant!
    private static final int SCALE = 32;

    private RowTest() throws NoSuchAlgorithmException {
        sha256 = MessageDigest.getInstance("SHA-256");
    }

    public static void main(String... args) throws NoSuchAlgorithmException {
        new RowTest().start();
    }

    private void start() {
        // Declare types of the most generic type (here, interface List is sufficient). Try to name it something meaningful ?
        List<Integer> games = new ArrayList<>();
        Random random = new Random();
        games.add(10);
        while (sum(games) < 100) {
            int value = random.nextInt(100 - sum(games) + 1);
            // Using contains multiple times on a list of numbers? consider using a Hash structure
            if (value == 0 || games.contains(value)) {
                continue;
            }
            games.add(value);
        }
        System.out.println(sum(games) + " " + games.size());

        byte[][] players = new byte[games.size()][];
        int[] score = new int[games.size()];
        double[] reword = new double[games.size()];

        String guid = UUID.randomUUID().toString();

        float totalGames = 1_000;
        for (int i = 0; i < totalGames; i++) {
            for (int j = 0; j < games.size(); j++) {
                int player = games.get(j);
                byte[] min = null;
                for (int k = 0; k < player; k++) {
                    byte[] hash = hash(i, guid, k, player);
                    if (hash.length != 32) {
                        throw new Error("Hash error");
                    }
                    if (min == null || compare(hash, min) == -1) {
                        min = hash;
                    }
                }
                players[j] = min;
            }
            // Split these operations. For debugging, it'll be easier to follow! It's bad practice to russian-doll method calls with array access and unary operations. Method calls alone might pass.
            final int winner = findWinner(players);
            score[winner]++;

            rewordPlayers(players, reword);
        }

        for (int i = 0; i < score.length; i++) {
            System.out.println(games.get(i) + " won\t" + score[i] / totalGames * 100 + "%\tof the times, he earned\t" + reword[i]);
        }

        // Use streams everywhere! they are good for you
        double totalReword = Arrays.stream(reword).sum();

        System.out.println("\nTotal reword " + totalReword + " / " + totalGames);
    }

    private void rewordPlayers(byte[][] players, double[] reword) {
        // No need to store this locally, especially this the var name doesn't mentions which rounding is done
        // int rounding = BigDecimal.ROUND_CEILING;


        // Guess what? we can stream this :)
        BigDecimal total = Arrays.stream(players) // Streaming all players
                .map(RowTest::dividePlayer) // calculating the inverse
                .reduce(BigDecimal::add)// adding all
                .orElse(BigDecimal.ZERO); // If no player, then zero

        for (int j = 0; j < players.length; j++) {
            BigDecimal a = dividePlayer(players[j]);
            BigDecimal b = a.divide(total, SCALE, BigDecimal.ROUND_CEILING);
            reword[j] += b.doubleValue();
        }
    }

    // For easier reading, i'm extracting this code to reduce redundancy
    private static BigDecimal dividePlayer(byte[] player) {
        BigDecimal playerReword = new BigDecimal(new BigInteger(player));
        return BigDecimal.ONE.divide(playerReword, SCALE, BigDecimal.ROUND_CEILING);
    }


    // Make everything you can 'static', makes it easy to see it doesn't change object state
    private static int findWinner(byte[][] players) {
        return IntStream.range(0, players.length)// Browse all players
                        .reduce((i, j) -> compare(players[i], players[j]) < 0 ? i : j)// For any two, pick index of the lowest
                        .orElse(-1); // Default value
    }

    /**
     * if a > b return 1 else if a < b return -1 else return 0
     */
    private static int compare(byte[] a, byte[] b) {
        int aLength = a.length;
        int bLength = b.length;

        // More concise.
        // Also you're just looking for the longest streak of '0' at the end of the byte.
        // A 'while' loop is much more useful and shows that the loop is *supposed* to be interrupted.
        // A 'for' loop is assumed to (generally) span the entire range regardless.
        while (aLength >= 0 && a[aLength] == 0) {
            aLength--;
        }
        while (bLength >= 0 && b[bLength] == 0) {
            bLength--;
        }

        // Why not just return aLength - bLength ? compare can return any number. Only its sign matter.
        if (aLength > bLength) {
            return 1;
        } else if (bLength > aLength) {
            return -1;
        }

        for (int k = 0; k < aLength; k++) {
            // avoiding having variables with only differing capitalization!
            int maskedA = a[k] & 0xff;
            int maskedB = b[k] & 0xff;
            // Why not just return maskedA - maskedB ? compare can return any number. Only its sign matter.
            if (maskedA > maskedB) {
                return 1;
            }
            if (maskedA < maskedB) {
                return -1;
            }
        }
        return 0;
    }

    private byte[] hash(int i, String value, int k, int player) {
        // Never reassign a reference given to you in input!
        String appended = i + "," + value + "," + k + "," + player;
        return sha256.digest(appended.getBytes());
    }

    // Again, make this static!
    private static int sum(List<Integer> list) {
        // can be easily replaced with streams, for better performance etc.
        return list.stream().mapToInt(i -> i).sum();
    }
}
\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.