Jump to content
  • Advertisement
Sign in to follow this  
l jsym l

[java] Is this code a bad programming practice?

This topic is 2584 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hey, I did a project for a Java class that I am currently taking to get my Bachelors Degree. I'm still pretty new to programming and no expert by any means. Anyway, I finished this assignment and handed it in, then just yesterday I was told by my professor that how I coded my classes are completely wrong and "I don't have any idea how to program". He says I'm not suppose to code a random generator in a class. Is this true? Also, I got all the correct output for this program and it runs successfully every time I run it.

The program has a warehouse, producers, and consumers. The user enters min and max values for all the classes and then the random generator generates how much foos the warehouse can hold, how much foos the producers put in the warehouse, and how much foos the consumers take out. The consumers and producers are a queue that just keep rotating as they are called.

Here is my code:

Warehouse Class
import java.util.*;

public class Producer
{
// Random generator used to generate random numbers for input
Random generator = new Random();

// Queue used to store amounts
Queue<Integer> pQueue = new LinkedList<Integer>();

// Private variables used for Producer Class
private int foos_max, foos_min;
private int producer_max, producer_min;
private int total_produced = 0, entered = 0;;

/*
* Default Constructor: Sets foos_min, foos_max, producer_max, and producer_min to default 0.
*/
public Producer()
{
this.foos_max = 0;
this.foos_min = 0;
this.producer_max = 0;
this.producer_min = 0;
}

/*
* Default Constructor: Initializes variables to input parameter
* @param a Initial value of producer_min
* @param b Initial value of producer_max
* @param c Initial value of foos_min
* @param d Initial value of foos_max
*/
public Producer( int a, int b, int c, int d )
{

// Producers Min and Max
this.producer_min = a;
this.producer_max = b;

// Foos Min and Max
this.foos_min = c;
this.foos_max = d;

// Generate Random number for number of producers.
int num_producers = generator.nextInt( producer_max - producer_min ) + producer_min;

for( int i = 0; i < num_producers; i++ )
{
int temp = generator.nextInt( foos_max - foos_min + 1) + foos_min;

pQueue.add( temp );
}
}

/*
* produceFoo() Adds the amount of the front of pQueue to the warehouse
* @param a The warehouse that is getting "foos" added to it
*/
public void produceFoo( Warehouse a )
{
if ( ( pQueue.element() + a.getFoos() ) <= a.getMax() )
{
int temp = pQueue.remove();
a.addFoo( temp );
total_produced = total_produced + temp;
pQueue.add( temp );
entered++;
}
}

/*
* getSize() Returns the size of pQueue
*/
public int getSize()
{
return pQueue.size();
}

/*
* toString() Returns a String of the elements in the queue.
*/
public String toString()
{
String tempStr = "";
for ( int i = 0; i < pQueue.size(); i++ )
{
int tempInt = pQueue.remove();
tempStr = tempStr + " " + tempInt;
pQueue.add( tempInt );
}

return tempStr;

}

/*
* getNext() Returns the first element in the queue
*/
public int getNext()
{
return pQueue.element();
}

/*
* getProduced() Returns total_produced, the total amount of "foos" produced
*/
public int getProduced()
{
return total_produced;
}

/*
* getEntered() Returns entered, the total amount of times "foos" were entered into warehouse
*/
public int getEntered()
{
return entered;
}

}


Consumer Class
import java.util.*;

public class Consumer
{
// Random generator used to generate random numbers for input
Random generator = new Random();

// Queue used to store amounts
Queue<Integer> cQueue = new LinkedList<Integer>();

// Private variables used in Consumer Class
private int consumer_min, consumer_max;
private int foos_min, foos_max;
private int total_consumed = 0, entered = 0;

/*
* Default Constructor: Sets foos_max, foos_min, consumer_max, and consumer_min to default 0
*/
public Consumer()
{
this.foos_max = 0;
this.foos_min = 0;
this.consumer_max = 0;
this.consumer_min = 0;
}

/*
* Default Constructor: Initializes variables to input parameter
* @param a Initial value of consumer_min
* @param b Initial value of consumer_max
* @param c Initial value of foos_min
* @param d Initial value of foos_max
*/
public Consumer( int a, int b, int c, int d )
{
// Consumer Min and Max
this.consumer_min = a;
this.consumer_max = b;

// Foos Min and Max
this.foos_min = c;
this.foos_max = d;

// Generate random number for number of producers
int num_consumers = generator.nextInt( consumer_max - consumer_min ) + consumer_min;

for( int i = 0; i < num_consumers; i++ )
{
// Get random number to put into queue
int rTemp = generator.nextInt( foos_max - foos_min + 1 ) + foos_min;

// Set random number into cQueue
cQueue.add( rTemp );
}
}

/*
* consumeFoo() Removes the amount at the front of cQueue from warehouse
* @param a The warehouse that is getting "foos" removed from it
*/
public void consumeFoo( Warehouse a )
{
if ( cQueue.element() <= a.getFoos() )
{
int temp = cQueue.remove();
a.removeFoo( temp );
total_consumed = total_consumed + temp;
cQueue.add( temp );
entered++;
}
}

/*
* getSize() Returns the size of cQueue
*/
public int getSize()
{
return cQueue.size();
}

/*
* toString() Returns tempStr, a string of all the elements in the queue
*/
public String toString()
{
String tempStr = "";
for ( int i = 0; i < cQueue.size(); i++ )
{
int tempInt = cQueue.remove();
tempStr = tempStr + " " + tempInt;
cQueue.add( tempInt );
}

return tempStr;

}

/*
* getNext() Returns the first element in the queue
*/
public int getNext()
{
return cQueue.element();
}

/*
* getConsumed() Returns total_consumed, the total number of "foos" consumed
*/
public int getConsumed()
{
return total_consumed;
}

/*
* getEntered() Returns entered, the total number of times "foos" were consumed from the warehouse
*/
public int getEntered()
{
return entered;
}
}


Producer Class
import java.util.*;

public class Producer
{
// Random generator used to generate random numbers for input
Random generator = new Random();

// Queue used to store amounts
Queue<Integer> pQueue = new LinkedList<Integer>();

// Private variables used for Producer Class
private int foos_max, foos_min;
private int producer_max, producer_min;
private int total_produced = 0, entered = 0;;

/*
* Default Constructor: Sets foos_min, foos_max, producer_max, and producer_min to default 0.
*/
public Producer()
{
this.foos_max = 0;
this.foos_min = 0;
this.producer_max = 0;
this.producer_min = 0;
}

/*
* Default Constructor: Initializes variables to input parameter
* @param a Initial value of producer_min
* @param b Initial value of producer_max
* @param c Initial value of foos_min
* @param d Initial value of foos_max
*/
public Producer( int a, int b, int c, int d )
{

// Producers Min and Max
this.producer_min = a;
this.producer_max = b;

// Foos Min and Max
this.foos_min = c;
this.foos_max = d;

// Generate Random number for number of producers.
int num_producers = generator.nextInt( producer_max - producer_min ) + producer_min;

for( int i = 0; i < num_producers; i++ )
{
int temp = generator.nextInt( foos_max - foos_min + 1) + foos_min;

pQueue.add( temp );
}
}

/*
* produceFoo() Adds the amount of the front of pQueue to the warehouse
* @param a The warehouse that is getting "foos" added to it
*/
public void produceFoo( Warehouse a )
{
if ( ( pQueue.element() + a.getFoos() ) <= a.getMax() )
{
int temp = pQueue.remove();
a.addFoo( temp );
total_produced = total_produced + temp;
pQueue.add( temp );
entered++;
}
}

/*
* getSize() Returns the size of pQueue
*/
public int getSize()
{
return pQueue.size();
}

/*
* toString() Returns a String of the elements in the queue.
*/
public String toString()
{
String tempStr = "";
for ( int i = 0; i < pQueue.size(); i++ )
{
int tempInt = pQueue.remove();
tempStr = tempStr + " " + tempInt;
pQueue.add( tempInt );
}

return tempStr;

}

/*
* getNext() Returns the first element in the queue
*/
public int getNext()
{
return pQueue.element();
}

/*
* getProduced() Returns total_produced, the total amount of "foos" produced
*/
public int getProduced()
{
return total_produced;
}

/*
* getEntered() Returns entered, the total amount of times "foos" were entered into warehouse
*/
public int getEntered()
{
return entered;
}

}


Main Class - testing file
import java.util.*;
import java.io.*;

public class Main
{
// Number of ticks the simulation has executed
static int simTicks = 0;

public static void main( String[] args)
{

// Create a object of Random used for generating random numbers
Random generator = new Random();


// Create a scanner object for system input
Scanner scanner = new Scanner( System.in );


// Creating variables for simulation
int warehouse_min, warehouse_max;
int p_c_min, p_c_max;
int foos_min, foos_max;


// Get Max and Min Values for the warehouse
System.out.println( "What is the minimum that the warehouse can hold?");
warehouse_min = scanner.nextInt();
System.out.println( "What is the maximum that the warehouse can hold?");
warehouse_max = scanner.nextInt();


// Creating the warehouse
Warehouse wHouse = new Warehouse( warehouse_min, warehouse_max );


// Get max and Min Values for Producers and Consumers
System.out.println( "What is the minimum number of producers and consumers?" );
p_c_min = scanner.nextInt();
System.out.println( "What is the maximum number of producers and consumers?" );
p_c_max = scanner.nextInt();


// Get the max and min values for foos.
System.out.println( "What is the minimum number of foos that can be produced or consumed?" );
foos_min = scanner.nextInt();
System.out.println( "What is the maximum number of food that can be produced or consumed?" );
foos_max = scanner.nextInt();


// Creating the Consumers and Producers Queue
Producer pQueue = new Producer( p_c_min, p_c_max, foos_min, foos_max );
Consumer cQueue = new Consumer( p_c_min, p_c_max, foos_min, foos_max );


// Code for Two Simulation Ticks
// Try block for exception handling
try
{

// Create a PrintWriter that writes to file "output.txt"
FileWriter fw = new FileWriter( "output.txt" );
PrintWriter pw = new PrintWriter( fw );


// Do this until "foos" = warehouse max
while ( wHouse.getFoos() != wHouse.getMax() )
{
// Creating random coin flip
int flip = generator.nextInt( 2 );

// If 0 then producer is added
if ( flip == 0 )
{
pQueue.produceFoo( wHouse );
}

// If 1 then consumer is added
if ( flip == 1 )
{
cQueue.consumeFoo( wHouse );
}
simTicks++;

// Print data to file
print( wHouse, pQueue, cQueue, pw );

}


// Do this until "foos" = 0
while ( wHouse.getFoos() != 0 )
{
// Creating random coin flip
int flip = generator.nextInt( 2 );

// If 0 then producer is added
if ( flip == 0 )
{
pQueue.produceFoo( wHouse );
}

// If 1 then consumer is added
if ( flip == 1 )
{
cQueue.consumeFoo( wHouse );
}
simTicks++;

// Print data to file
print( wHouse, pQueue, cQueue, pw );

}


// Do this until "foos" = warehouse max
while ( wHouse.getFoos() != wHouse.getMax() )
{
// Creating random coin flip
int flip = generator.nextInt( 2 );

// If 0 then producer is added
if ( flip == 0 )
{
pQueue.produceFoo( wHouse );
}

// If 1 then consumer is added
if ( flip == 1 )
{
cQueue.consumeFoo( wHouse );
}
simTicks++;

// Print data to file
print( wHouse, pQueue, cQueue, pw );
}


// Do this until "foos" = 0
while ( wHouse.getFoos() != 0 )
{
// Creating random coin flip
int flip = generator.nextInt( 2 );

// If 0 then producer is added
if ( flip == 0 )
{
pQueue.produceFoo( wHouse );
}

// If 1 then consumer is added
if ( flip == 1 )
{
cQueue.consumeFoo( wHouse );
}
simTicks++;

// Print data to file
print( wHouse, pQueue, cQueue, pw );

}

// Close the output file
pw.close();
}

// FileNotFoundException Handler
catch ( FileNotFoundException e )
{
System.out.println( "File Not Found Exception Error. " );
}

// IOException Handler
catch (IOException e)
{
System.out.println( "I/O Exception Error." );
}


System.out.println( "\n" );


// Recursion Call
System.out.println( "Please enter the number of base stars: " );
int stars = scanner.nextInt();
// Call the recursive function for Recursive Problem
recursion( stars, stars );

}

/*
* print() Prints output to an output file
* @param w Warehouse that contains information to be printed from
* @param p Producer that contains information to be printed from
* @param c Consumer that contains information to be printed from
*/
public static void print( Warehouse w, Producer p, Consumer c, PrintWriter pw )
{

// The data to be printed to the output file
pw.println( "The arrangement of Customers: " + c.toString() );
pw.println( "The arrangement of Producers: " + p.toString() );
pw.println( "The number of 'foos' in the Warehouse: " + w.getFoos() );
pw.println( "The number of 'foos' each Producer seeks to store: " + p.getNext() );
pw.println( "The number of 'foos' Consumer's requires: " + c.getNext() );
pw.println( "The total number of 'foos' Producers has stored: " + p.getProduced() );
pw.println( "The total number of 'foos' Consumers have consumed: " + c.getConsumed() );
pw.println( "The number of times Producers have entered Warehouse: " + p.getEntered() );
pw.println( "The number of time Consumers have entered Warehouse: " + c.getEntered() );
pw.println( "The number of Simulation Ticks: " + simTicks );
pw.println("");


}






// Recursion Method
/*
* recursion() Uses recursion to print out a sequence of stars
* @param n Number or rows and stars to be printed on each row
* @param max Total number of stars for the base line, needed for spacing
*/
public static void recursion( int n, int max )
{
//int max =

if ( n == 1 )
{
printing( 1, max );
}
else
{
recursion( n - 1, max);
printing( n, max );
}

}

/*
* printing() Uses spacing to print out formatted output
* @param n Number of stars to print out
* @param max Max number of characters, used for spacing
*/
public static void printing( int n, int max )
{
// Loops for iteration and spacing
int count = max - n;

for( int i = 0; i < count; i++ )
{
System.out.print( " " );
}

for( int i = 0; i < n; i++ )
{
System.out.print( "* " );
}

System.out.println();

}

}


At the end of the main class - test file there is a little bit of recursion. This is an add on onto the assignment, no problems here.

Share this post


Link to post
Share on other sites
Advertisement
Seems to be an excessive amount of copy and pasting - at a glance each of those classes look, for the most part, exactly the same. There is no error checking, so you are vulnerable to malicious (or just ignorant) input which, among other things, may cause your collections to throw. Even without bad input, your collections usage could use some work as you never check to see if they are empty. Your conditionals (if / else ) are inefficient, though technically correct. If you have a integer that's either 0 or 1 and it's not 0, then it's just 1. So there's no need to use a duplicate if, just use else.

While I would not say that you're a expert, I can't say I see the "you don't know how to program". Certain parts are dead on - such as the recursion - so it's not like you're a lost cause even if you did rip it from someone else. I can see his point about the random number generator; it would have been better to have passed it in to your classes instead of creating four of them. But, you know, there's nothing technically wrong with what you did. I've seen much worse.

The overall judgement, to me, depends on what standard you're expected to be held against. If this is for newbie land I don't see the big deal, all things considered. If this is for some kind of advanced course, however, then you've got some work to do.

Also, I got all the correct output for this program and it runs successfully every time I run it.[/quote]
Academia is nothing if not impractical. Not that your code isn't an eyesore but there is always something to be said for "getting it to work".

As you can see I'm a bit of a care bear unless it's a serious project =P.

Share this post


Link to post
Share on other sites
Well, injecting the random generator would be preferable. You cannot really unit test the code as it currently is. Unless perhaps he is wondering why you're using random values at all, did the requirements include them?

The code isn't great, but it isn't terrible either. The classes are quite coupled. The coding convention is inconsistent. The variable names are poor in places. There are some odd areas, such as altering the queue when printing it. Because you're calling remove, you're only printing the first element every time,

However, if you are fairly new then that is indeed harsh criticism. The fact that your instructor failed to make it clear to you what their objections were is more serious though. Saying "wrong" isn't very useful. Clearly indicating where mistakes were made and where improvements can be made is far more useful.

Correct output isn't often the aim in academia. Indeed, for some trivial programs you can produce the correct output without really learning anything. Some, or sometimes all, of the marks will come from how the project demonstrates your understanding. Imagine the classic program, often one of the first ones written, which requires that a diamond pattern is output onto the screen. The instructor intends that this be done using loops or recursion, but obviously one could obtain the correct output by hard-coding it.

Share this post


Link to post
Share on other sites
Yes, The requirements were to use random numbers. The only part of the program he really bashed me on was the random generator in my classes. I know I could code it different and generate the random values in say the main class, them pass them to the class. However, by the sounds of it he's not going to give me much credit at all for something I could fix in a matter of time.

Share this post


Link to post
Share on other sites
What did the assignment require?


and "I don't have any idea how to program"[/quote]
If that is what he said, then respond: "and you apparently don't have any idea how to teach me"

Share this post


Link to post
Share on other sites
Are you sure he wasn't referring to the fact that your Random instances aren't actually private. Maybe he assumed you were accessing them from other classes (variables lacking a specific access modifier default to "package" access, and it appears your classes belong to the same package).

Share this post


Link to post
Share on other sites
He was referring to me using the Random Instance at all. I know I should have made a separate method outside of the class and just passed it the value but according to him what I did is completely wrong and I am no good at programming. I don't know, I guess I was just wondering if it was "that" bad that I did this in the class.

Share this post


Link to post
Share on other sites
Got my grade back. I received a 50% on it and according to him my Recursion problem was completely wrong? I don't get it.

Share this post


Link to post
Share on other sites
I am curious, is there anyway you could post the assignment you were given?
Are you getting a degree in Information Technology and are taking a programming class as part of your requirements,
or are you getting a degree in software and are just starting to learn programming?

If it makes you feel any better, I once had a programming teacher that didn't even know how to program.
He actually told the class that you could overload a C++ function by only changing the return type. There were many
people in the class who raised their hands to question that statement, but he didn't even acknowledge us.

It was a terrible class. You could have a terrible instructor. Not everyone is good at teaching, and those who are lacking
seldom have the internal drive and motivation to improve.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!