2
\$\begingroup\$

Can someone critique this Java code?

package javaapplication;

import java.util.Date;
import java.lang.Long;

interface infix_operator {

    public int apply(int x, int y);
}

class f implements infix_operator {

    public int apply(int x, int y) {
        return x + (2 * y);
    }
}

class g implements infix_operator {

    public int apply(int x, int y) {
        return (2*x) + y;
    }
}

class h implements infix_operator {

    public int apply(int x, int y) {
        return x + y;
    }
}

class k implements infix_operator {

    public int apply(int x, int y) {
        return x * y;
    }
}

public class JavaApplication {

    static long to_64bit_signed_integer (long value)
    {
        long bit16 = value & 65535;
        value = (bit16 << 16) | bit16;
        value = (value << 16) | bit16;
        value = (value << 16) | bit16;
        return Long.reverse(value);
    }

    public static void main(String[] args) {
        long current_time_value = new Date().getTime();
        System.out.println("Current Time: " + current_time_value);

        infix_operator[] operator_array = {new f(), new g(), new h(), new k()};

        int [] node = new int[31];
        int [] to_signed_value = new int[] {0,-2,1,-1};
        int [] to_unsigned_value = new int[] {0,2,1,3};

        long value = to_64bit_signed_integer (current_time_value);

        value = value >> 2;

        for (int leaf_index=30; leaf_index>=15; leaf_index--)
        {
            node[leaf_index] = to_signed_value [(int)(value & 3)];
            value = value >> 2;
        }

        for (int internal_index = 14; internal_index >= 0; internal_index--)
        {
            int op = to_unsigned_value [(int)(value & 3)];
            int child_index = 2*internal_index + 1;
            node [internal_index] = operator_array[op].apply(node[child_index], node[child_index+1]);
            value = value >> 2;
        }

        System.out.println("Hash Value: " + node[0]);
    }
}
\$\endgroup\$
6
\$\begingroup\$

Just a couple of points which are more from a stylistic point-of-view rather than strict language requirements or algorithmic correctness:

  • Classes should be named upper-case: Car, House, TwoTimesPlusOne, Addition
  • Interfaces should also be named upper-case: InfixOperator
  • Variable names should use camel-case: leafIndex, unsignedValue
  • i, j and k are used as iterator variables (the more you reference a variable, the shorter it should be named)
  • Prefer lists over arrays and make use of the enhanced for loop

    for(InfixOperator i : operators) {
        //...
    }
    

Adhering to common language standards will make your code more easily understandable for other programmers.

\$\endgroup\$
4
  • \$\begingroup\$ I agree. Class and variable names should carry meaning otherwise they're confusing for the reader and the programmer. I really loathe it when people are so lazy that they only give single letter names to variables/classes. That sort of thing should really only be limited to for loops. \$\endgroup\$ – W.K.S Jun 28 '12 at 21:29
  • \$\begingroup\$ Instead of "limited to loops", I'd say that very short names are nice as long as their visibility / line-span is limited to a few lines (which is about the same as you said). \$\endgroup\$ – maaartinus Jun 29 '12 at 16:05
  • 2
    \$\begingroup\$ "These are by no means hard rules, feel free to break them as the need arises." - But only if you never intend another programmer to see your code. IMO, you should treat these as hard and fast rules, and only break them when you have no sensible alternative. Folks, this is a Code Review site. We should teach good habits, not excuses for bad habits. \$\endgroup\$ – Stephen C Jul 3 '12 at 11:03
  • \$\begingroup\$ @StephenC Agree, I removed it. \$\endgroup\$ – Matt Jan 23 '17 at 11:11
5
\$\begingroup\$

You can replace new Date().getTime() by System.currentTimeMillis()

For the rest of the code, it would help if you explain the algorithm you are implementing, or if you name it (if it's a known hash algorithm)

\$\endgroup\$
3
\$\begingroup\$

The algorithm takes the current time and hashes it somehow. If you want to implement a hash function, so you should do it, not only provide a composite thing. The way you did it you can only provide a single random number, that's not really very general use.

Is the algorithm you implemented any good? This depends on the goals (which you haven't stated).

Should it provide an unguessable long value? If so, it fails as there is just a thousand possible candidates each second. Compare it to new SecureRandom().nextLong().

Should the hash function be fast? Measure it and compare it to existing secure ones, there are dozens of them implemented in Java, including the SHA-3 candidates.Or compare it to the non-secure but fast ones like here.

Should it be secure? This is a nearly impossible task even for the smartest guys having studied all the theory behind.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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