1
\$\begingroup\$

I've found two ways of writing the same program (one that only uses local variables, but no methods other than the main one) and other that uses one instance variable that is used in two methods.

This program aims to simulate the radioactive decay of atoms and uses a random number generator.

Here are the two versions of the code:

/*
 * File: HalfLife.java
 * 
 * This program simulates the decaying process of radioactive atoms that
 * have a half life of one year. The program will show the amount of atoms
 * remaining at the end of each year until all atoms has decayed
 */

import acm.program.*;
import acm.util.RandomGenerator;

public class HalfLife extends ConsoleProgram {

    /* Number of atoms */
    private static final int NATOMS = 10000;

    public void run() {
        println("There are " + NATOMS + " atoms initially.");
        int remainingAtoms = NATOMS;
        int year = 0;
        while (remainingAtoms > 0) {
            /* Simulate decaying process for each atom. Each atom has a 50% chance of decay. */
            for (int i = remainingAtoms; i > 0; i--) {
                if (rgen.nextBoolean()) {
                    remainingAtoms--;
                }
            }
            year++;
            println("There are " + remainingAtoms + " atoms at the end of year " + year);
        }
    }

    /* Create an instance variable for the random number generator */
    private RandomGenerator rgen = RandomGenerator.getInstance();
}

That one had only local variables, now for the other one (I removed comments in this one since it's redundant):

public class AtomicReduction extends ConsoleProgram {

    private static final int BEG_ATOMS = 10000;

    public void run () {
        int nYears = 1;
        println ("There are " + BEG_ATOMS + " atoms in the beginning");
        while (nAtoms > 0) {
            println ("There are " + (atomicRed (nAtoms)) + " atoms at the end of year " + nYears);
            nYears++;
        }
    }

    private int atomicRed (int initialAtoms) {
        for (int i = 0; i < initialAtoms; i++) {
            boolean isReduced = rGen.nextBoolean();
            if (isReduced) nAtoms--;
        }
        return nAtoms; 
    }

     private int nAtoms = BEG_ATOMS;

     private RandomGenerator rGen = RandomGenerator.getInstance();
}

Which is "the better" version? And why? I kind of prefer the first one because it keeps everything local, however the latter has more decomposition and can be potentially easier to understand (with proper comments).

Note: this makes use of ACM libraries so the main method is public void run.

\$\endgroup\$
4
\$\begingroup\$

Firstly, some minor nitpicks on your code:

   for (int i = remainingAtoms; i > 0; i--) {

Why are you counting down instead of up? I can see why after careful inspection, but a comment would be useful.

        println ("There are " + (atomicRed (nAtoms)) + " atoms at the end of year " + nYears);

You've violated Command-Query Seperation. atomicRed changes the state of the object, and so it really shouldn't return information about the object. It certainly shouldn't appear in the middle of a print a call.

        boolean isReduced = rGen.nextBoolean();
        if (isReduced) nAtoms--;

Why did you seperate isReduced onto its own line?

As for your actual question: people differ on this point. My general theory is that seperating out functions is good. But don't move previously local variables to the class in order to do that. Doing so only makes your class harder to follow.

In this case, I suggest something like

while(nAtoms > 0)
{
     nAtoms = remainingAtomsAfterYear(nAtoms);
     println("There are " + nAtoms + " atoms at the end of year " + nYears);
     nYears++;
}

This allows you to pull the logic regarding atom decay into a separate function, but avoids storing state on the class. I think this actually gets the best of both approaches.

Alternately, you could do:

while(!atoms.empty())
{
    atoms.simulateYear();
    println("There are " + atoms.count() + " atoms at the end of year " + atoms.years());
}

And move all the logic into another class. That's overkill here, but is helpful if the state gets more complicated.

\$\endgroup\$
  • \$\begingroup\$ Depending on the stage of learning, I would say moving the logic into its own class would be very helpful. You could even encapsulate the decay logic into a strategy to allow different rates of decay. Learning these good habits early is easier if you're ready for it. :) \$\endgroup\$ – David Harkness Apr 14 '13 at 20:25
  • \$\begingroup\$ "You've violated Command-Query Seperation. atomicRed changes the state of the object, and so it really shouldn't return information about the object. It certainly shouldn't appear in the middle of a print a call." - Why is this bad? Does it make the code harder to understand or is prone to bugs? \$\endgroup\$ – Antonio Ricardo Diegues Silva Apr 14 '13 at 20:40
  • 1
    \$\begingroup\$ @AntonioRicardoDieguesSilva, it makes code easier to follow which helps it be less prone to bugs. As you've used the code it looks like atomicRed() is just returning some information that you are printing. But atomicRed() is actually the part that does all the work. It would be really easy to miss that its there. \$\endgroup\$ – Winston Ewert Apr 14 '13 at 20:43
  • \$\begingroup\$ As for creating a class just for the logic that seems an interesting idea but I'm just learning to build java classes. Ill keep that idea in mind as I'll progress in my studies! Thanks to both of you for your input! \$\endgroup\$ – Antonio Ricardo Diegues Silva Apr 14 '13 at 20:43
  • \$\begingroup\$ @AntonioRicardoDieguesSilva, good luck with your studies! \$\endgroup\$ – Winston Ewert Apr 14 '13 at 20:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.