current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
1

I've written an algorithm which can multiply two hex values and return a hex as a result. What do you think about the below solution?

package com.datastructute.arraystring;

public class hexadecimal {
    public static void main(String[] args) {

        System.out.println(multiplyHex("AE08FE2111111111", "BF"));
        System.out.println(multiplyHex("DF", "BC"));        // A3C4
        System.out.println(multiplyHex("ADF398", "BA48"));  // 7e93e8f2c0

        // Test Screnarios
        System.out.println(multiplyHex(null, null));
        System.out.println(multiplyHex("  ", "  "));
        System.out.println(multiplyHex("hyh", "hyhy"));
        System.out.println(multiplyHex("abyh", "ashyhy"));
        System.out.println(multiplyHex("-1-1-1", "-1-1-1"));
        System.out.println(multiplyHex("AE08FE2111111111AE08FE2AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE2111111111AE08FE21111111111111AE08FE2111111111AE08FE21111111111111AE08FE21111AE08FE211111111111111", "AE0AE08FE21111111118FE2111111111AE08FE2111111111")); 
    }

    public static String multiplyHex(String str1, String str2)  {
        if(str1 == null || str2 == null)
            return "Null values are not accepted.";

        char[] hex1 = str1.toCharArray();
        char[] hex2 = str2.toCharArray();

        int[][] ArrHexMatrix;
        int arrLength = hex1.length + hex2.length;
        int arrIndexLength = hex1.length + hex2.length - 1;
        int lines = hex2.length;
        ArrHexMatrix = new int[hex2.length][arrLength];

        int mod = 0;
        int carry = 0;
        int count = 0;
        int index = 0;

        for (int i = lines - 1; i >= 0; i--) {

            carry = 0;
            count = 0;
            for (int j = hex1.length - 1; j >= 0; j--) {
                try {
                    if(getInt(hex2[i])==-1 || getInt(hex1[j])==-1)
                        return "Wrong chracter";

                    mod = (getInt(hex2[i]) * getInt(hex1[j]) + carry) % 16;
                    carry = ((getInt(hex2[i]) * getInt(hex1[j])) + carry) / 16;

                    if (j == 0) {
                        ArrHexMatrix[index][arrIndexLength - count - index] = mod;
                        ArrHexMatrix[index][arrIndexLength - count - 1 - index] = carry;
                    } else {
                        ArrHexMatrix[index][arrIndexLength - count - index] = mod;
                    }

                } catch (Exception e) {
                    e.printStackTrace();
                }
                count++;
            }
            index++;
        }

        int sum = 0;
        mod = 0;
        carry = 0;
        count = 0;

        char[] total = new char[arrLength];
        for (int i = arrLength - 1; i >= 0; i--) {
            sum = 0;
            for (int j = 0; j < lines; j++) {
                sum += ArrHexMatrix[j][i];
            }
            mod = (sum + carry) % 16;
            carry = (sum + carry) / 16;

            try {
                total[i] = getChar(mod);
            } catch (Exception e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }

        return String.valueOf(total);
    }

    private static int getInt(char chr) {
        switch (chr) {
        case '0':
            return 0;
        case '1':
            return 1;
        case '2':
            return 2;
        case '3':
            return 3;
        case '4':
            return 4;
        case '5':
            return 5;
        case '6':
            return 6;
        case '7':
            return 7;
        case '8':
            return 8;
        case '9':
            return 9;
        case 'A':
            return 10;
        case 'B':
            return 11;
        case 'C':
            return 12;
        case 'D':
            return 13;
        case 'E':
            return 14;
        case 'F':
            return 15;
        default:
            return -1;
        }

    }

    private static char getChar(int val) throws Exception {
        switch (val) {
        case 0:
            return '0';
        case 1:
            return '1';
        case 2:
            return '2';
        case 3:
            return '3';
        case 4:
            return '4';
        case 5:
            return '5';
        case 6:
            return '6';
        case 7:
            return '7';
        case 8:
            return '8';
        case 9:
            return '9';
        case 10:
            return 'A';
        case 11:
            return 'B';
        case 12:
            return 'C';
        case 13:
            return 'D';
        case 14:
            return 'E';
        case 15:
            return 'F';
        default:
            throw new Exception();
        }
    }
}
share|improve this question
add comment

3 Answers

up vote 9 down vote

Your getInt and getChar methods are very long and cumbersome, they can be replaced by this:

Using arrays:

private char[] values = new char[]{ '0', '1', '2', '3', '4', '5', '6', '7',
      '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };

char getInt(int i) {
    return values[i];
}

Java has already built-in methods to do this conversion though, so all that is needed is really:

int getInt(char chr) {
    return Character.digit(chr, 16);
}

char getChar(int val) {
    return Character.forDigit(val, 16);
}

Hexadecimal values are often showed in lower-case, personally I like that better because it makes it easier to separate the characters (a-f) from the digits (0-9). If you still want to use upper-case characters though, use this:

return Character.toUpperCase(Character.forDigit(val, 16));

    private static char getChar(int val) throws Exception {

Don't throw Exception, use IllegalArgumentException instead. IllegalArgumentException is a RuntimeException and therefore does not need to be caught (or declared in the throws declaration), but you are still free to do so if you would like. (Although I would not recommend it)

Learn to use JUnit to do the testing.

import static org.junit.Assert.*;

public class MultiplyHexTest {
    @Test
    public void testSomething() {
        assertEquals("A3C4", multiplyHex("DF", "BC"));
    }

    @Test(expected = IllegalArgumentException.class)
    public void testFailure() {
        multiplyHex("  ", "  ");
    }
}

ArrHexMatrix, being a name for a variable, should start with a lowercase character according to the Java naming conventions.

share|improve this answer
    
Thank you Simon, i ll look at junit testing. As you see the time complexity is O(n^2), do you know any better solution for this problem ? –  talhakosen 21 hours ago
    
@talhakosen The BigInteger class also has a multiply method with complexity O(n^2), so unfortunately I don't know any faster solution. –  Simon André Forsberg 18 hours ago
1  
+1 for suggesting JUnit. Reading the console output to verify results is time-consuming and error-prone. –  Robert Snyder 17 hours ago
    
ok thanks @SimonAndréForsberg –  talhakosen 17 hours ago
add comment
up vote 8 down vote

Exceptions:

It is common practice to throw an IllegalArgumentException if the input values to a method are not legal. In the sepcific case of null values, there is debate about whether the right response is an IllegalArgumentException, or a NullPointerException. I prefer IllegalArgumentException....

Also, in Java, (and most languages), it is very easy to introduce bugs when making small changes to 1-liner conditionals (just ask apple). When making the changes correctly to 1-liners (without introducing bugs) the actual small changes require adding in braces, so small changes change a lot. In general, use braces, even for 1-liners.

The code:

    if(str1 == null || str2 == null)
        return "Null values are not accepted.";

Should be:

    if(str1 == null || str2 == null) {
        throw new IllegalArgumentException("Null values are not accepted.");
    }

One of the reasons for the exception, by the way, is... consider the following code:

System.out.println(multiplyHex("A", multiplyHex("B", null));

With your code in the current state, the result will be:

   "Wrong chracter"

Additionally, the "wrong character" message is .... useless. You shoudl have an exception that indicates what the wrong character is:

int hxval1 = getInt(hex1[j]);
if (hxval1 < 0) {
    throw new IllegalArgumentException(String.format("Illegal character '%s' at position %d in the input value: %s", hex1[j], j, str1));
}

Use exceptions

share|improve this answer
    
Thanks rolfl, i got it. As you see the time complexity is O(n^2), do you know any better solution for this problem ? –  talhakosen 21 hours ago
add comment
up vote 0 down vote

Why not just convert the hex to decimal, then multiply the decimals, then convert back to hex? This seems like an obvious O(n) solution to the problem.

share|improve this answer
4  
If it's a long hexadecimal String, then you'd probably have to use the BigInteger class, which has a multiply-method that is also \$O(n^2)\$ –  Simon André Forsberg 20 hours ago
    
thanks for your suggestion @Julian –  talhakosen 17 hours ago
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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