patternjavaMinor
Data structure to count occurrences of entries
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;
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
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
However, this will also mean that when you use variables of type
Naming
The
On the other hand, in
I think you can just drop that.
because the convention is to use
Unnecessary operations
In
this part pointlessly compares the first node against itself:
Since the first
this way would be more optimal:
Simplify using early returns
It can be a good idea to return early when the first node is
this way you could reduce one nesting level, which is slightly more readable,
like this:
You can apply the same logic to the
Node class staticYou 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.