3
\$\begingroup\$

I made a simple program that uses OOP methods. Please tell me if I used the 4 methods right. And if not, please show me how.

class Encap{
    private String name;
    public void setName(String newValue){
        name = "Hello, "+newValue+"!";
    }

    public String getName(){
        return name;
    }
}

abstract class Abstraction {
    int fibCount, flag=0, y;
    String inputName;
    String names[] = new String [] {"Anna","Joe","Carl"};
    Scanner x = new Scanner(System.in);
abstract void abscompute();
}

class abs2 extends Abstraction{
    @Override
    void abscompute() 
    {
         System.out.print("Enter number of fibonacci: ");
         fibCount = x.nextInt();
         int[] fib = new int[fibCount];
         fib[0] = 0;
         fib[1] = 1;
         for(int i=2; i < fibCount; i++){
             fib[i] = fib[i-1] + fib[i-2];
         }

         for(int i=0; i< fibCount; i++){
                 System.out.print(fib[i] + " ");

                }

    }
}
public class Fibonacci {

    public static void main(String[] args) {
        Abstraction abs = new abs2();
        Encap encap = new Encap();
        System.out.print("Enter your name: ");
        abs.inputName = abs.x.next();
        for (String name : abs.names) {
            if(abs.inputName.equals(name)){
                encap.setName(abs.inputName);
                System.out.println(""+encap.getName());
                abs.flag = 1;
            }
        }
        if(abs.flag!=1){
            System.out.println("Name doesn't exist..");
        }
        else{
            abs.abscompute();
        }
    }
}
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$
class Encap{

I find all your class names confusing. You seem to be describing the code, not what it does. Maybe something like

class Greeter {

would be more understandable.

    private String name;
    public void setName(String newValue){
        name = "Hello, "+newValue+"!";
    }

The name variable isn't a name, but a greeting. Why not call it that?

Also, newValue isn't very descriptive. That actually is a name, so

    private String greeting;

    public void setGreeting(String name) {
        greeting = "Hello, " + name + "!";
    }

Now I can easily see that setGreeting takes a name as an input.

Some people might argue that setGreeting should only set the greeting, not determine what it is. You could either create a constructor to handle this with an immutable variable, or you could change the name to changeGreeting. Or you can ignore that convention, but you may get negative feedback.

abstract class Abstraction {

This could be

abstract class Sequence {

    private int[] sequence;

    Sequence(int length) {
        sequence = new int[length];
    }

    int getLength() {
        return sequence.length;
    }

    abstract void generate();
}

This seems a clearer set of names.

You also may want to look into how to use constructors

    int fibCount, flag=0, y;

It's generally preferable to have one variable per declaration, particularly if you are initializing as well.

    int fibCount;
    int flag = 0;
    int y;

Now you can easily see that only flag is initialized.

Why declare y? You never use it.

Why make fibCount and flag object fields? You only use them in one method. They could be local variables instead.

    String names[] = new String [] {"Anna","Joe","Carl"};

This doesn't seem to have anything to do with the rest of this. Why declare that variable in this class?

    Scanner x = new Scanner(System.in);

Why name your Scanner x? Is it a secret that it is a scanner? More common names would be scan or scanner. You could also name it something like input or stdin.

I'd move the input and output out of the class and into main. Then you can make scanner a local variable as well.

class abs2 extends Abstraction{

Again, this would be clearer as

class FibonacciSequence extends Sequence {

Now I know what I expect to find here.

        for (String name : abs.names) {
            if(abs.inputName.equals(name)){
                encap.setName(abs.inputName);
                System.out.println(""+encap.getName());
                abs.flag = 1;
            }
        }
        if(abs.flag!=1){
            System.out.println("Name doesn't exist..");
        }
        else{
            abs.abscompute();
        }

This would be easier to read as a method call:

        if (validName(inputName)) {
            greeting.setGreeting(inputName);
            sequence.generate();
        }
        else {
            System.out.println("Name doesn't exist..");
        }

Then instead of

                abs.flag = 1;
            }
        }

You could say

                return true;
            }
        }

        return false;

There are other places with similar feedback, but perhaps you should take a try at it yourself.

\$\endgroup\$
0

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.