HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Is there a better version for verbosing the output of the euclidean method?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
themethodversioneuclideanoutputbetterforthereverbosing

Problem

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;
        }

    }

 }

Solution

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.

Code Snippets

if (a < b) {
    r = a;
    a = b;
    b = r;
}
if (a < b) {
    int temp = a;
    a = b;
    b = temp;
}
int r = a % b;
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));
}
public static void main(String[] args) {
    System.out.println(gcd(24, 16, (a, b, r) -> System.out.println(a + "=" + a / b + "*" + b + "+" + r)));
}

Context

StackExchange Code Review Q#55262, answer score: 6

Revisions (0)

No revisions yet.