5
\$\begingroup\$

I would love to hear any thoughtful suggestions on how to improve my programming style in order to be more understandable.

Listed below is a sample of my code which I wrote in Eclipse:

//Implement using a linked list
public class FrequencyBag<T>
{
// TO DO: Instance Variables
private Node firstNode;
private int numOfItems;

private class Node{
//Node Instance Variables

    private T data;
    private int frequency;
    private Node next;

    private Node(T Data, Node nextNode){
        data= Data;
        next=nextNode;
        frequency=1;
    }

    private void addF(){
        frequency++;
    }

    private int getF(){
        return frequency;
    }

}


/**
 * Constructor
 * Constructs an empty frequency bag.
 */
public FrequencyBag()
{
 firstNode=null;
 numOfItems =0;
}

/**
 * Adds new entry into this frequency bag.
 * @param aData the data to be added into this frequency bag.
 */
public void add(T aData)
{

  boolean found =false;
  Node currNode=firstNode;

  while(currNode !=null){
      if(currNode.data.equals(aData)){
          currNode.addF();
          found=true;
          break;
      }

      currNode= currNode.next;
  }

  if(!found){
      Node tempNode=firstNode;
      firstNode= new Node(aData,tempNode);

  }
  numOfItems++; 

  }
/**
 * Gets the number of occurrences of aData in this frequency bag.
 * @param aData the data to be checked for its number of occurrences.
 * @return the number of occurrences of aData in this frequency bag.
 */
public int getFrequencyOf(T aData)
{
    Node currNode= firstNode;
    while(currNode!=null){
        if(currNode.data.equals(aData)){
            return currNode.getF();
        }
     currNode= currNode.next;
    }

    return 0;
}

/**
 * Gets the maximum number of occurrences in this frequency bag.
 * @return the maximum number of occurrences of an entry in this
 * frequency bag.
 */
public int getMaxFrequency()
{
    if(firstNode!=null){
    Node currNode= firstNode;
    Node maxNode= firstNode;
    while(currNode!=null){
        if(currNode.getF()>=maxNode.getF()){
            maxNode=currNode;
        }
        currNode=currNode.next;
    }
    return maxNode.getF();
    }
    else{
        return 0;
    }
}

/**
 * Gets the probability of aData
 * @param aData the specific data to get its probability.
 * @return the probability of aData
 */
public double getProbabilityOf(T aData)
{
  if(firstNode!=null){
    boolean flag=true;
    double dataF=0;
    double prob=0;
    Node currNode= firstNode;
    while(currNode!=null){
        if(currNode.data.equals(aData)){
             dataF=(double)currNode.getF();
             flag=false;
             break;
        }
     currNode= currNode.next;
    }   
    if(!flag){
      prob= dataF/(double)numOfItems;
    }
    return prob;
  }
  else{
      return 0;
  }
}

/**
 * Empty this bag.
 */
public void clear()
{
    firstNode=null;
    numOfItems=0;
}

/**
 * Gets the number of entries in this bag.
 * @return the number of entries in this bag.
 */
public int size()
{
    return numOfItems;
}
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$ – Vogel612 Jan 22 '15 at 20:32
3
\$\begingroup\$

Make the inner Node class static

You can do this, because this class doesn't use anything in the enclosing class. A non-static class contains a private reference to the enclosing class, which is just a waste when you don't really need it.

When you make the class static you will also need to add a template parameter of its own, like this:

private static class Node<T> {
    // ...
}

However, this will also mean that when you use variables of type Node, you will also need to specify the template parameter T, changing all occurrences to Node<T>.

Naming

The getF and addF methods are poorly named. It would be easier to read if you spell them out as getFrequency, and incrementFrequency. On the other hand, in getFrequencyOf, I find the "Of" redundant, I think you can just drop that. Data is not a good variable name in Java, because the convention is to use camelCase, for example data instead.

Unnecessary operations

In getMaxFrequency, this part pointlessly compares the first node against itself:

    if (firstNode != null) {
        Node currNode = firstNode;
        Node maxNode = firstNode;
        while (currNode != null) {

Since the first if already ensures that the first node is not null, this way would be more optimal:

    if (firstNode != null) {
        Node maxNode = firstNode;
        Node currNode = firstNode.next;
        while (currNode != null) {

Simplify using early returns

It can be a good idea to return early when the first node is null, this way you could reduce one nesting level, which is slightly more readable, like this:

    if (firstNode == null) {
        return 0;
    }
    Node maxNode = firstNode;
    Node currNode = firstNode.next;
    while (currNode != null) {
        if (currNode.getF() >= maxNode.getF()) {
            maxNode = currNode;
        }
        currNode = currNode.next;
    }
    return maxNode.getF();

You can apply the same logic to the getProbabilityOf method (drop the "Of", return early, iterate from firstNode.next).

\$\endgroup\$
2
\$\begingroup\$

You should put some spacing in between your operators and other things.

boolean found =false;
Node currNode=firstNode;

while(currNode !=null){
   if(currNode.data.equals(aData)){
       currNode.addF();
       found=true;
       break;
   }

  currNode= currNode.next;

Should look like this

boolean found = false;
Node currNode = firstNode;

while (currNode != null) {
   if (currNode.data.equals(aData)) {
       currNode.addF();
       found = true;
       break;
   }

   currNode = currNode.next;

Make sure that you do the following

  • Indent consistently
  • space out the operators and the opening braces
  • Use Egyptian Style bracing (unless coding in C#
  • Do away with unneeded comments

Then it looks something like this

public class FrequencyBag<T> {
    private Node firstNode;
    private int numOfItems;

    private class Node {
        private T data;
        private int frequency;
        private Node next;

        private Node(T Data, Node nextNode) {
            data = Data;
            next = nextNode;
            frequency = 1;
        }

        private void addF() {
            frequency++;
        }

        private int getF() {
            return frequency;
        }
    }

    public FrequencyBag() {
        firstNode = null;
        numOfItems = 0;
    }

    /**
    * Adds new entry into this frequency bag.
    * @param aData the data to be added into this frequency bag.
    */
    public void add(T aData) {
        boolean found = false;
        Node currNode = firstNode;

        while(currNode != null){
            if(currNode.data.equals(aData)){
                currNode.addF();
                found = true;
                break;
            }
            currNode = currNode.next;
        }

        if(!found) {
            Node tempNode = firstNode;
            firstNode = new Node(aData,tempNode);
        }
        numOfItems++; 
    }

    /**
    * Gets the number of occurrences of aData in this frequency bag.
    * @param aData the data to be checked for its number of occurrences.
    * @return the number of occurrences of aData in this frequency bag.
    */
    public int getFrequencyOf(T aData) {
        Node currNode= firstNode;
        while(currNode!=null){
            if(currNode.data.equals(aData)){
                return currNode.getF();
            }
        currNode= currNode.next;
        }
        return 0;
    }

    /**
    * Gets the maximum number of occurrences in this frequency bag.
    * @return the maximum number of occurrences of an entry in this
    * frequency bag.
    */
    public int getMaxFrequency() {
        if(firstNode!=null){
            Node currNode= firstNode;
            Node maxNode= firstNode;
            while(currNode!=null){
                if(currNode.getF()>=maxNode.getF()){
                    maxNode=currNode;
                }
                currNode=currNode.next;
            }
            return maxNode.getF();
        } else {
            return 0;
        }
    }

    /**
    * Gets the probability of aData
    * @param aData the specific data to get its probability.
    * @return the probability of aData
    */
    public double getProbabilityOf(T aData) {
        if(firstNode != null) {
            boolean flag = true;
            double dataF = 0;
            double prob = 0;
            Node currNode = firstNode;
            while(currNode != null) {
                if(currNode.data.equals(aData)) {
                    dataF = (double)currNode.getF();
                    flag = false;
                    break;
                }
                currNode = currNode.next;
            }   
            if (!flag) {
                prob = dataF / (double)numOfItems;
            }
            return prob;
        }
        else {
            return 0;
        }
    }

    /**
    * Empty this bag.
    */
    public void clear() {
        firstNode = null;
        numOfItems = 0;
    }

    /**
    * Gets the number of entries in this bag.
    * @return the number of entries in this bag.
    */
    public int size() {
        return numOfItems;
    }
}

And is much easier to read.

I left the block comments because they looked like they were there for intellisense.

Your Variable naming was pretty good, but I would reconsider these

  • aData
  • dataF
  • prob
  • flag

flag isn't so bad except I don't know what the flag means.

I assume prob is probability, but probability of what?

And that leads to this Method name getProbabilityOf()

Get probability of .... what?

Another one that isn't so bad getFrequencyOf() you could probably drop the Of off the end on this one, or elaborate what you are getting the frequency of, these last two items might just be really nit-picky, so take them with a grain of salt.

\$\endgroup\$

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.