Requesting a Code Review

[Jordan D. Williams]

Hello everyone!

I recently started learning Java and I am proud to say that I have completed my second program. The first one being "Hello World!".

This is a simple program that can be used to solve simple one variable equations, including quadratic equations. I know there is no error catching (at all) at the moment and the coefficients can only be integers right now. I also know that there are always a few ways to accomplish the same task in Java so I wanted to ask my code was a "good" way of solving the problem. Basically I am asking the people with more experience here to look over my code and let me know if there are ways that I can make the code more efficient. Are there any classes that can be combined? I have classes that only contain functions only because I wanted to reference them from the main(). Of course that was before I realized that in order to reference functions in classes I needed to instantiate the object first. And since instantiation takes CPU cycles, that is probably not the best way to do this.

Basically any comments at all would be greatly appreciated! Thanks!!!

::: FILE LIST :::

EquationSelector.java
EquationSolver.java (main method is here)
GetCoeffs.java
KeyboardInput.java
QuadraticEquation.java
SimpleEquation.java


::: SOURCE CODE :::

EquationSelector.java :

package com.nerdifier.experiments;

import static java.lang.System.out;

public class EquationSelector {

	int equationType;
	
	KeyboardInput input = new KeyboardInput();
	
	public int equationType() {
		do {
			out.print("Enter a number: ");
			equationType = input.keyboardInput();
			if (equationType < 1 || equationType > 2) {
				out.println("You have entered an invalid number. \r" +
					"Please enter a number between 1 and 2.");
			} else {
				out.println("Thank you!");
			}
		} while (equationType < 1 || equationType > 2);
		return (int) equationType;
	}
}

EquationSolver.java :

package com.nerdifier.experiments;

import static java.lang.System.*;

public class EquationSolver {
	
	public static void main(String[] args) {
		
		int equationType;
		int[] coeffs;
		float solution;
		float[] solutions;
		
		String description;
		description = ("This program solves simple and quadratic " +
						"equations of the type: ax + b = c and " +
						"ax^2 + bx + c = d \r");
		out.println(description);
		out.println("Please choose which equation which you would like to solve: \r");
		out.println("1. ax + b = c");
		out.println("2. ax^2 + xb + c = d \r");
		
		EquationSelector equationSelector = new EquationSelector();
		equationType = equationSelector.equationType();
		
		GetCoeffs getCoeffs = new GetCoeffs();
		coeffs = getCoeffs.getCoeffs(equationType);
		
		switch (equationType) {
		case 1:
			SimpleEquation simpleEquation = new SimpleEquation();
			solution = simpleEquation.simpleEquationSolver(coeffs);
			out.println("The solution for X is: " + solution);
			break;
		case 2:
			QuadraticEquation quadraticEquation = new QuadraticEquation();
			solutions = quadraticEquation.quadraticEquationSolver(coeffs);
			out.println("The solution for X1 is:" + solutions[0]);
			out.println("The solution for X2 is:" + solutions[1] + "\r");
			break;
		}
		
		out.println("Thank you for using EquationSolver!");
		out.println("Good bye!");
		
	}
	
}

GetCoeffs.java :

package com.nerdifier.experiments;

import static java.lang.System.out;

public class GetCoeffs {
	public int[] getCoeffs(int equation) {
		int coeffA;
		int coeffB;
		int coeffC;
		int coeffD;
		
		KeyboardInput input = new KeyboardInput();
		//EquationSelector equationSelector = new EquationSelector();
		
		out.println("Please enter a value for a:");
		out.print("a = ");
		coeffA = input.keyboardInput();
		out.println("Please enter a value for b:");
		out.print("b = ");
		coeffB = input.keyboardInput();
		out.println("Please enter a value for c:");
		out.print("c = ");
		coeffC = input.keyboardInput();
		
		if (equation == 2) {
			out.println("Please enter a value for d:");
			out.print("d = ");
			coeffD = input.keyboardInput();
			int[] coeffs = {coeffA, coeffB, coeffC, coeffD};
			return coeffs;
		} else {
			int[] coeffs = {coeffA, coeffB, coeffC};
			return coeffs;
		}
	}
}

KeyboardInput.java :

package com.nerdifier.experiments;

import java.util.Scanner;

public class KeyboardInput {
	public int keyboardInput() {
		Scanner keyboard = new Scanner(System.in);
		int k = keyboard.nextInt();
		return (int) k;
		}
}

QuadraticEquation.java :

package com.nerdifier.experiments;

public class QuadraticEquation {
	
	public float[] quadraticEquationSolver(int[] coeffs) {
		int a = coeffs[0];
		int b = coeffs[1];
		int c = coeffs[2];
		int d = coeffs[3];
		float x1; // x1 is the first "root (solution) to the quadratic equation
		float x2; // x2 is the second "root (solution) to the quadratic equation
		float squareRoot;
		
		// ensures that the right hand side of the equation is zero
		c = c - (d);
		
		squareRoot = (int) Math.pow(b, 2) - 4*(a*c);
		x1 = (-1*(b) + (float) Math.sqrt(squareRoot))/(2*(a));
		x2 = (-1*(b) - (float) Math.sqrt(squareRoot))/(2*(a));
		float[] x = {x1, x2};
		return x;
	}
}

SimpleEquation.java :

package com.nerdifier.experiments;

public class SimpleEquation {
	
	public float simpleEquationSolver(int[] coeffs) {
		int a = coeffs[0];
		int b = coeffs[1];
		int c = coeffs[2];
		float x; // x is the variable in the ax + b = c equation
		x = (c - (b)) / a;
		return (float) x;
	}
}

[Campbell Ritchie]

You should know better than to ask for code review when I am awake. Mwaahaahaahaah!
Actually there is a lot of good code there. These changes could make it even better:

1: Don’t use tabs for indentation; use spaces. Reasoning here.

2: Far too much code in the main method. Most of that code ought to be moved into the rest of the class, and it looks as if it can be divided into about 3 separate methods, if not more. Lots of small methods = good, one big method = bad.

3: I like the use of the static imports.

4: No constructors for any of the classes. I agree with what they say here that you ought to write a constructor in every class.

5: The KeyboardInput class looks to me like a utility class. It should have all static members, a private constructor, and the Scanner object a private static field. If you search this forum you will find suggestions I have written earlier about utility classes.

6: Never use float arithmetic unless you have a very good reason. Use doubles throughout.

7: Your method to solve quadratic equations lloks as if it could be a static method in a utility class, too. Or you would pass the cooefficients to the constructor and use them as fields in an object.

8: That method will return NaN if there is no real solution. It is worth learning the strange behaviours of NaN.

[Campbell Ritchie]

9: I think you have an unnecessary (int) cast somewhere. Please check.

[dennis deems]

Campbell Ritchie wrote:I like the use of the static imports.

I do not. It is jarring to see out.println when you are used to see System.out.println. System is a member of the package java.lang so there is no need for an import statement at all.

[Jordan D. Williams]

Campbell Ritchie wrote:You should know better than to ask for code review when I am awake. Mwaahaahaahaah!
Actually there is a lot of good code there. These changes could make it even better:

1: Don’t use tabs for indentation; use spaces. Reasoning here.

2: Far too much code in the main method. Most of that code ought to be moved into the rest of the class, and it looks as if it can be divided into about 3 separate methods, if not more. Lots of small methods = good, one big method = bad.

3: I like the use of the static imports.

4: No constructors for any of the classes. I agree with what they say here that you ought to write a constructor in every class.

5: The KeyboardInput class looks to me like a utility class. It should have all static members, a private constructor, and the Scanner object a private static field. If you search this forum you will find suggestions I have written earlier about utility classes.

6: Never use float arithmetic unless you have a very good reason. Use doubles throughout.

7: Your method to solve quadratic equations lloks as if it could be a static method in a utility class, too. Or you would pass the cooefficients to the constructor and use them as fields in an object.

8: That method will return NaN if there is no real solution. It is worth learning the strange behaviours of NaN.

Thank you kid sir! That will take a LOT to process!! I am not sure about half of what you said, but I will search the forums and try to figure it out. I will post the revised code when I am done.

The only reason why I imported java.lang.System.* was so that I don't have to type System.out.print... I realize it doesn't really save that much time/typing, but you know... I was trying to use everything I know.

One more thing... In the current book that I am reading they explain that every time you make a String Java creates a new object and you can't change the contents of that object. So every time I type System.out.println("Some string") it create a new object, right? And if that's right... Would it be better to connect some of the strings in another method with StringBuilder maybe?

Thanks for all the pointers, by the way!

[Bill Johnston]

Jordan D. Williams wrote:

Campbell Ritchie wrote:You should know better than to ask for code review when I am awake. Mwaahaahaahaah!
Actually there is a lot of good code there. These changes could make it even better:

1: Don’t use tabs for indentation; use spaces. Reasoning here.

2: Far too much code in the main method. Most of that code ought to be moved into the rest of the class, and it looks as if it can be divided into about 3 separate methods, if not more. Lots of small methods = good, one big method = bad.

3: I like the use of the static imports.

4: No constructors for any of the classes. I agree with what they say here that you ought to write a constructor in every class.

5: The KeyboardInput class looks to me like a utility class. It should have all static members, a private constructor, and the Scanner object a private static field. If you search this forum you will find suggestions I have written earlier about utility classes.

6: Never use float arithmetic unless you have a very good reason. Use doubles throughout.

7: Your method to solve quadratic equations lloks as if it could be a static method in a utility class, too. Or you would pass the cooefficients to the constructor and use them as fields in an object.

8: That method will return NaN if there is no real solution. It is worth learning the strange behaviours of NaN.

Thank you kid sir! That will take a LOT to process!! I am not sure about half of what you said, but I will search the forums and try to figure it out. I will post the revised code when I am done.

The only reason why I imported java.lang.System.* was so that I don't have to type System.out.print... I realize it doesn't really save that much time/typing, but you know... I was trying to use everything I know.

One more thing... In the current book that I am reading they explain that every time you make a String Java creates a new object and you can't change the contents of that object. So every time I type System.out.println("Some string") it create a new object, right? And if that's right... Would it be better to connect some of the strings in another method with StringBuilder maybe?

Thanks for all the pointers, by the way!

For a second application after "Hello World" I am impressed - good job. I too would probably prefer not to have the static import of System but I am not adamant about it.

Don't forget to to have a "default" in your switch statement; even though it looks like you'll never need it here, if you change the code in the other class that mandates 1 or 2, you will.

I also agree with the idea of breaking up the code a bit more and not having as much real logic in the main method.

As to Strings - you are wise to be onto that, but in this case you should not worry. However what I would do is make all the Strings that really are constants, constants, by adding them as final static Sting constants up front and then in your print statements just print the constant by name: example

private static final String ENTRYA = "Please enter value for a",
                            ENTRYB = "Please enter value for b"; //etc

You could put that "entry" data in a loop (probably a for loop) as well, something like:

private static final String ENTRYA = "Please enter value for ";
private static final char[] ENTRIES = new char[] {'a','b','c'};
// ... 
for (int i = 0; i < 3; i++)
  System.out.println(ENTRYA+ENTRIES[i]);

[Jordan D. Williams]

Bill Johnston wrote:
For a second application after "Hello World" I am impressed - good job. I too would probably prefer not to have the static import of System but I am not adamant about it.

Thanks! I read a LOT before I attempted this.

Realistically it's not my "second" program. It's really my first that I have written on my own. I have follow, however, many examples from different books that were programs themselves. This one the first one I wrote myself though...

I am still struggling to understand how everything fits together in OOP. When I was a kid I used to program in Pascal and that language is Linear. We never got past working with more than one file. Then I did some scripting with AutoIT3 and that is also very linear. It took me a long time to grasp the concept of having code broken into many different classes. And of course I still have a HUGE amount of learning to do. Constructors, abstract methods, working with arrays, threads, security etc., etc., etc. Not to mention that when I start thinking about access modifiers my head starts hurting, because as I understand it every class, method, constructor, and field (and probably other things) have access modifier and depending on where those elements are contained the scope of what can have access to those elements changes... Yep... The pain is coming!

Thanks also for the recommendations I see exactly why I should have a default switch. Of course I also need to put in the error handling, but I haven't read that chapter of the book yet .

Soon...

[Bill Johnston]

Well you just keep on going like you are I think you'll do fine. A couple of pointers ...

Access modifiers are important for a certainty. But I would say that until you get into synchronization and abstract classes, the main ones you'll use are coderanch, private, final and static ... and you will likely use these in much the same way from application to application, at least for a while. For example, I doubt you'll need native, if at all, for a long time. Volatile and transient likely once in a while, but probably also no for a little while. Protected, synchronized and abstract will probably be the next few you'll need to learn to use.

About OOPS. Yes, coming from a procedural language background it does take some time. You're not alone there. Keep at it and you will get it fine. Just take the book one chapter at a time and practice, practice, practice ... writing code till it coming out of your ears ;)

[Jayesh A Lalwani]

Jordan D. Williams wrote:

I am still struggling to understand how everything fits together in OOP.

Soon...

You have a good base here to take it a step further

You can define an interface called Equation with one method solve that takes coeffs and returns solutions. You can have 2 classes that implement that interface:- SimpleEquation, QuadraticEquation. In EquationSolver, you can have an array of type Equation of size 2,. The first one should be of type SimpleEquation, second one QuadraticEquation. Then change your main method to code to the Equation interface. Your code in the main method will reduce quite a bit.

Then you can add CubicEquation, QuarticEquation, etc.

[James Boswell]

I would implicitly declare the four parameters a, b, c and d for the method quadraticEquationSolver Makes the interface clearer IMO. Also, return a list of floats instead of an array as the quadratic equation can produce 0, 1 or 2 results.

Your actual coding style is very clear and well commented.

[Jordan D. Williams]

Hello again everyone!

I just wanted to let all the people that replied that I didn't abandon this project so you didn't waste your words. I am taking a while, because I am trying to understand how to implement all of your suggestions and I am also trying to add error handling for the input. Also maybe even implement the ability for people to type their selection as in words - instead of having to type "1" to type "one". Personally I would never type "one" when I can simply type "1", but it's good exercise .

I am encountering some stumbling block though so I have a few questions.

James Boswell wrote:I would implicitly declare the four parameters a, b, c and d for the method quadraticEquationSolver Makes the interface clearer IMO. Also, return a list of floats instead of an array as the quadratic equation can produce 0, 1 or 2 results.

Your actual coding style is very clear and well commented.

Thanks for the reminder about the number of roots. I had completely overlooked that! How do I return a list of floats? Every book that I have seen so far shows only one variable in the return statement.

Jayesh A Lalwani wrote:Then change your main method to code to the Equation interface. Your code in the main method will reduce quite a bit.

I am completely lost on this part. I did code an interface that is implemented in SimpleEquation and Quadratic, but I am not sure what you mean by that last part. I also am not sure that my interface is coded properly since as I understand it, it doesn't really do anything for my program. I'll post everything soon, so you'll be able to see what I mean.

Bill Johnston wrote: ... Don't forget to to have a "default" in your switch statement; even though it looks like you'll never need it here, if you change the code in the other class that mandates 1 or 2, you will...

Thanks for this. I have added a "default" case.

Bill Johnston wrote: ... You could put that "entry" data in a loop (probably a for loop) as well, something like:

private static final String ENTRYA = "Please enter value for ";  
private static final char[] ENTRIES = new char[] {'a','b','c'};  
// ...   
for (int i = 0; i < 3; i++)  
  System.out.println(ENTRYA+ENTRIES[i]); 

I really like this idea. It's OK to write it for 2-3 values manually, but if the input was from a source and had to be done 100s of times, it would be crazy to do it without a loop. Great practice! Thanks!

Campbell Ritchie wrote:
You should know better than to ask for code review when I am awake. Mwaahaahaahaah!
Actually there is a lot of good code there. These changes could make it even better:
1: Don’t use tabs for indentation; use spaces. Reasoning here.
2: Far too much code in the main method. Most of that code ought to be moved into the rest of the class, and it looks as if it can be divided into about 3 separate methods, if not more. Lots of small methods = good, one big method = bad.
3: I like the use of the static imports.
4: No constructors for any of the classes. I agree with what they say here that you ought to write a constructor in every class.
5: The KeyboardInput class looks to me like a utility class. It should have all static members, a private constructor, and the Scanner object a private static field. If you search this forum you will find suggestions I have written earlier about utility classes.
6: Never use float arithmetic unless you have a very good reason. Use doubles throughout.
7: Your method to solve quadratic equations lloks as if it could be a static method in a utility class, too. Or you would pass the cooefficients to the constructor and use them as fields in an object.
8: That method will return NaN if there is no real solution. It is worth learning the strange behaviours of NaN.

In order:
1. Got it. Very good reasoning! I switched the setting in eclipse to add spaces when I hit Tab.
2. I see. I did that and I learned that in order to call those methods from within the main method I need to have those other methods as "static". Could you elaborate on this "static" state? It's one of the areas that is kinda hazy for me.
3. Some other people said they don't. Their reasoning behind it is that they are so use to seeing System.out.print... If this just a preference or there is a reason why static imports are good?
4. I have only really heard of constructors. I have some material on it and I will be reading up on it. I don't know if I'll get it in the next build, but definitely in the one after.
5. I tried searching for posts mentioning "utility classes" but I didn't really come up with anything that I was able to understand. I searched the whole forum though. Did you mean just in "Beginning Java"?
6. Switched all floats to doubles. What would be an example of a "good reason to use floats" instead of doubles?
7. Unfortunately I am completely lost on this one...
8. I added logic to check if the discriminant is less than 0. If it is, then the equation has no solutions. On the other hand, I don't know what NaN. I haven't really had time to look it up, but I will - before I ask you to spend any more time on explaining it to me.

Thanks again everyone for your input. I should be posting the reworked version of my code either tonight or tomorrow night. Again I just wanted to let everyone know that you didn't waste your time. My little project is still alive .

[Winston Gutkowski]

Dennis Deems wrote:

Campbell Ritchie wrote:I like the use of the static imports.

I do not. It is jarring to see out.println when you are used to see System.out.println. System is a member of the package java.lang so there is no need for an import statement at all.

Sorry Campbell. I'm with Dennis here. An unnecessary addition to the language in my view that promotes otherwise unportable code - and what was the whole premise of Java? If you're having problems with import statements, look to your naming standards.

Winston

[Jordan D. Williams]

Hello again Campbell Ritchie:

I just found this post that highlights a utility class that you were talking about. (I wasn't sure if I should copy and paste the code here. If I shouldn't have posted it and just linked to it, please let me know. Thanks).

/** 
 * Includes methods which are useful whenever you are programming, eg for reading from 
 * the keyboard with a Scanner. 
 */  
public final class MyUtils  
{  
/** 
 * Private constructor to prevent anybody creating an instance of this class 
 */  
   private MyUtils()  
   {  
       throw new UnsupportedOperationException("Don't try to instantiate MyUtils class");  
   }  
  
/** 
 * A Scanner object which reads from the standard input stream. 
 */  
   private static final java.util.Scanner inScan = new java.util.Scanner(System.in);  
/* Note use of fully-qualified class name, so no "import" is required */  
  
   /** 
    * Reads the next available <tt>int</tt> from the standard input stream: usually the 
    * command line or terminal unless System.in has been diverted or closed 
    *@param message A message of the form "Please enter a number" to be displayed before 
    * input 
    *@param errorMessage A message of the form "That wasn't a real number: try again" to 
    * be displayed if the input is incorrect 
    *@return The next <tt>int</tt> entered 
    */  
   public static int readIntFromIn(String message, String errorMessage)  
   {  
      System.out.printf("%s " message);  
      while (!inScan.hasNextInt())  
      {  
          System.out.printf("%s ", errorMessage);  
          inScan.next();  
      }  
      return inScan.nextInt();  
   }  
/* 
 * You can create yourself a utilities class like this; there are other things you can read from 
 * the keyboard with a Scanner, which you can look up in the Scanner documentation. When 
 * you think of other things, you can add them to this class, too. 
 */  
}  

Could you please let me know how could I use this? For example, since I can't create an instance of it, how do I set what the "message" or the errorMessage" is supposed to be? Also how do I get the return value? I feel like I am missing something very basic here...

Thanks.

[Campbell Ritchie]

Who wrote rubbish like that old post? I have not the faintest idea how to call that method, unless you try this sort of thing:

int i = MyUtils.readIntFromIn(
            "Please enter the number:",
            "What sort of wally are you, thinking that is a number?"
                 + " Try again:");

[Jordan D. Williams]

Hello everyone! Here is the revised code. Let me know what you think. I believe it is definitely more complex than the first version, but I am afraid it might be, at time, unnecessarily complex. I also have the slight feeling that it is unorganized. I am not really sure how to explain this, but I have a feeling that the logic that logically should be in the same place, I have put in different places. Example: My Utils class checks if the input is a number, but it is in the EquationSoler.equationSelecto() that I check if the number is within the appropriate range. Is that truly disorganized, or it's normal and it just seems unorganized to me because I am still thinking in terms of procedural languages?

Here is the code:

::: FILE LIST :::
Equation.java
EquationSolver.java
QuadraticEquation.java
SimpleEquation.java
Utils.java - This class is not my doing. I ripped off Campbell Ritchie and changed the name of the class and methods.

Equation.java - just an interface which is implemented by SimpleEquation.java and QuadraticEquation.java and hopefully other equation types once I code them later

package com.nerdifier.experiments;

public interface Equation {

    double[] solve(int[] coeffs);

}

EquationSolver.java - I tried to implement most of the suggestions for this class. I don't know if what I did made it better or worse, but that's why I am posting here , I took most of the logic out of the main method. Created a bunch of other methods that I hope are "smaller" than what I had before? I am now using a loop for coefficient input to make it easier to extend functionality with adding more equation types and needing more coefficients. The range of accepted numbers is not automatically read from the length of the array containing the EQUATION_FORMAT array that holds all the standard forms of the equations that the program can solve. The input is checked if is a number and if it isn't then the user is asked to "try again". Switched most text output from System.out.prinln("...") to System.out.printf("...") because I like writing %n to get a new line instead of System.out.println(). Is one preferable over the other?

package com.nerdifier.experiments;

public class EquationSolver {

    static final String DESCRIPTION = "This program solves simple and quadratic "
            + "equations of the type: ax + b = c and "
            + "ax^2 + bx + c = d.";
    static final String[] EQUATION_FORMAT = {"", //The selection starts from 1 
            "ax + b = c", // 1 = SimpleEquation
            "ax^2 + bx + c = d"}; // 2 = Quadratic Equation
    
    
    public static void main(String[] args) {

        double[] solution;
        double[] solutions;
        
        descriptionDisplay();    
        int equationSelector = equationSelector();
        int[] coeffs_returned = getCoeffs(equationSelector);

        switch (equationSelector) {
        case 1:
            SimpleEquation simpleEquation = new SimpleEquation();
            solution = simpleEquation.solve(coeffs_returned); 
            System.out.printf("The solution for X is: " + solution[0] + "%n%n");
            break;
        case 2:
            QuadraticEquation quadraticEquation = new QuadraticEquation();
            solutions = quadraticEquation.solve(coeffs_returned);
            if (QuadraticEquation.discriminant < 0) {
                break;
            } else{
                System.out.printf("The solution for X1 is: " 
                        + solutions[0] + "%n");
                System.out.printf("The solution for X2 is: " 
                        + solutions[1] + "%n%n");
                break;
            }
         default:
                 System.out.println("An error occured. Pleas try again.");
        }
        
        closing();
    }
    
    public static void descriptionDisplay() {
        System.out.printf(DESCRIPTION + "%n%n");
        System.out.printf("Please choose which equation which you " +
        		"would like to solve: %n%n");
        // Takes the value for the equation type 
        for (int i = 1; i < 3; i++) {
            System.out.printf(i + ".%s%n", EQUATION_FORMAT[i]);
        }
        System.out.println(); 
    }

    public static int equationSelector() {
        int equationType;
        do {
            equationType = Utils.readIntFromIn("Enter a number: ", "This is " +
            		"not a number. Please try again: ");
            System.out.println();
            if (equationType < 1 || equationType > 2) {
                System.out.printf("You have entered an invalid number. %n"
                        + "Please enter a number between 1 and " +
                        (EQUATION_FORMAT.length-1) + ".%n%n");
            } else {
                System.out.printf("Thank you! %n%n");
            }
        } while (equationType < 1 || equationType > 2);
        return (int) equationType;
    }
    
    public static int[] getCoeffs(int equation) {
        // How can I enter the array without initializing it and still use the
        // for loop later and assigning  a value to the 3 index? If I later 
        // add more equation types I will have to rewrite this array every time
        // I add one.
        int[] coeffs = {0, 0, 0, 0};

        final String VALUE_REQUEST = "Please enter a value for "; 
        final String INPUT_NOT_A_NUMBER = "That is not a number. " +
        		"Please try again: ";
        final char[] VALUES = { 'a', 'b', 'c'};

        for (int i = 0; i < 3; i++) {
            int entry = Utils.readIntFromIn( VALUE_REQUEST + VALUES[i] + ": ",
                    INPUT_NOT_A_NUMBER);
            coeffs[i] = entry;
        }
        
        if (equation == 2) {
            coeffs[3] = Utils.readIntFromIn(VALUE_REQUEST + "d: ",
                    INPUT_NOT_A_NUMBER);
            System.out.println();
            return coeffs;
        } else {
            System.out.println();
            return coeffs;
        }
    }
    
    public static void closing() {
        System.out.printf("Thank you for using EquationSolver!" + "%n");
        System.out.printf("Good bye!");
    }
}

QuadraticEquation.java - just implemented the Equation.java interface and changed the names of a few variables

package com.nerdifier.experiments;

public class QuadraticEquation {

    static double discriminant;
    
    public QuadraticEquation() {
    }
    
    public double[] solve(int[] coeffs) {
        int a = coeffs[0];
        int b = coeffs[1];
        int c = coeffs[2];
        int d = coeffs[3];
        double x1 = 0; // x1 is the first "root (solution) to the quadratic equation
        double x2 = 0; // x2 is the second "root (solution) to the quadratic equation

        // Ensures that the right hand side of the equation is zero
        c = c - (d);

        discriminant = (int) Math.pow(b, 2) - 4 * (a * c);
        // Checks to make sure there are real solutions 
        if (discriminant < 0) {
            System.out.println("There are no real solutions.");
            System.out.println();
        } else {
            x1 = (-1 * (b) + Math.sqrt(discriminant)) / (2 * (a));
            x2 = (-1 * (b) - Math.sqrt(discriminant)) / (2 * (a));
        }
        if (x1 < x2) {
            double[] solutionsForX = { x1, x2 };
            return solutionsForX;
        } else {
            double[] solutionsForX = { x2, x1 };
            return solutionsForX;
        }
    }
}
/* Any other way of doing this that I will not have to have a return statement 
*  twice? I tried putting the return statement outside of the if statement
*  but eclipse complains saying "solutionsForX cannot be resolved to a variable"
*/

SimpleEquation.java - just implemented the Equation.java interface and changed the names of a few variables

package com.nerdifier.experiments;

public class SimpleEquation implements Equation {

    public SimpleEquation() {
    }
    
    public double[] solve (int[] coeffs) {
        int a = coeffs[0];
        int b = coeffs[1];
        int c = coeffs[2];
        double x; // x is the variable in the ax + b = c equation
        x = (c - (b)) / a;
        double[] solution = {x, 0}; // an array to write the solution
        return solution;
    }
}

Utils.java - again this is not mine. I got it from Campbel Ritchie - thank you! It actually combines the input and error checking in one class. That's awesome!

package com.nerdifier.experiments;

/**
 * Includes methods which are useful whenever you are programming, eg for
 * reading from the keyboard with a Scanner.
 */
public final class Utils {
    /**
     * Private constructor to prevent anybody creating an instance of this class
     */
    private Utils() {
        throw new UnsupportedOperationException(
                "Don't try to instantiate MyUtils class");
    }

    /**
     * A Scanner object which reads from the standard input stream.
     */
    private static final java.util.Scanner inScan = new java.util.Scanner(
            System.in);

    /* Note use of fully-qualified class name, so no "import" is required */

    /**
     * Reads the next available <tt>int</tt> from the standard input stream:
     * usually the command line or terminal unless System.in has been diverted
     * or closed
     * 
     * @param message
     *            A message of the form "Please enter a number" to be displayed
     *            before input
     * @param errorMessage
     *            A message of the form "That wasn't a real number: try again"
     *            to be displayed if the input is incorrect
     * @return The next <tt>int</tt> entered
     */
    
    public static int readIntFromIn(String message, String errorMessage) {
        System.out.printf("%s ", message);
        while (!inScan.hasNextInt()) {
            System.out.printf("%s ", errorMessage);
            inScan.next();
        }
        return inScan.nextInt();
    }
}

Thanks again for all your time people!! I can't tell you how much you are helping me learn!

[Winston Gutkowski]

Jordan D. Williams wrote:Is that truly disorganized, or it's normal and it just seems unorganized to me because I am still thinking in terms of procedural languages?

Difficult to say, but you often end up with more classes/modules/methods in OO than you would with procedural programs, which tend, by their nature, to be monolithic.

However, just looking at your QuadraticEquation.solve(), a few tips:

Why not have a, b, c and d as parameters? You don't appear to do anything with your coeffs array except set it to internal fields with those names anyway, and giving them explicit names will help you to document them (I'm still not quite sure what d is supposed to be).

-1 * (b) == -b.

Mat.pow(b, 2) == b * b (and the latter is probably a lot quicker).

I'd also suggest that you might be better off using doubles instead of ints. That way you only have to round if you want to.

HIH

Winston

[Jordan D. Williams]

Hey Winston.

Thanks for the tips.

"d" is on the right hand side of the equation. I put it there in case the user has an equation that has something other than zero on the right hand side. That way they don't have to subtract it from "c".

Winston Gutkowski wrote:

-1 * (b) == -b.

Wouldn't that evaluate to -b if b is negative? Example:

int b = -1;
int oppositeB = -b; 
SYstem.out.print(b);

Now when I think about it, it makes sense. Of course -1 * (b) = -b. Very often when I try to explain a math problem to a student (I am a private tutor on my free time) they want to just take the number without the sign. So I have to explain to them that the sign before the number is part of the number. I remember reading a while back how Java stores numbers and that the sign of a number is in fact part of the number. So when I make b=-b I will always get the "opposite" of b.

Thanks for pointing that out. It's a simple, but very useful thing. I probably should have thought of, but oh well .

Winston Gutkowski wrote:Mat.pow(b, 2) == b * b (and the latter is probably a lot quicker).

I was so proud of myself for finding the power function that I couldn't resist using it. You are absolutely right though. I'll change it in the next version.

Winston Gutkowski wrote:Why not have a, b, c and d as parameters? You don't appear to do anything with your coeffs array except set it to internal fields with those names anyway, and giving them explicit names will help you to document them (I'm still not quite sure what d is supposed to be).

Sounds good. I will look into that. I think I went with taking an array as an argument because I collect the input with a for loop. Besides, QuadraticEquation.solve() implements the Equation interface and SimpleEquation.solve() also implements the Equation interface. Would I be able to take the coeffs as parameters and still implement the method so it can be consistent when I add more classes that solve other equation types?

Thanks.

[Campbell Ritchie]

Jordan D. Williams wrote: . . . This class is not my doing. I ripped off Campbell Ritchie . . .

Never believe anything he tells you. Any code he wrote will be total rubbish.

You are right to acknowledge things like that. If you hand that code in for marking, you must acknowledge its source, even though there is nothing novel about the code. [That method is a standard idiom for using Scanners.] If you don’t acknowledge it, you might get in trouble for plagiarism.

[Winston Gutkowski]

Jordan D. Williams wrote:Besides, QuadraticEquation.solve() implements the Equation interface and SimpleEquation.solve() also implements the Equation interface.

Ah, sorry. Didn't see that change.

Would I be able to take the coeffs as parameters and still implement the method so it can be consistent when I add more classes that solve other equation types?

No, not really. However, what you could do is to define solve() in Equation as
public abstract double solve(double... coeffs); // note '...' instead of '[]'
That would allow subclasses to take however many arguments they need (and don't forget to document it ).
(You could use int, but I think you'll find double more flexible)

Varargs allow parameters to be passed in as either arrays or values, viz:
myQuadratic.solve(2, 3, 4);

Another option might be to allow clients to set arguments themselves, perhaps via the constructor, and then run a solve() method with no parameters.

Winston

[Jordan D. Williams]

Hello everyone!

I have been working hard on rewriting the first version of my EquationSolver experiment and with the help of many from the community today I am posting the second version - Equation Solver 2.0

You can find all the code here - in an SVN Repository.

I am not posting the code directly here because there are too many files - 9 classes and 3 properties files.

Please let me know what you think and how it can be improved. I am really more focused on what you think about the design and the overall execution of everything. If you have suggestions for features that I can implement, that would be cool too.

Thanks so much for your help!

[Junilu Lacar]

Overall, it looks like a pretty good job. I might have a few nits to pick but nothing big really.

Directory names are usually all lower case, e.g. resources and utils instead of Resources and Utils. This makes it easier to distinguish them from source files, which are capitalized

Would be nice to see some unit tests, but you'll probably start getting into that later. The earlier you get into the habit of unit testing as you write code, the better. The jury is in on Test-Driven Development (TDD) and the verdict is "It's good."

There are quite a few classes with static only methods. There didn't seem to be anything obviously undesirable about them being that way. It may be just my style but I tend to get fidgety when I see only static methods in a class, a private constructor and no factory methods ... reminds me too much of procedural code.

Comments are good but sometimes they can get in the way of someone trying to read the code. I have more nits about the JavaDoc style below

Your Equation implementations are unnecessarily coupled to the GetCoeffs class. Why would an equation care how coefficients are obtained? See discussion below about inappropriate coupling

Class names should be nouns or noun phrases. GetCoeff is a verb phrase. Refactor this. You may even be able to eliminate this class altogether. See TMI discussion below

Inappropriate Coupling - TMI (too much intimacy)
When a class knows too much about other class(es), it creates unnecessary coupling. Why would an Equation care about how its coefficients are obtained? Your implementations of Equation know too much about this detail -- they are coupled too tightly with GetCoeff. This is where my niggle about private constructors without factory methods comes into play. The thing that clued me in to this smell was that the method name getValues in the Equation interface did not logically match the method's return type. It felt to me like the method wanted to be named something like buildEquation or createEquation with a variable length list of coefficients appropriate for the type of equation passed into it.

Here's the refactoring I would perform:

// I try to start with package-private scope, then increase
// if I find that it actually needs to be public
interface Equation {
    // add new methods, with JavaDoc to explain usage
    // Note: declaring as public is redundant, it's already implied for interface methods
    // Note: notice that I spell out the parameter name. I hate abbv'ns, don't you?

    /**
     * Returns the default values of all coefficients needed by this Equation. The 
     * elements in the returned array can be replaced with new values before the
     * array is passed back to the solve method. The array is guaranteed to have
     * all the necessary values required to solve this Equation.
     *
     * @return an array of default values for all coefficients needed to solve this Equation.
     */
    double[] getCoefficients();

    /**
     * Solves this Equation using the given values for coefficients
     */
    double[] solve(double[] coefficients);

    // later, delete these when coupling has been loosened by refactoring
    Equation getValues();
    double[] solve();
    ...
}

// *** Refactor: eliminate calls to methods with TMI
// assuming there's a declaration like this: Equation eq;
// eq.getValues();  <--- anything like this is replaced by code below

double[] coefficients = eq.getCoefficients();

// maybe you don't even need a GetCoeff class...?
showDefaultsAndGetNewValues(coefficients);  // implement this 

double results = eq.solve(coefficients);

Nits about your JavaDocs

For your reference, here's the JavaDocs style guide: http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

Avoid starting your comments with "This class" or "This method". That's pretty much redundant. Try to start with a verb. For classes, I usually start with "Represents whatever concept..." For interfaces, I usually start with "Implementations of <interface name> are expected to <describe the general contract for behavior>" or "Defines the behavior of a/an implementation of <interface name>." I usually prefer the former.

Avoid restating the implementation details. Those should be apparent when you read the code itself. Describe the intent, not how it is achieved. Case in point: The JavaDoc comment for EquationSelector.selectEquation() just rehashes what should be evident to the reader when they read the code. When I see JavaDocs like this, I smell a need to clarify the code's intent. Compose method refactoring is my goto technique to clarify. The code in EquationSolver.main is nice and composed, for example. Notice how you didn't have to put JavaDocs? JavaDocs is for people who want to use the class/method but not get intimate with the implementation details.

[Junilu Lacar]

One more nit I just found:

Look at the EquationSelector class. What happens to it if you add new types of equations? Say you wanted to add Exponential and Cubic equations to your portfolio of equations. This is going to require you to change EquationSelector, right? Is that change really necessary? Should EquationSelection have to change if you add or remove Equation types? Probably not. I smell a need to refactor. But I would wait until I actually had to add a new Equation type. I would have tests that provide a safety net to facilitate refactoring. And I would do TDD on this.

Here's how it would go down:

1. Someone (maybe myself) asks me to add Exponential and
Cubic equations to the list of supported equations
2. I develop Exponential equation and its unit tests via TDD
3. I integrate Exponential equation and test the program end to end.
4. I find that I need to change EquationSelector to get it to give Exponential
equation as a choice.
5. I refactor so that I won't have to change EquationSelector when I add
(or remove) new Equation types.
6. I develop Cubic equation and its unit tests via TDD
7. I integrate Cubic equation and test the program end to end.
8. I'm done.

[Jordan D. Williams]

Junilu Lacar wrote:One more nit I just found:

Look at the EquationSelector class. What happens to it if you add new types of equations? Say you wanted to add Exponential and Cubic equations to your portfolio of equations. This is going to require you to change EquationSelector, right? Is that change really necessary? Should EquationSelection have to change if you add or remove Equation types? Probably not. I smell a need to refactor. But I would wait until I actually had to add a new Equation type. I would have tests that provide a safety net to facilitate refactoring. And I would do TDD on this.

Here's how it would go down:

1. Someone (maybe myself) asks me to add Exponential and
Cubic equations to the list of supported equations
2. I develop Exponential equation and its unit tests via TDD
3. I integrate Exponential equation and test the program end to end.
4. I find that I need to change EquationSelector to get it to give Exponential
equation as a choice.
5. I refactor so that I won't have to change EquationSelector when I add
(or remove) new Equation types.
6. I develop Cubic equation and its unit tests via TDD
7. I integrate Cubic equation and test the program end to end.
8. I'm done.

First of all, I would like to thank you for taking the time to write such an in-depth review of my code. I know that the fastest way to get better at something is to listen to the people who have "been there, done that"! Learning from the experience of others is much less painful than learning through your own experience...

I have a few questions on your comments. Most of them I understood, some of them I am a little confused about, and some of them I flat out have no idea what you are talking about - but that's OK because I want to learn . I would go over the things I understand and almost understand first, and then move on to the things that I have no idea what you mean.

Frankly, I should have known about the lower case names for the directory names. I actually had them lowercase in the previous version. I will change that for the next version. Quick question though... What do I do when I have a directory name that is composed of more than one words? Ex: Equation Solver... What is the naming convention? equationsolver? equationSolver? Or just flat out "don't use more than one word" for the directory?

Your comments on the class names are another thing that I should have known. As soon as I read that I remembered reading it in the Java Tutorials and a couple of other places. Thanks for the reminder.

Spelling out the full name of the members is also something that I have read about, but I failed to implement. If someone else was reading my code, it might be difficult to understand what is "coeff" supposed to mean.

In regards to the JavaDoc, it makes complete sense what you are saying. I will try to do what you suggest. Mostly the comments that I put in are for myself. Because I am so new to Java, most of these comments are for myself to remind me what the code actually does. I just don't know Java well enough yet to be able to look at a piece of code and understand what it does - especially with some of the more complex code (which might not be that complex at all, but it is for me most of the times ). Although it's probably a better idea to develop good JavaDoc writing style and if I need to leave reminders for myself I should use regular comments.

Here is my reasoning about the methods with private constructor and only containing static methods (I have only heard about factory methods, but I don't know how they work so I am leaving that out...):

The InputGetter and the GetCoeffs are "utility" classes so I didn't think that I should be letting anyone instantiate those. Now that I think about it, the EquationSelector class is also a utility class and probably should be in the Utils package. The reason, in regards to EquationSelector specifically, why I decided to declare a private constructor and static methods is because this class will always be used. I noticed that the main() method is static and it made sense to me, because it will always be run. So I figured since EquationSelector will always be run, rather than creating more logic in the main() method by having to instantiate it before I can use it, I thought I should just make it's methods static. I am not saying I don't agree with you. I don't have enough knowledge on the subject to agree or disagree. I just wanted to explain why I did what I did. Please let me know if that reasoning is not a "good enough" reason to implement it in such a way. By the way... I don't mean that sarcastically at all. If this is not good, I want to know how to do it better.

I wrote GetCoeffs in such a way so that I don't have to write a new method to get the coefficients every time I add a new equation type. If I don't have that class then I will have a lot of repeating code in the class of each equation type that would have to get the appropriate coefficients. I don't remember what the term is used when you just copy and paste code and just change the relevant parts, but when I read about it, I was told that was something I should try to avoid doing. Is there a way to keep GetCoeffs and maybe rewrite the implementation so that there isn't too much TMI while still practicing code reuse, instead of copy-paste? Please correct me if I am wrong, but from what I understand, with your suggested implementation, I would have to have a getCoefficients() method in each equation type class and gather the coefficients through that method, rather than the GetCoeffs utility class.

And now to the part which I know nothing about:

Unit testing... I have no idea how to do it? Are there any resources that can point me to? (Besides the one you showed in your post? I did not get anything on that page) That is something that is completely new to me. I mean.. I know that you "test" your code before you release the application, but as far as "how" to write and implement code that does that, I have no idea.

Thanks so much for all your time! I really appreciate it!

[Junilu Lacar]

Lots of stuff, so we'll break it down:

Jordan D. Williams wrote:from what I understand, with your suggested implementation, I would have to have a getCoefficients() method in each equation type class and gather the coefficients through that method, rather than the GetCoeffs utility class.

Yes, you would have to implement getCoefficients() in each implementation of Equation. No, you wouldn't have each implementation gather the values. That's the whole point. The Equation isn't responsible for gathering the values. Here's the gist of the "conversation" that should go on:

Cast of characters

Client - some code that wants to use an Equation
SomeEquation - any implementation of Equation
User - the user who wants to solve SomeEquation

Client: "Hey, SomeEquation, tell me this: what coefficients do you need to calculate a result?"

SomeEquation: "I need this many numbers, those values you see are just defaults..."

(SomeEquation hands back an array with as many elements as coefficients that
it needs to calculate a result)

Client: "Ok, I see you have N coefficients. Give me a second."

(Client looks at first coefficient that SomeEquation gave back)

Client: "Hey, User, SomeEquation's first coefficient has a default value of X, do you want to use some
other value instead?"

User: "Yeah, use 5."

(Client replaces first coefficient with 5)

(Client looks at second coefficient that SomeEquation gave back)

Client: "Hey, User, SomeEquation's second coefficient has a default value of Y, do you want to use
some other value instead?"

User: "Yeah, use -2."

(Client replaces second coefficient with -2)

.... and so on until Client has asked User about all the coefficients that SomeEquation needs

Client: "Hey, SomeEquation, here are those coefficients back, except I changed the values
to the ones that User gave me. What's the answer if you use those instead?"

.... (you can probably finish this story now)

-----
Notice that there was no interaction whatsoever between the Equation and the User. That's because Equation doesn't care how the values are obtained. Equation's only responsibility is to give out the array with the right number of elements and the default values that can be replaced. When it gets back the array, it just uses the values it finds in there to calculate a result.

[Junilu Lacar]

Jordan D. Williams wrote:Unit testing... I have no idea how to do it? Are there any resources that can point me to? (Besides the one you showed in your post? I did not get anything on that page) That is something that is completely new to me. I mean.. I know that you "test" your code before you release the application, but as far as "how" to write and implement code that does that, I have no idea.

Unit Testing

Since you're already doing so well, you should just start looking directly into Test-Driven Development (TDD). See my post about it here: https://coderanch.com/t/584943/java/java/days-frustration-errors-troubleshoot-any#2662869

Don't worry if it seems like a more advance topic for you. I personally think that it's the way students should be taught. It seems that teachers focus too much on the syntax and mechanics of the language with little emphasis on design, testability, and maintainability. It's like saying "I'll teach you the words and phrases, but you have to figure out how to string them up into sentences and paragraphs so you can carry on a meaningful conversation."

Going slightly off topic for a bit, but bear with me. Assume that you have first been taught how to set up a Java project, and set up and run a JUnit test, which is what AccumulatorTest is. Assume also that you have already been taught the basic operation of an assignment statement and the for-loop. Do you think it would be very difficult for a beginner to complete this exercise? What about the challenge questions? IMO, this would be a very gentle introduction to unit testing and eventually Test-Driven Development. The mechanics of the language become almost a side detail. More importantly, it uses an example to teach students how to communicate a design to other developers via a set of tests.

Exercise 1. Assigning values and using the for-loop (10 points)

Instructions: Given the JUnit test below, write the code that would make it pass. Use what you learned about assigning values and the for-loop in your solution.

package edu.myschool.cs.tdd101;

import static org.junit.Assert.*;

import org.junit.Before;
import org.junit.Test;

public class AccumulatorTest {

    private Accumulator testSubject;

    @Before
    public void setup() {
         testSubject = new Accumulator();
    }

    @Test
    public void sum_of_1_to_3_should_be_6() {
        assertEquals(6, testSubject.total(3));
    }

    @Test
    public void sum_of_1_to_10_should_be_55() {
        assertEquals(55, testSubject.total(10));
    }

    @Test
    public void sum_of_1_to_100_should_be_5050() {
        assertEquals(5050, testSubject.total(100));
    }
}

Challenge 1 - Extra credit (2 points)
Modify AccumulatorTest so that instead of using a hard-coded value for the expected result, you will use an established formula to double check the brute-force calculations made by your Accumulator.

Challenge 2 - Extra credit (4 points)
Add a test to AccumulatorTest that will require you to implement an overloaded version of total() that takes an array of integers as its parameter. The result should be the sum of all the integers in the array. Your solution should include the code in Accumulator that will make the test pass.

[Jordan D. Williams]

Hi there Junilu.

I will be posting my solution after I get some sleep... I thought I was going to do it today, but time did not permit.

Thanks for the challenge!

[Junilu Lacar]

Jordan, I gave that "exercise" simply to make a point about what I thought would be a better way to teach students how to program in a way that's more in line with the kind of tasks they should be doing in the professional world. I feel it will make the kids think in a more object-oriented way. Unlike the kind of programs I'm seeing a lot of students/beginners writing nowadays, the solution will not even require a public static void main method. In fact, if they did write a main method, I would deduct points or just hand it back to them to rework without a main method.

Here's a dirty little "secret" you might not know: Professional developers hardly ever need to write public static void main methods for code that gets deployed to production. I have written public static void main methods for deployment to production probably no more than five times in over a decade of professional development with Java. That's less than once every two years! Why then, do we teach kids, or at least allow them to keep doing over and over again as if it were the right thing to do, things that they probably won't have to do in the real world? It seems crazy to me, or irresponsible at best.

If you feel like accepting the programming challenge, I'd be happy to comment on your solution as well.

[Jordan D. Williams]

Junilu Lacar[b wrote:Exercise 1. Assigning values and using the for-loop (10 points)[/b]

Instructions: Given the JUnit test below, write the code that would make it pass. Use what you learned about assigning values and the for-loop in your solution.

package edu.myschool.cs.tdd101;

import static org.junit.Assert.*;

import org.junit.Before;
import org.junit.Test;

public class AccumulatorTest {

    private Accumulator testSubject;

    @Before
    public void setup() {
         testSubject = new Accumulator();
    }

    @Test
    public void sum_of_1_to_3_should_be_6() {
        assertEquals(6, testSubject.total(3));
    }

    @Test
    public void sum_of_1_to_10_should_be_55() {
        assertEquals(55, testSubject.total(10));
    }

    @Test
    public void sum_of_1_to_100_should_be_5050() {
        assertEquals(5050, testSubject.total(100));
    }
}

Challenge 1 - Extra credit (2 points)
Modify AccumulatorTest so that instead of using a hard-coded value for the expected result, you will use an established formula to double check the brute-force calculations made by your Accumulator.

Challenge 2 - Extra credit (4 points)
Add a test to AccumulatorTest that will require you to implement an overloaded version of total() that takes an array of integers as its parameter. The result should be the sum of all the integers in the array. Your solution should include the code in Accumulator that will make the test pass.

I have chosen to accept this challenge. Now let's see if I can handle it.

I believe this is the code that will pass the tests in Excercise 1:

I am currently at a computer that does not have JDk installed so I wasn't able to test this to see if it will even compile, but hopefully I got it right. Also, for those that are unaware, I do not know how to use JUnit or Test Driven Development (TDD), thus this being a "challenge" for me.

Excercise 1:

package edu.myschool.cs.tdd101;   

public class Accumulator {
	
	int totalSum = 0;
	
	public int total(int n) {
		for (int i = 1; i < n+1; i++) {
			totalSum += i;
		}
	}
}

Challenge 1:
My guess is that both the Accumulator() calss and the AccumulatorTest() class will need to bechanged in order to successfully complete this challenge. Here is my try...

package edu.myschool.cs.tdd101;   

public class Accumulator {
	
	private int totalSum = 0;
	private int totalSumFormula;
	
	public int total(int n) {
		for (int i = 1; i < n+1; i++) {
			totalSum += i;
		}
	}
	
	public int totalFormula(int n) {
		totalSumFormula = n(n+1)/2;
	}
	
}

package edu.myschool.cs.tdd101;   
  
import static org.junit.Assert.*;   
  
import org.junit.Before;   
import org.junit.Test;   
  
public class AccumulatorTest {   
  
    private Accumulator testSubject;   
  
    @Before  
    public void setup() {   
         testSubject = new Accumulator();   
    }   
	
	@Test
	public void sum_by_formula_of_1_to_3_should_be_6() {
		assertEquals(testSubject.totalForumal(3), testSubject.total(3));
	}
  
	@Test
	public void sum_by_formula_of_1_to_10_should_be_55() {
		assertEquals(testSubject.totalForumal(10), testSubject.total(10));
	}
  
    @Test
	public void sum_by_formula_of_1_to_100_should_be_5050() {
		assertEquals(testSubject.totalForumal(100), testSubject.total(100));
	}
	  
}  

Alternatively, it could be that I don't have to change the Accumulator() class. If that's the case then maybe the AccumulatorTest() class should look like this:

package edu.myschool.cs.tdd101;   
  
import static org.junit.Assert.*;   
  
import org.junit.Before;   
import org.junit.Test;   
  
public class AccumulatorTest {   
  
    private Accumulator testSubject;   
	private int sumByFormula;
  
	public int sumByFormula(int n) {
	sumByFormula = n(n+1)/2;
	}
    @Before  
    public void setup() {   
        testSubject = new Accumulator();
    }   
  
    @Test  
    public void sum_of_1_to_3_should_be_6() {
		sumByFormula(3);
        assertEquals(sumByFormula, testSubject.total(3));   
    }   
  
    @Test  
    public void sum_of_1_to_10_should_be_55() {
		sumByFormula(10);	
        assertEquals(sumByFormula, testSubject.total(10));   
    }   
  
    @Test  
    public void sum_of_1_to_100_should_be_5050() {   
		sumByFormula(100);
        assertEquals(sumByFormula, testSubject.total(100));   
    }   
}  

Challenge 2:

Theis is the code I believe I should have for the AccumulatorTest() class:

package edu.myschool.cs.tdd101;   
  
import static org.junit.Assert.*;   
  
import org.junit.Before;   
import org.junit.Test;   
  
public class AccumulatorTest {   
  
    private Accumulator testSubject;   
  
    @Before  
    public void setup() {   
         testSubject = new Accumulator();   
    }   
  
    @Test
    public void sum_of_1_to_3_should_be_6() {
		for (int i = 1; i < 4; i++) {
			ints[i-1];
		}
        assertEquals(6, testSubject.total(testSubject.ints));   
    }   
  
    @Test  
    public void sum_of_1_to_10_should_be_55() {   
        for (int i = 1; i < 11; i++) {
			ints[i-1];
		}
		assertEquals(55, testSubject.total(testSubject.ints));   
    }   
  
    @Test  
    public void sum_of_1_to_100_should_be_5050() {   
        for (int i = 1; i < 101; i++) {
			ints[i-1];
		}
		assertEquals(5050, testSubject.total(testSubject.ints));   
    }   
} 

And this is the code I believe I need to have in order to make the above AccumulatorTest() test pass:

package edu.myschool.cs.tdd101;   

public class Accumulator {
	
	private int totalSum = 0;
	private int[] ints;

	public int total(int n) {
		for (int i = 1; i < n+1; i++) {
			totalSum += i;
		}
	}
	
	public int total(int[] ints) {
		for (int i = 1; i < n+1; i++) {
			totalSum += ints[i-1];
		}
	}

}

Well... This is what I came up with. Hopefully I got at least part of it right. Any comments will be appreciated.

[Junilu Lacar]

For doing it without the benefit of running and seeing if it actually works, you did pretty good.

Now, let's see if my compiler skills are up to par (these comments without the benefit of actually compiling your code on a computer, just my old glob of gray matter):

Exercise 1: Won't compile. Error at line 11. Line 5 is unnecessary. Otherwise, the loop appears to be correct, although I would make the termination clause be "i <= n" instead

Challenge 1, try 1:
- Accumulator: aside from the problems carried over from before, line 15 won't compile. The formula is correct but there is a syntax error in the expression. When you correct the syntax error, you'll still get a compile error at Line 15.
- AccumulatorTest: this looks like it will compile.
This is not the solution I would be looking for though

Challenge 1, try 2:
This is the approach you should take. Given the requirement to use brute-force calculation in the solution, the test should contain the alternative calculation method.
- AccumulatorTest: Line 14 will not compile. Same problems as in try 1: syntax error on the expression and a compile error on the same line. There's no need for the sumByFormula variable on Line 11. Just call the sumByFormula method directly.
- Accumulator: correct, no need to change for this challenge

Challenge 2:
This one has the most problems. You'll need to read up on the idioms for using a for-loop. See the Java Tutorials.
AccumulatorTest - you shouldn't modify tests that are already passing. These become your safety net for future changes. If you make a change and tests start to fail, you know you've messed up something. Only new tests should fail. Old tests should still pass. You should add a new test that calls the overloaded total() method. Leave the ones already there alone.
Accumulator -
Line 5 - this variable is unnecessary at this level. Move it into the methods that need it
Line 6 - this variable is also unnecessary. It can be eliminated.
Line 9 - same comment about loop termination condition as before
Line 12 - Compile error
Line 15 - Compile error - Unknown variable 'n'. This loop can follow the standard idiom for looping through a collection or array. See the enhanced for-loop (aka for-each loop) in the Java Tutorials.
Line 18 - Compile error

Just to clarify the requirement for Challenge 2: If the Accumulator were to be passed the array {1, 7, 11, 17, 21}, the total method should return 57
Still, for doing this without a JDK to compile the code and get feedback, it's a pretty good effort.

[Jordan D. Williams]

Well I guess for not using an IDE it wasn't that bad. I just forgot to return any of the functions. Forgot to declare some of the variables, and learned that I must have the * sign to do multiplication. Otherwise the compiler thinks I a trying to call a function that does not exist.

So here is my second try. This time with Eclipse and even tested with JUnit so I know it all works . The only question now is if this is what you had in mind as a solution.

Exercise 1:

Previously, I had int totalSum = 0; outside of the total(int n) method and you said I don't need it. Is this what you meant or is there a way in which I can eliminate it entirely?

package edu.myschool.cs.tdd101;     
      
public class Accumulator {  
          
    public int total(int n) {
        int totalSum = 0;
        for (int i = 1; i <= n; i++) {  
            totalSum += i;  
        }
        return totalSum;  
    }  
}  

Challenge 1:

package edu.myschool.cs.tdd101;     
        
import static org.junit.Assert.*;     
        
import org.junit.Before;     
import org.junit.Test;     
        
public class AccumulatorTest2 {     
     
    private Accumulator testSubject;     
      
    public int sumByFormula(int n) {  
        int sumByFormula = n*(n+1)/2;
        return sumByFormula;
    }  

    @Before    
    public void setup() {     
        testSubject = new Accumulator();  
    }     
    
    @Test    
    public void sum_of_1_to_3_should_be_6() {  
        assertEquals(sumByFormula(3), testSubject.total(3));     
    }     
        
    @Test    
    public void sum_of_1_to_10_should_be_55() {  
        assertEquals(sumByFormula(10), testSubject.total(10));     
    }     
        
    @Test    
    public void sum_of_1_to_100_should_be_5050() {     
        assertEquals(sumByFormula(100), testSubject.total(100));     
    }     
}

Challenge 2:

AccumulatorChallenge2.class:

package edu.myschool.cs.tdd101;     
  
public class AccumulatorChallenge2 {  
  
    public int total(int n) {  
        int totalSum = 0;
        for (int i = 1; i < n+1; i++) {  
            totalSum += i;  
        }
        return totalSum;
    }  
    
    public int total(int[] ints) {
        int totalSum = 0;
        for (int i : ints ) {  
            totalSum += i;  
        }
        return totalSum;
    }
}

AccumulaotrChallenge2Test.class:

package edu.myschool.cs.tdd101;     
        
    import static org.junit.Assert.*;     
        
    import org.junit.Before;     
import org.junit.Test;     
        
public class AccumulatorChallenge2Test {     
     
    private AccumulatorChallenge2 testSubject;     
     
    @Before    
    public void setup() {     
         testSubject = new AccumulatorChallenge2();     
    }     
        
    @Test  
    public void sum_of_1_to_3_should_be_6() {  
        assertEquals(6, testSubject.total(3));  
    }  
  
    @Test  
    public void sum_of_1_to_10_should_be_55() {  
        assertEquals(55, testSubject.total(10));  
    }  
   
    @Test  
    public void sum_of_1_to_100_should_be_5050() {  
        assertEquals(5050, testSubject.total(100));  
    }  
        
    @Test  
    public void array_sum_of_1_4_34_and_234_should_be_273() {
        int[] ints = {1, 4, 34, 234};
        assertEquals(273, testSubject.total(ints));     
    }
    
    @Test  
    public void array_sum_of_54_7_negative_16_and_negative_18_should_be_27() {
        int[] ints = {54, 7, -16, -18};
        assertEquals(27, testSubject.total(ints));     
    }
    
}

[Junilu Lacar]

Jordan D. Williams wrote:So here is my second try. This time with Eclipse and even tested with JUnit so I know it all works . The only question now is if this is what you had in mind as a solution.

Yes, absolutely. Now I hope you have been test-driven infected. It's a great way to program. I'm confident that you're ready for the material in "Growing Object-Oriented Software, Guided by Tests" now.

Just two more itty-bitty nits (Refactoring):
- sumByFormula doesn't really need the local variable. You can reduce this method to just one line. The method is already called "sumByFormula" and that's enough to document what it's doing.
- "totalSum" is kind of a redundant name. Just "total" or "sum" will make reading the code flow a lot better.

Like I said, they're itty-bitty refactorings (eliminate the temporary variable and rename variable). And notice how clean and crisp the code is. No superfluous parts. (First rule of ZOM). It's beautiful.

You might wonder if the simplicity you see in this code is just because it's a little "exercise" program. Surely, real-world applications are much more complex, right? Yes, they are much more complex. But none of the parts of a complex application need to be much more complicated that any of the code you just wrote. If you keep things as simple as they can be at any particular level of the application, there are virtually no bounds to how complex you can grow a system. See this: http://www.noop.nl/2008/08/simple-vs-complicated-vs-complex-vs-chaotic.html for more about complex vs complicated.

Anyway, great job!

[Junilu Lacar]

One final nit, now that I think about it...

The names of the tests you added for the overloaded total method could still be improved. You picked up well on the naming convention I used but it would make the intent of the two new tests much clearer if you took away some of the details. If you replaced the specific numbers you use with more general phrases like "should subtract negative numbers" or "sum of all positive numbers", the intent of the tests become clearer. The actual numbers used in these tests are less significant in clarifying the intent. In the previous tests, the range was significant in documenting the intent of the tests, hence the actual numbers used were in the names.

The format I follow for naming tests is one recommended by Neal Ford, an author and popular speaker at developer conferences. Neal recommends making test names more readable by using underscores instead of the usual Java method name convention. His reasoning is that these are test cases so they don't have to follow the same standard. Clarity of purpose and readability is more important for tests, starting with the name. So test names should follow formats similar to these:

should_do_whatever_when_condition
does_whatever_when_condition
result_when_condition_should_be_expected

Examples:
should_return_1_when_initialized
should_return_false_when_still_processing

grounded_when_does_not_pass_inspection
sends_out_of_office_reply_when_on_vacation

state_tax_when_working_in_TEXAS_should_be_0
driving_when_under_the_influence_should_cost_maximum_points

Names like this help make the tests another type of documentation (Detailed Design document). Again, it's beautiful.

[Jordan D. Williams]

Junilu Lacar wrote:
Yes, absolutely. Now I hope you have been test-driven infected. It's a great way to program. I'm confident that you're ready for the material in "Growing Object-Oriented Software, Guided by Tests" now.

Well sir this is in a huge part thanks to you!

Thank you so much for showing me this. I actually learned a whole lot! And it was a good introduction to JUnit and unit testing in general. I obviously am just scratching the surface of learning how to design tests and the like, but I think it's a great start. I will definitely be reading that book you suggested. Before this I had to run the whole program from the beginning to test it and see if it was doing what I wanted it to do. Now, it just takes a click of a button! Can it get much better than that?

Thanks so much again! Now I will be working on refactoring the Equation Solver program to remove the TMI and hopefully implement a couple of more equation types.

[Jordan D. Williams]

Junilu Lacar wrote:
Inappropriate Coupling - TMI (too much intimacy)
When a class knows too much about other class(es), it creates unnecessary coupling. Why would an Equation care about how its coefficients are obtained? Your implementations of Equation know too much about this detail -- they are coupled too tightly with GetCoeff. This is where my niggle about private constructors without factory methods comes into play. The thing that clued me in to this smell was that the method name getValues in the Equation interface did not logically match the method's return type. It felt to me like the method wanted to be named something like buildEquation or createEquation with a variable length list of coefficients appropriate for the type of equation passed into it.

Hello there Junilu,

I have been trying to figure out to accomplish what you suggested, but so far I got nothing... I think I just really like my CoefficientGetter (refactored from GetCoeffs) class and I don't want to get rid of it. The main reason why is that my program is designed to be "localized" (It was realy just me playing around with properties files and it sort of stayed that way) so adding strings that will appear on the screen is sort of a pain now. The other thing I like about that class is that I just have to pass an int to the getCoefficients (refactored from CoeffGetter) method and it takes as many as needed, without me having to change anything after, if I want to add more equation types. Is there any other way by which to get rid of that TMI between the Equation implementations and Coefficientgetter?

I should probably say that I was guided by Winston Gutkowsky and his suggestion in this thread. He hasn't had a chance to take a look at the final implementations so there is a good chance I might have misinterpreted something that he said/suggested.

Ahh... I feel like the answer is right in front of me, but I just can't see it! I know what you are saying needs to happen... The Equation implementation should not interact with the user directly. What I don't understand is why is it so hard for me to figure out how to accomplish that!?

Thanks for your help!

[Junilu Lacar]

One nice thing about TDD is that it is actually more of a design activity than it is a testing activity. The tests become sample code for clients of a class. Want to see how to use an Equation class? Go look at the tests! Here's what I would have for testing Equations (you fill in the imports and all the other things that will make this code compile):

/**
 * Verifies that any implementation adheres to the general
 * contract for Equations. Extend this class for each specific 
 * implementation.
 */
public abstract class EquationTest {
    private Equation testSubject;

    @Before
    public void setUp() {
        testSubject = getTestSubject();
    } 

    @Test
    public void getCoefficients_does_not_return_null() {
        assertNotNull("Equation.getCoefficients() should not be null",
            testSubject.getCoefficients());
    }
    @Test
    public void provides_at_least_one_coefficients() {
        assertTrue("An equation must have at least one coefficient",
            testSubject.getCoefficients().length >= 1);
    }

    /**
     * Override this method in subclasses to provide a specific
     * Equation implementation.
     */
    protected abstract Equation getTestSubject();
}

// QuadraticEquationTest.java
public QuadraticEquationTest extends EquationTest {

    QuadraticEquation quadTestSubject;  // for tests specific to quadratic equations

    @Override
    protected Equation getTestSubject() {
        return new QuadraticEquation();
    }

    @Before
    public void setUp() {
        quadTestSubject = new QuadraticEquation();
    }

    @Test
    public void should_have_4_coefficients() {
         assertEquals("Should return array with 4 elements",
             4, quadTestSubject.getCoefficients().length);
    }

    @Test
    public void solve_with_positive_coefficients() {
         int[] coeffs = quadTestSubject.getCoefficients();
         for (int i : coeffs) {
              coeffs[i] = // simulate user input of positive #s
         }
         assertEquals(WHATEVER_YOU_EXPECT, quadTestSubject.solve(coeffs));
    }

    @Test
    public void solve_with_negative_coefficients() {
         int[] coeffs = quadTestSubject.getCoefficients();
         for (int i : coeffs) {
              coeffs[i] = // simulate user input of negative #s
         }
         assertEquals(WHATEVER_YOU_EXPECT, quadTestSubject.solve(coeffs));
    }
}

You don't have to get rid of the GetCoeffs class if you don't want to. Just have it be the client of Equation instead of having Equation know about GetCoeffs. That is, GetCoeffs will take an Equation and ask it to provide an array of coefficients. It will then iterate through the array as in the last two tests shown, get the user input and put each new value back into the array. When all coefficients have been replaced by user-provided values, the array gets passed back to Equation. Equation is oblivious to the fact that the numbers came from the user. It could have come from anywhere. That's why having a test first, is nice because you can work out what the API for a class is.

BTW, the last two tests there, I wouldn't really write them like that. I just did that to give you an idea of what the code in GetCoeffs would look like when you reverse the dependency from Equation--(depends on)-->GetCoeffs to GetCoeffs--(depends on)-->Equation.

So, here's another challenge, since you seem to like challenges. If I wanted to have a rule that said "Any implementation of Equation should throw an IllegalArgumentException if its solve() method is passed an array that has fewer elements in it that it requires." Where would you put the test for that and how would you write it? Enjoy!

[Jordan D. Williams]

Very, very interesting! Thanks so much!

I haven't really had a chance to dive into this as I have not been home, but I had a quick question about the tests that you coded...

Do both EquationTest and QuadraticEquationTest run simultaneously? In other words QuadraticEquationTest extends EquationTest, but the entirety of EquationTest also gets executed when you run the QuadraticEquationTest?

That question probably stems from my little understanding of inheritance. When you instantiate a class that extends a parent class, I guess the parent class also gets instantiated?

Basically you have this:

  @Override  
    protected Equation getTestSubject() {  
        return new QuadraticEquation();  
    }  

so that when you run this:

 @Before  
    public void setUp() {  
        testSubject = getTestSubject();  
    }

creates a QuadraticEquation object and runs the test on THAT type of object?

NOTE: I have not gotten to the Inheritance part of the Java Tutorials so I am a little hazy on that still. I also have zero understanding of how abstract classes and methods work, so I will have to read up on that too before I can fully understand what is going on in the above code. One step at a time though.

Thanks.

[Junilu Lacar]

Jordan D. Williams wrote:Do both EquationTest and QuadraticEquationTest run simultaneously? In other words QuadraticEquationTest extends EquationTest, but the entirety of EquationTest also gets executed when you run the QuadraticEquationTest?

Kind of like that. You can't actually run an EquationTest directly because it's an abstract class. So there's not really two classes running simultaneously when you run a subclass of EquationTest, like QuadraticEquationTest, for example. There's only that subclass running. The subclass just also happens to include whatever code was defined in its parent, EquationTest. When JUnit runs, it will see the tests that were inherited from the parent, as well as any methods marked by other annotations such as @Before and executes those as well.

That question probably stems from my little understanding of inheritance. When you instantiate a class that extends a parent class, I guess the parent class also gets instantiated?

As I explained above, only one instance is created: a QuadraticEquationTest. It's like when you were born, you took on some of your father's characteristics but that didn't create another "instance" of your father in you. In a sense, you "are" your father but at the same time you are "your father's child". Maybe it's better to say "You are of your father." Another analogy: you know how sometimes people will say "You laugh just like your father." or "You do this just like your father." It's kind of like that with object inheritance. Whatever the parent is programmed to do, so shall the child do it as well. Only as the programmer, you can chose to make the child either do something exactly like it's parent, in which case you'd just call/invoke the inherited method or you can make the child do that thing slightly differently than its parent, in which case you'd override the method and program some new logic.

Basically you have this:
(elided code)
creates a QuadraticEquation object and runs the test on THAT type of object?

Yes, when JUnit executes the child test, it will execute the methods inherited from the parent as well. When the code in the parent that invokes getTestSubject() is executed, Java can see that the runtime object is a QuadraticEquationTest and knows that by inheritance and overriding, it should execute the getTestSubject() method in the child, QuadraticEquationTest in this case. This is part of what's known as polymorphism in object-orientation. Kind of like saying, "Hey, you! Do the thing that your father does, but do it the way you do it."

Edit: An important thing to note -- the code in the parent still executes from the point of the view of the parent though. Notice that in the parent test, the declared return type of getTestSubject() is Equation, not QuadraticEquation or whatever implementation class of Equation you might come up with. That means that the tests in EquationTest are tests for the general contract for behavior of any implementation of Equation. In other words, any and all subclasses of Equation are expected to behave the way the tests in EquationTest expect any Equation should behave.

It's like your grandfather saying something like "Well, kiddo, you're going to do this because you're a Williams and a Williams always does this! So there!" It doesn't matter if it's you, your dad, your sister, your cousin, or whatever relative, no matter how far removed, he is always going to have the same expectation if the person he's talking to carries the Williams family name. So EquationTest is the "granddaddy" of all Equation tests and the code that it passes on to its children and its children's children and so on is going to enforce some basic behavior for anything that it can consider an Equation.

[Jordan D. Williams]

Junilu Lacar wrote:
It's like when you were born, you took on some of your father's characteristics but that didn't create another "instance" of your father in you. In a sense, you "are" your father but at the same time you are "your father's child". Maybe it's better to say "You are of your father." Another analogy: you know how sometimes people will say "You laugh just like your father." or "You do this just like your father." It's kind of like that with object inheritance. Whatever the parent is programmed to do, so shall the child do it as well. Only as the programmer, you can chose to make the child either do something exactly like it's parent, in which case you'd just call/invoke the inherited method or you can make the child do that thing slightly differently than its parent, in which case you'd override the method and program some new logic.

That, good sir, is probably the best explanation of Inheritance I will ever read! Awesome! You have a gift for these things - explaining things in a way people understand. Well... At least I get it when you explain things.

Thanks so much for that very, very clear analogy.