2
\$\begingroup\$

My code is working fine Could you please review how I can make it more accurate and efficient?

import java.util.Scanner;

public class LinearProbing {

    public static void hash(String store[],int index,int num){
        if (store[index].equals(" ")) {
            store[index] = String.valueOf(num);
        } else {
            index++;
            if(index==10){
                index=0;
            }
            hash(store,index,num);
        }

    }

    public static void main(String args[]) {
        String store[] = new String[10];
        for (int a = 0; a < store.length; a++) {
            store[a] = " ";
        }

        Scanner input = new Scanner(System.in);

        int num;
        int index;

        for (int a = 0; a < store.length; a++) {
            System.out.print("Enter Num: ");
            num = input.nextInt();
            index = num % 10;
            hash(store,index,num);
        }

        for (int a = 0; a < store.length; a++) {
            System.out.println("Index: "+a+ "Store: "+store[a]);
        }


    }

}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  1. You do not have to initialize string array with "". By default array values will be initialized with null. So afterwards, you can check if (store[index] == null).

  2. You are already doing '%' operation in for loop, no need to check if(index == 10)

  3. String array is not required at all as mentioned by @kraskevich. But if you do need, make it as a property of class LinearProbbing. So that you do not have to pass it within methods.

  4. No actual implementation of Hash is involved. So better rename methods.

\$\endgroup\$
1
\$\begingroup\$
  1. I'd recommend to move declare store 10 in a final variable with a meaningful name. It's not clear what it stands for right now. It's the number of buckets, isn't it? You can improve the readability by calling NUMBER_OF_BUCKETS or BUCKETS_COUNT.

  2. The hash table actually stores numbers. There's no need to convert them to strings. It works slower and makes no sense. The meaning of the " " constant is also unclear (Why not null? The null clearly stands for no value). But again, it doesn't matter if you fix it and start treating integers as integers. Converting everything to a String is a bad practice (unless the data is actually a string).

  3. The name of the function hash is also meaningless. It doesn't compute any hash. The name doesn't reflect what it actually does. It should be something like insert or add. I'd also create a separate hash table class and move this there (and make it non-static). That would reflect its actual purpose: adding an element to the hash table.

  4. Having a fixed size 10 for your hash table makes its next to useless in practice (what if you want to add more elements?).

  5. The hash function can be iterative (you can just loop until you find a free cell). It would save a lot of stack space if you decide to make the table bigger in the future.

To sum up, I'd do the following:

  • Get rid of "magic" constants.

  • Treat integers as integers, not strings

  • Move the hash table into a separate HashTable class

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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