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.

Here is my implementation of the Euclidean Algorithm. My question is how to make it more "professional". It's working right, but isn't this too newbie?

public class Euklideszi {

    public int Euklideszi(int a, int b, boolean lepesrolLepesre) {
        int r = a % b;
        if (a < b) {
            r = a;
            a = b;
            b = r;
        }
        if (a % b == 0) {
            if (lepesrolLepesre) {
                System.out.println("A legnagyobb közös osztó: " + b);
            }
            return b;
        } else {
            while (r != 0) {
                r = a % b;
                if (lepesrolLepesre) {
                    System.out.println(a + "=" + a / b + "*" + b + "+" + r);
                }
                a = b;
                b = r;
            }
            if (lepesrolLepesre) {
                System.out.println("A legnagyobb közös osztó: " + a);
            }
            return a;
        }

    }

 }
share|improve this question
2  
You could start with writing everything in English. While code in your own language may definitely be considered professional in some branches, it is not the way to go with the current internationalization. –  skiwi Jun 25 at 19:28
add comment

5 Answers 5

There are a couple of "newbie" things here, yes.

  • Your method has an uppercase first character, it should be lower-case.

  • Your method is not static, which means that an instance of the class has to be created just to use your method. You can make the method static, and add a private constructor to the class so that it can't be initialized from outside.

  • As @skiwi says in a comment, please use English identifier names when you code.

  • boolean lepesrolLepesre is used to toggle whether or not the method should log anything to console. This is a bad thing. A method like this should not output anything to the console, it should simply return the result.

  • This code:

    if (a < b) {
        r = a;
        a = b;
        b = r;
    }
    

    is used to switch so that a always is the largest variable, and it's using r as a temporary variable. Even though this works, I would use a temp variable here that's only visible in this scope.

    if (a < b) {
        int temp = a;
        a = b;
        b = temp;
    }
    int r = a % b;
    

    Note that here, I put the initial calculation of r after this switch has taken place.

  • There is no need to use the else as you return inside the if. You can remove else and reduce the indentation there.

Edit:

In the comments below you asked for a cleaner way for the user to see every step, you can accomplish this by using a callback interface.

Then you can modify your code to:

public static interface EuklidesStepCallback {
    void onStep(int a, int b, int r);
}
public static int gcd(int a, int b, EuklidesStepCallback callback) {
    ...
    while (r != 0) {
        r = a % b;
        if (callback != null) {
            callback.onStep(a, b, r);
        }
        a = b;
        b = r;
    }
    return a;
}   

public static void main(String[] args) {
    EuklidesStepCallback callback = new EuklidesStepCallback() {
        @Override
        public void onStep(int a, int b, int r) {
            System.out.println(a + "=" + a / b + "*" + b + "+" + r);
        }
    };
    System.out.println(gcd(24, 16, callback));
}

If you're able to use Java 8, you can use a lambda for the callback:

public static void main(String[] args) {
    System.out.println(gcd(24, 16, (a, b, r) -> System.out.println(a + "=" + a / b + "*" + b + "+" + r)));
}

This callback system is more flexible in case you'd want to save each of the steps to a List or similar.

share|improve this answer
    
Thanks for suggestions. I want the steps to be visible if the user wants to see every step. So I did the printlines deliberately. Now I see, it's not a good solution, but how can I print every step if I want? –  molnardenes Jun 25 at 21:59
    
@molnardenes Excellent question! I've edited my answer and provided a clean way to do this. Although using Loggers, as suggested by 200_success, is also an option - unless you some day might want to save the steps to a list. –  Simon André Forsberg Jun 25 at 22:13
add comment

You are rightly concerned about logging. System.out.println() is fine during development, but should be taken out before you consider your code to be "complete". Using a boolean flag to trigger verbose execution is a bad idea, since it clutters the function's interface for purposes that have nothing to do with your algorithm. Furthermore, you'll still need a mechanism for switching the printouts on and off, preferably without recompilation.

Fortunately, there is a solution: the Java Logging API.

In your code:

import java.util.logging.*;

public class Euklideszi {

    private static final Logger logger = Logger.getLogger("Euklideszi");

    public static int gcd(int a, int b) {
        …
        logger.fine(a + "=" + a / b + "*" + b + "+" + r);
        …
    }
}

(I've changed the method signature, not only to drop the boolean flag. The function should be static, so that you don't have to create a new Euklideszi() to call it. Also, gcd() is a more useful name. Maybe lko(), if you insist.)

By default, messages at the FINE level are suppressed. To enable them, create a euklideszi.properties file containing:

handlers = java.util.logging.ConsoleHandler
.level = FINE
java.util.logging.ConsoleHandler.level = FINE
java.util.logging.SimpleFormatter.format = %2$s: %5$s%n

Then, run Java using the command-line parameter

java -Djava.util.logging.config.file=euklideszi.properties …

to use that configuration. (There are other ways to configure logging — read the documentation.)

share|improve this answer
    
gcd = Greatest Common Divisor, but what's lko? –  Simon André Forsberg Jun 25 at 22:01
1  
@SimonAndréForsberg legnagyobb közös osztó –  200_success Jun 25 at 22:03
add comment

Your algorithm implementation seems to be excessively cumbersome.

For example, the first line is int r = a % b;, but then you discard r by using it as temporary storage space for swapping a and b. Then you compute a % b again.

But the heart of the algorithm is in the while loop. I believe you can discard the if (a % b == 0) special case if you write that loop properly.

public static final Logger logger = Logger.getLogger("Euklideszi");

public static int gcd(int a, int b) {
    // Without loss of generality, let a >= b
    if (a < b) {
        int swap = a;
        a = b;
        b = swap;
    }

    int r;
    while ((r = a % b) != 0) {
        logger.fine(a + "=" + a / b + "*" + b + "+" + r);
        a = b;
        b = r;
    }
    logger.fine("A legnagyobb közös osztó: " + b);
    return b;
}
share|improve this answer
    
As @bowmore points out, the swap is unnecessary. –  200_success Jun 29 at 3:17
add comment

I am not trying to be rude but just think about your question.

You are asking your question in English, and you want us to review your code. But than your code is not English.

One small thing that I saw: you have have if condition, that print some thing if it's true, in two different places. You can convert it to method.

if (lepesrolLepesre) {
     System.out.println("A legnagyobb közös osztó: " + a);
}

private yourConditionMeaning(boolean condition, int value){
     if (condition) {
          System.out.println("A legnagyobb közös osztó: " + value);
     }
}
share|improve this answer
    
Why do you have a boolean argument if you do not use it ? –  Marc-Andre Jun 25 at 20:38
    
@Marc-Andre oh yeah, missed that. Tnx –  Ilya_Gazman Jun 25 at 20:43
add comment

I immediately wondered why you method was so long. Of course the verbose related code added quite some bloat, but even then, the code could be as short as this (I used the more traditional name gcd for greatest common divisor) :

public int gcd(int a, int b) {
    for (int r = a % b; r != 0; r = a % b) {
        a = b;
        b = r;
    }
    return b;
}

How do I get there from your code?

Step 1 : remove code that handles verbose, and rename method :

public int gcd(int a, int b) {
    int r = a % b;
    if (a < b) {
        r = a;
        a = b;
        b = r;
    }
    if (a % b == 0) {
        return b;
    } else {
        while (r != 0) {
            r = a % b;
            a = b;
            b = r;
        }
        return a;
    }

}

Now if a is not the larger of a and b you switch their values. Ironically, a property of Euclid's algorithm is that it automatically performs this switch by itself. So the code to make sure a is greater than b is superfluous (we kick it)

public int gcd(int a, int b) {
    int r = a % b;
    if (a % b == 0) {
        return b;
    } else {
        while (r != 0) {
            r = a % b;
            a = b;
            b = r;
        }
        return a;
    }

}

Checking whether b is a divisor if a explicitly is also superfluous (again the algorithm handles this by itself already) we kick that too.

public int gcd(int a, int b) {
    int r = a % b;
    while (r != 0) {
        r = a % b;
        a = b;
        b = r;
    }
    return a;
}

Now if r becomes 0 we're done, yet this form still moves b into a and r into be before checking. Converting this to a for loop solves that problem, note that with this conversion our answer now is b instead of a, once we exit the loop.

public int gcd(int a, int b) {
    for  (int r = a % b; r != 0; r = a % b) {
        a = b;
        b = r;
    }
    return b;
}

But we're not done. This code doesn't check input validity (0 values are not dealt with properly, negative values are not dealt with properly)

Other answers suggest you to make this method static. I would advise against that. While it is more convenient to use, it also introduces greater coupling of clients. In fact I would suggest having this class implement an interface and allow different algorithms to be implemented as classes implementing that interface (a different implementation may be using shift operations to extract the common power of 2 from the arguments)

For verbosity I can only also recommend the callback approach, although I would be registering callback instances seperately from calling the method.

But I would also question the use of adding verbose behaviour to the class at all. It seems to have no value other than debugging. If that is the case, you could simply use a debugger.

If the verbosity is needed because this method is used as part of a command line tool, I think this level of detail is unwanted or unneeded. Verbose output should help the user spot problems with their inputs. For such verbose output, using the decorator pattern should do the trick (this is also a good reason to have this class implementing an interface). The verbose decorator then simply produces output (without if checks) before delegating to the non verbose delegate.

share|improve this answer
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.