bad programming style??- (getter/setter methods )

[Wolfgang Obi]

Hi,
it's me once again
this is an assignment i did the other day.....
i was told though it works, it was programmed badly (i.e. that it is bad style to declare all the variables as static and public etc etc)...
I'm supposed to work with private variables that are to be acessed over 'getter()', and 'setter()' methods, .... how do i do this,...i must confess I'm pretty new to Java, and i havent a clue as to where im to start,... would i have to rewrite the entire programme?

help!
what ELSE is there to watch out for?

(PS: IM.readInt()... is a method used to read from the command line)
thanx.

 
public class HCF extends YesNo{
 
      public static int num1;
      public static int num2;
      public static int rest;
      public static int factor;
      public static int menuPoint;
 
      public static int gGt(int firstInt, int secondInt){
 
            rest = (firstInt % secondInt);
            if((num1 == 0) || (num2 == 0)){
               System.out.println("Pls. no Zeros, pls try again!");
               System.out.println();
               return 0;
 
            }
            if(rest == 0){
               factor = secondInt;
               System.out.println
               ("The Highest Common Factor(HCF) is: "+factor);
               System.out.println();
               return factor;
 
            }else{
                gGt(secondInt, rest); //recursive call!
            }
            return factor;
      }
 
      public static void Enter(){
               System.out.println("Please enter your two integers.");
               System.out.println();
               System.out.print("your first number please: ");
               num1 = IM.readInt();
               System.out.println();
               System.out.print("your second number please: ");
               num2 = IM.readInt();
               System.out.println();
               gGt(num1,num2);
      }
 
      public static void progInfo(){
               System.out.println();
               System.out.println              ("**********************************************************************");
               System.out.println
               ("********* Program: HCF written by XXXX *********** ");
               System.out.println
               ("**This program does the following.---.....---..... **");
               System.out.println
 ("**********************************************************************");
               System.out.println();
      }
      public static void trueORfalse(){
         enter = enter.toLowerCase();
         int result1 = userSaysYes(enter);
         if (result1 >= 0){
         System.out.println("okay, ride on...");
         Enter(); interaction();
         }else{
            int result2 = userSaysNo(enter);
            if (result2 >= 0){
               System.out.println("oky-dokie!...thanx! ;-)");
               //stop(1);
            }else
               System.out.println("Invalid entry,..pls Try again!");
               interaction();
         }
        }
 
      public static void interaction (){
         System.out.println("what do u want to do next? ");
         System.out.println();
         System.out.println(" '0' Key ---->> Again?");
         System.out.println();
         System.out.println(" '2' Key ---->> End?");
         System.out.println();
         System.out.println("Choose then press [ENTER] ");
         System.out.println();
         menuPoint = IM.readInt();
         System.out.println();
 
         switch(menuPoint){
 
            case 0 : System.out.println
           ("okay, go again ?");System.out.println();
            Enter();trueORfalse();break;
            //case 1 : System.out.println();
            case 2 : System.out.println(" Thanks and goodbye.. ;-)");
            System.out.println();break;
            default: System.out.println
           ("Entry NOT recognised! :-( ");interaction();break;
         }
      }
 
         public static void main(String[] noah){
 
            progInfo();
            Enter();
            interaction();
 
      }
}
   

[Ernest Friedman-Hill]

In this case, you've got only one class, so the get/set methods part really doesn't apply. But as to public vs. private variable and the general style questions... yes, you have some changes to make.

The first, most obvious one: member variables -- variables declared in a class body -- should with very few exceptions be declared private, not public. That's a trivial change, I'm sure you'll agree. Making variables private supports encapsulation, an important principle of object-oriented design -- consult your textbook for details.

Second, making everything static like this is bad style. Instead, make nothing static except for main(), then change the body of main to look like:

public static void main(String[] argv) {
  HCF app = new HCF();
  app.progInfo();
  app.Enter();
  app.interation();
}

Again, we're working toward object-oriented principles here; in this case, simply by using an HCF object rather than not.

Third, most of the time, methods should communicate by parameters and return values, not by passing things through member variables. If you want to give your friend a beer, do you do it by putting the beer on a shelf and asking him to take it off the shelf? No, you do it by handing the beer to him directly. The same thing here: one method shouldn't pass data to another by putting data into a member variable for another to find: it should instead pass the data as method parameters.

You've done a little bit of this, but it looks like you're being tentative about it. The method gCt (awful name, by the way; can't even guess what it means. Don't be afraid of long names if they're easier to understand!) The method gCt accepts two parameters named firstInt and secondInt, but it also refers to members num1 and num2. The method Enter passes the two parameters in, but it also leaves copies in num1 and num2. So gCt is using two different copies of those same two numbers -- which is confusing to read and error prone. The fix is to eliminate the member variables num1 and num2 altogether. First, move the declarations from the class scope into the method Enter. Now gCt won't compile anymore, so you'll need to change num1 into firstInt and num2 into secondInt in that method. Now those two methods communicate directly, just like handing a friend a beer.

But wait -- there's more! What about the "rest" and "factor"? These are both declared at class scope, and yet they're not used by any other method. Close inspection shows that they're not communicated between recursive calls -- i.e., one recursive call won't read values set by another recursive call. Therefore, they can both become local variables inside gCt. You can move menuPoint also using similar logic.

Yet another point: gCt returns a value, but is that value ever used? Nope. There are two possibilities, then: gCt could be changed to just return void, or gCt could be changed to return a value but not print it; the caller should print the return value instead. The second option is much better: ideally a method should just do one thing, and computing a value and displaying it counts as two.

I hope this helped.

[Wolfgang Obi]

Wow Ernest....it all looks so simple after you've explained it!
thank you so much....
everything worked so far except.....
how do i make the caller print the value, since "gGt" it has the option of returning the value "factor" from three different parts of the method (i.e. after the two "if" statements and at the end of the method) ?

i've made it return void for now (but I'd like to take your advice concerning the "one method - one function" philosophy.

(meanwile the points concerning the nomenclature have been gratefully noted - thanks a million!)

Yet another point: gCt returns a value, but is that value ever used? Nope. There are two possibilities, then: gCt could be changed to just return void, or gCt could be changed to return a value but not print it; the caller should print the return value instead. The second option is much better: ideally a method should just do one thing, and computing a value and displaying it counts as two.