Basic class design

[Matt Kidd]

Okay. I recall accessors and mututators but writing them is a totally different story. The class is suppsed to model a complex number. My main difficulty is with the addition and subtraction methods as I'm not sure if the logic I used would actually work. Thoughts?
Quick overview:
a) Addition of two Complex nubmers: the real parts are added together and the imaginary parts are added together
b) Subtraction of two Complex nubmers: the real part of the left operand (supposed to be this) is subtracted from the real part of the left operand and the imaginary part of the right operand si subtracted from the imaginary part of the left operand.
c) Printing Complex numbers in the form (a,b) where a is the real part and b is the imaginary part.

public class Complex {
//member variables
private float realPart;
private float imaginaryPart;
//constructors
public void Complex()
{
realPart = 0.0F;
imaginaryPart = 0.0F;
}
public void Complex(float number)
{
realPart = number;
}
public void Complex(float inumber)
{
imaginaryPart = inumber;
}
public void Complex(float number, float inumber)
{
realPart = number;
imaginaryPart = inumber;
}

//accessors
public float getRealPart ()
{
return this.realPart;
}
public float getImaginaryPart()
{
return this.imaginaryPart;
}

//mutators
//adds to Complex numbers together
public Complex void addition(Complex a)
{
float tempReal = 0.0F;
float tempImag = 0.0F;

tempReal = this.getRealPart() + a.getRealPart();
tempImag = this.getImaginaryPart() + a.getImaginaryPart();
Complex temp = new Complex(tempReal, tempImag);
return temp;
}
//subtracts two Complex numbers
public Complex void subtraction(Complex a)
{
float tempReal = 0.0F;
float tempImag = 0.0F;

tempReal = this.getRealPart() - a.getRealPart();
tempImag = this.getImaginaryPart() - a.getImaginaryPart();
Complex temp = new Complex(tempReal, tempImag);
return temp;
}
//print out a Complex number
public void print(float number, float inumber)
{
System.out.println( "(" + number + ", " + inumber + "i)" );
}
}

I figured that in the addition and subtraction methods the newly created Complex would be garbage collected once out of the scope of the method. Only problem would be if there were numerous calls and garbage collection hadn't been done in a while.

[Sayed Ibrahim Hashimi]

Ok, first thing that I noticed were that all your "Constructors" had the return type of void. They should have no return type, otherwise they are not constructors, just regular methods. So you should change all of those to have this form:

public Complex(...)

Accessors, you got those right, they are the get...() methods. Here are the mutators:

public void setRealPart(int realPart)
{
    this.realPart = realPart;
}
//...so on

About the garbage collection, you don't have to worry about that because in your add/subtract method you are creating a temp Complex object so everytime you go into it you are getting a new object to deal with. You don't have to worry about memory leaks, because this is not C++, although I have heard there are rare occassions of memory leaks, but I don't know when this happens. Anyway, hope this clears somethings up.
[ August 20, 2002: Message edited by: Ibrahim Hashimi ]

[Neil Laurance]

Some additional points:
[1] The following 2 constructors cannot be distinguished by the compiler:
public void Complex(float number)
public void Complex(float inumber)
[2] A better set of constructors:
Complex() { this(0.0F, 0.0F); }
Complex(float re) { this(re, 0.0F); }
Complex(float re, float im) {
this.re = re;
this.im = im;
}
[3] I would suggest replacing print with toString like:
public String toString() {
return "{" + re + "," + im + "}";
}
Using toString, you can then simply create output with:
Complex c = new Complex(3.0F, -4.0F);
System.out.println("c is " + c);
Hope some of this helps
[ August 20, 2002: Message edited by: Neil Laurance ]

[Frank Carver]

Just a few comments. The may all seem like fiddlin'-small points, but I feel it's really important to get each step right before you move on, so please don't be offended. What I plan to do is first tighten up your existing code, then look at how adding new features might work to try and show some of what "maintainability" means.
Let's consider your "mutators":

//mutators
 
//adds to Complex numbers together
public Complex void addition(Complex a)
{
  float tempReal = 0.0F;
  float tempImag = 0.0F;
 
  tempReal = this.getRealPart() + a.getRealPart();
  tempImag = this.getImaginaryPart() + a.getImaginaryPart();
 
  Complex temp = new Complex(tempReal, tempImag);
  return temp; 
}
 
//subtracts two Complex numbers
public Complex void subtraction(Complex a)
{
  float tempReal = 0.0F;
  float tempImag = 0.0F;
 
  tempReal = this.getRealPart() - a.getRealPart();
  tempImag = this.getImaginaryPart() - a.getImaginaryPart();
 
  Complex temp = new Complex(tempReal, tempImag);
  return temp; 
}

First a few overall issues.
A "mutator" is a method which changes (mutates) the contents of an object. These don't. I'd call these something like "business methods".
The mutators as suggested by a previous poster are a reasonable choice.
I hope you have already spotted the "void" which shouldn't be there. These are methods which return a type Complex, not a type "void" or "Complex void" (which makes no sense).
I'm very wary of comments which say something that the code should say. Remember that most people who see one of these methods will be seing a call to it, not the definition, so a comment in the definition is of little use. Code that reads well and expresses its intention is much more valuable. How about removing the redundant comments and changing the method names to "add" and "subtract" ?
Now on to the more tricky stuff which some people may disagree with.
I hope you still have the freedom to change the public interface to this class, as I feel you have started down a dangerous road. Imagine someone comes to this code and doesn't know or can't quite remember exactly what these methods actually do. It's not to great a leap to mistakenly assume that the "add" method actually accumulates it's argument to the object being operated on. After all "c1.add(c2)" can easily be read as if it is like "c1 += c2". This seems like needless possible confusion if we can make the code clearer.
I suggest that these methods shouldn't be instance methods at all, but class methods. Consider:

public static Complex add(Complex a, Complex b)
{
  float tempReal = 0.0F;
  float tempImag = 0.0F;
 
  tempReal = a.getRealPart() + a.getRealPart();
  tempImag = a.getImaginaryPart() + b.getImaginaryPart();
 
  Complex temp = new Complex(tempReal, tempImag);
  return temp; 
}

I feel that this much more strongly expresses what you are actually doing. because it is "static", it cannot modify an object's contents, and an "add" method which takes two arguments and returns the sum is very straight forward to understand.
If you really need the old syntax as well, it's trivial to add it, but please think hard before doing it:

public Complex add(Complex a)
{
  return add(this, a);
}

When we do this for subtract too, and add in some of the previous suggestions, here's what we get:

public class Complex {
 
  private float realPart;
  private float imaginaryPart;
 
  public Complex(float real, float imaginary) {
    realPart = real;
    imaginaryPart = imaginary;
  }
 
  public Complex(float real) {
    this(real, 0.0F); 
  }
 
  public Complex() { 
    this(0.0F, 0.0F); 
  }
  
  //accessors
  public float getRealPart() {
    return realPart;
  }
  
  public float getImaginaryPart() {
    return imaginaryPart;
  }
  
  //mutators
  public void setRealPart(float real) {
    realPart = real;
  }
  
  public void setImaginaryPart(float imaginary) {
    imaginaryPart = imaginary;
  }
  
  public static Complex add(Complex a, Complex b)
  {
    float tempReal = 0.0F;
    float tempImag = 0.0F;
   
    tempReal = a.getRealPart() + b.getRealPart();
    tempImag = a.getImaginaryPart() + b.getImaginaryPart();
   
    Complex temp = new Complex(tempReal, tempImag);
    return temp; 
  }
  
  public static Complex subtract(Complex a, Complex b)
  {
    float tempReal = 0.0F;
    float tempImag = 0.0F;
   
    tempReal = a.getRealPart() - b.getRealPart();
    tempImag = a.getImaginaryPart() - b.getImaginaryPart();
   
    Complex temp = new Complex(tempReal, tempImag);
    return temp; 
  }
  
  public String toString() {
    return "(" + realPart + "," + imaginaryPart + ")";
  }
}

Before we move on to adding new features, let's take a look at the code, to make sure it looks OK, and check if it is as good as it can be. If we can see any duplication of code, we should remove it. "Once and Only Once" (from Extreme Programming) or "DRY :- Don't Repeat Yourself" (from the Pragmatic Programmers) should be our mantra.
I can still see some duplication: "realPart = real;" and "imaginaryPart = imaginary;" both appear twice. So let's call setRealPart and setImaginaryPart in the constructor:

 
  public Complex(float real, float imaginary) {
    setRealPart(real);
    setImaginaryPart(imaginary);
  }

"add" and "subtract" have lots of similar lines, so we need to do something there, too. I can think of two basic ways to approach this. One is to refactor the code to gather the common lines together somehow. The other is to see how many of those similar lines we can eliminate altogether. I don't like to loose information, but a variables named "temp" don't really carry much information anyway. I feel that the following is only a bit less readable, and it does remove a lot of the apparent duplication !

  public static Complex add(Complex a, Complex b)
  {
    Complex ret = new Complex(
      a.getRealPart() + b.getRealPart(), 
      a.getImaginaryPart() + b.getImaginaryPart());
    return ret;
  }
  
  public static Complex subtract(Complex a, Complex b)
  {
    Complex ret = new Complex(
      a.getRealPart() - b.getRealPart(), 
      a.getImaginaryPart() - b.getImaginaryPart());
    return ret;
  }

Although there is still some duplication lurking there, eliminating it might overcomplicate the problem, so lets stop there for a moment:

public class Complex {
  
  private float realPart;
  private float imaginaryPart;
  
  public Complex(float real, float imaginary) {
    setRealPart(real);
    setImaginaryPart(imaginary);
  }
 
  public Complex(float real) {
    this(real, 0.0F); 
  }
  
  public Complex() { 
    this(0.0F, 0.0F); 
  }
  
  //accessors
  public float getRealPart() {
    return realPart;
  }
  
  public float getImaginaryPart() {
    return imaginaryPart;
  }
  
  //mutators
  public void setRealPart(float real) {
    realPart = real;
  }
  
  public void setImaginaryPart(float imaginary) {
    imaginaryPart = imaginary;
  }
  
  public static Complex add(Complex a, Complex b)
  {
    Complex ret = new Complex(
      a.getRealPart() + b.getRealPart(), 
      a.getImaginaryPart() + b.getImaginaryPart());
    return ret;
  }
  
  public static Complex subtract(Complex a, Complex b)
  {
    Complex ret = new Complex(
      a.getRealPart() - b.getRealPart(), 
      a.getImaginaryPart() - b.getImaginaryPart());
    return ret;
  }
  
  public String toString() {
    return "(" + realPart + "," + imaginaryPart + ")";
  }
}

I reckon by now you are beginning to wonder if all of this is getting anywhere. Sure, the methods are a bit shorter, but there's more of them. Now's the time to add some new functionality and see how easy it is.
Let's say we liked the idea of a way of "accumulating" adds in the same object ( like += ) when we thought about it earlier. Could come in handy if all that object creation and garbage collection does become a problem. Doing it the "old" way might have looked something like:

//adds a Complex numbers to this one
  public void acccumulation(Complex a)
  {
    float tempReal = 0.0F;
    float tempImag = 0.0F;
   
    tempReal = this.getRealPart() + a.getRealPart();
    tempImag = this.getImaginaryPart() + a.getImaginaryPart();
  
    this.realPart = tempReal;   
    this.imaginaryPart = tempImaginary;   
  }

With all the new tricks we have learned and written for ourselves now, we might start with something like:

  public void increaseBy(Complex that)
  {
    setRealPart(this.getRealPart() + that.getRealPart());
    setImaginaryPart(this.getImaginaryPart() + that.getImaginaryPart());
  }

But we've also learned to look at the whole code and look for duplication, so we don't stop there. I can't help noticing that we now sum the reals and the imaginaries in both "add" and "increaseBy". Surely we can eliminate that. Making "increaseBy" call "add" doesn't help cut out the object creation, but we can refactor "add" to use "increaseBy"!

  public static Complex add(Complex a, Complex b)
  {
    Complex ret = new Complex(a.getRealPart(), a.getImaginaryPart());
    ret.increaseBy(b);
    return ret;
  }

Interesting. Still looks a bit clunky, though, so I'd add a new constructor:

 
  public Complex(Complex that) {
    this(that.getRealPart()), that.getImaginaryPart());
  }

then we can write add as:

  public static Complex add(Complex a, Complex b)
  {
    Complex ret = new Complex(a);
    ret.increaseBy(b);
    return ret;
  }

That seems quite neat. And notice how there was a lot less refactoring needed after that change, because the class was in much better "shape" to start with.
Now imagine someone also needs the "inverse" of a complex number (-re,-im). Can we fix it? Yes we can!

  public Complex inverse()
  {
    return new Complex(-getRealPart(), -getImaginaryPart());
  }

That was easy, wasn't it!. But, before we move on, we must look at the whole thing again. Think this is too small a change to make a difference? Think again! A subtract is defined as just the addition of an inverse, so we can reduce the subtract code to:

  public static Complex subtract(Complex a, Complex b)
  {
    return add(a, b.inverse());
  }

And there goes that niggling similarity between add/increaseBy and subtract.
So a final look at where the code is now:

public class Complex {
  
  private float realPart;
  private float imaginaryPart;
  
  public Complex(float real, float imaginary) {
    setRealPart(real);
    setImaginaryPart(imaginary);
  }
  
  public Complex(float real) {
    this(real, 0.0F); 
  }
  
  public Complex() { 
    this(0.0F, 0.0F); 
  }
  
  public Complex(Complex that) {
    this(that.getRealPart()), that.getImaginaryPart());
  }
  
  //accessors
  public float getRealPart() {
    return realPart;
  }
  
  public float getImaginaryPart() {
    return imaginaryPart;
  }
  
  //mutators
  public void setRealPart(float real) {
    realPart = real;
  }
  
  public void setImaginaryPart(float imaginary) {
    imaginaryPart = imaginary;
  }
  
  public static Complex add(Complex a, Complex b)
  {
    Complex ret = new Complex(a);
    ret.increaseBy(b);
    return ret;
  }
  
  public static Complex subtract(Complex a, Complex b)
  {
    return add(a, b.inverse());
  }
  
  public void increaseBy(Complex that)
  {
    setRealPart(this.getRealPart() + that.getRealPart());
    setImaginaryPart(this.getImaginaryPart() + that.getImaginaryPart());
  }
  
  public Complex inverse()
  {
    return new Complex(-getRealPart(), -getImaginaryPart());
  }
  
  public String toString() {
    return "(" + realPart + "," + imaginaryPart + ")";
  }
}

Whew. That was more than I intended writing! Now it's over to you. Can you see any more ways to squeeze it? I can spot a few minor things I might change, but the main thing to learn is how to study your own code and make it the best you can.
I hope this helps someone.
Another day, maybe ask about how writing little automatic tests as you go along can help prevent screwups when tinkering like this...
[ August 20, 2002: Message edited by: Frank Carver ]

[Matt Kidd]

Actually Frank that was great and proved many more things to me besides a quick an easy way to write a class. A side effect, since I was able to follow your whole thought process, was that the end result class was what I initially had in mind to begin with. Two years ago I could of done that no problem but now I have to follow along a sniff a the lingering scent of a dying trail. I'll get back to it but I may have to break out my old college notes. I'll more than likely end up printing this out.

[Sayed Ibrahim Hashimi]

Frank...wow one of the best and most thoughtful posts I've ever seen. Way to go.

[Ilja Preuss]

Frank, very nice post! And in some parts even more extreme than I would have done it!
Just one additional thought:

After all "c1.add(c2)" can easily be read as if it is like "c1 += c2". This seems like needless possible confusion if we can make the code clearer.

Perhaps we could find a better name for that method if we want it to remain an instance method. I am not very firm in english mathematical terms - how could we express that we want to get back the sum? addUp? calculateSum?
What do you think?
[ August 21, 2002: Message edited by: Ilja Preuss ]

[Neil Laurance]

Mutator methods:
c1.plusEquals(c2);
c1.minusEquals(c2);
Non mutating methods, returns a new Complex:
c1 = c1.plus(c2);
c1 = c1.minus(c2);
c1 = c1.conjugate();
c1 = c1.squared();
I prefer the second approach personally
:roll:
[ August 21, 2002: Message edited by: Neil Laurance ]

[Robert Paris]

Wow! Frank, that was without a doubt the best Post I have ever seen! I hope everyone (myself included) can start doing more posts like that:
1. Informative
2. Well Organized
3. Very Well Explained
4. Gradually Increasing Knowledge/Growth
JavaRanch Sheriff's, is there any way to put some picture of a star next to Frank's name which will link to this post when you click on it?
Wow.
[ August 21, 2002: Message edited by: Robert Paris ]