HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Data structure to count occurrences of entries

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
occurrencesstructurecountdataentries

Problem

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
{
// 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;

Solution

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 {
    // ...
}


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.

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).

Code Snippets

private static class Node<T> {
    // ...
}
if (firstNode != null) {
        Node currNode = firstNode;
        Node maxNode = firstNode;
        while (currNode != null) {
if (firstNode != null) {
        Node maxNode = firstNode;
        Node currNode = firstNode.next;
        while (currNode != null) {
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();

Context

StackExchange Code Review Q#78355, answer score: 3

Revisions (0)

No revisions yet.