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.

The following code works in Python but seems to return zero in Java for some reason. Can you point out any errors?

import java.util.ArrayList;
import javax.swing.JOptionPane;

public class Test {
    public static void main(String[] args) {
        boolean run = true;
        String s = null;
        while (run){
            s = JOptionPane.showInputDialog(null, "Enter a string [quit to exit]: ");
            if (s.equalsIgnoreCase("quit")){
                run = false;
            }
            try{
                JOptionPane.showMessageDialog(null, "The string you entered can make " + permute(s) + " more words", "SUCCESS!", JOptionPane.INFORMATION_MESSAGE);
            }
            catch (Exception e){
                System.out.println("possible division by zero happened");
            }

        }
    }
    /*Newbie function for finding out the occurrence of a char in a string
    works good for the task at hand */
    public static int count(char CHAR, String s){
        int counter = 0;
        for (int i = 0 ; i < s.length() ; i++){
            if (s.charAt(i) == CHAR )
                counter++;

        }
        return counter;
    }

    //The actual permute function, takes a string
    public static final long permute(String s) throws Exception {
        ArrayList<Integer> pqr = new ArrayList<>();
        ArrayList<Character> checkList = new ArrayList<>();
        s = s.toLowerCase();
        int n = s.length();
        int c = 0;
        char character = 0;
        for (int i = 0 ; i < n ; i++){
            character = s.charAt(i);
            c = count( character, s );
            if (c>1 && !checkList.contains(character)){
                pqr.add(c);
                checkList.add(character);
            }
        }
        long r = 0;
        for (int j : pqr)
            r *= fact(j);
        return fact(n) / r;
    }
    //For finding out factorials
    public static long fact(int number){
        if (number <= 1){
            return 1;
        } else {
            return number * fact(number - 1);
        }
    }
}
share|improve this question
1  
On Code Review we strictly help improve working code. –  Winston Ewert Jun 30 '12 at 17:28
add comment

closed as off topic by codesparkle, Jeff Mercado, Winston Ewert Jun 30 '12 at 17:27

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

Maybe

long r = 0;

should be

long r = 1;

If the initial value of r is zero the r *= fact(j) expression will be zero.

Furthermore, currently it does not compile, you have to change

ArrayList<Integer> pqr = new ArrayList<>();
ArrayList<Character> checkList = new ArrayList<>();

to

ArrayList<Integer> pqr = new ArrayList<Integer>();
ArrayList<Character> checkList = new ArrayList<Character>();

Some other notes:

  1. Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used.

    See Effective Java, Second Edition, Item 45: Minimize the scope of local variables. (Google for "minimize the scope of local variables", it's on Google Books too.))

  2. Refer to objects by their interfaces, the declaration of the lists should be

    List<Integer> pqr = new ArrayList<Integer>();
    List<Character> checkList = new ArrayList<Character>();
    

    See Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

  3. Parameter names shouldn't be uppercase, like CHAR. I'd rename it to searchChar.

    See Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions

  4. The run flag could be substituted with a break:

    while (true) {
        final String string = JOptionPane.showInputDialog(null, "Enter a string [quit to exit]: ");
        if (string.equalsIgnoreCase("quit")) {
            break;
        }
        ...
    }
    
share|improve this answer
1  
THANK YOU! i really need effective java :) –  BUCKSHOT Jul 1 '12 at 7:16
add comment

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