Rv5 said:
I have an assignment due mid next week that I have completed. I was hoping
someone could take a look at the code and tell me what they think of the
style. Id like to know if this is good code that I can be proud of or if my
technique still needs some work. What can be improved? I created an htm
page that first lists the assignment, and under that is my code. I think
the code is good, but Ive thought that before...
http://www.69chargerrt.com/comp322.htm
In future, please post your source here, and skip the assignment question.
Either report a bug, syntax error, a design you can't achieve, or a request
about style.
Warning: So many kids ask this newsgroup "please do my homework for me" that
questions about homework should be phrased carefully.
#include <iostream>
#include <cstdlib>
#include <cstdio>
#include <string>
using namespace std;
That line defeats the purpose of namespaces. Write
using std::string;
etc. for each individual identifier you want to import. You'l find
surprisingly few of them.
int f; // max number of frames
That kind of comment is a "code smell" that the indentifier it comments
could have a better name. Try:
int max_number_of_frames;
int l; // length of reference string
string r; // reference string
Give variables the narrowest scope possible. These shouldn't be outside the
functions that use them.
int numfaults = 0; // number of page faults
char *frame; // frames that will contain the letters
int *counter; // counts letter time in the frame since last referenced
Students should start with STL and container classes like std::vector. They
should not need pointers for the first several assignments. Read
/Accelerated C++/ to learn these features in the right order.
bool analyzeFrames(int i)
{
for (int k = 0; k < f; k++)
{
// first trys to find the page in the frame
if (frame[k] == r)
{
This function is too long. A good place to break it is one of the inner
controlled block. Everything this 'if' controls could be inside a subsidiary
function.
counter[k] = 1; // finds the letter, resets its counter
for (int l = k+1; l < f; l++) // loaded pages time in frame continues
counter[l]++; // increment other page counters
return true;
}
// if the page is not loaded, see if there is a blank frame to load it in
else if (frame[k] == '-')
{
frame[k] = r; // fill the empty cell with next page
counter[k] = 1; // set that page counter to 1
numfaults++; // page was not in the frame, so a page fault occurs
return true;
}
else
counter[k]++; // increments counter, page not used this time
}
numfaults++; // page not found and could not be loaded, page fault occurs
return false;
}
void replace(int i)
{
// largest counter index is the frame that gets replaced
int largest = counter[0];
int index = 0;
// loop through counter array
for (int k = 1; k < f; k++)
if (counter[k] > largest) //finds the largest counter
{
largest = counter[k];
index = k;
}
frame[index] = r; // replaces least recent page with new one
counter[index] = 1; // sets new page counter to 1
}
int main()
{
// user enters values of variables
This function is also way too long. The first block of it, the input stuff,
could be inside a function called user_enters_values_of_variables().
cout << "Input maximum number of frames (f): ";
cin >> f;
cout << "Input length of reference string (0 < l < 101): ";
cin >> l;
cout << "Input reference string (r): ";
cin >> r;
Duplicated code (like this cout-cin sequence) is a sign there could be a
function. In this case, the function could call like this:
f = getValue("Input maximum number of frames (f): ");
// displays the values the user entered
cout << "f = " << f << endl;
cout << "l = " << l << endl;
cout << "r = " << r << endl;
// creates the proper number of cells in the frame and counter arrays
frame = new char[f];
counter = new int[f];
Nobody deletes these. Use std::vector (even if your professor did not cover
them yet). If in the rare chance you actually need an array, use 'delete[]
frame' to release the storage when you are done with it.
// fills the frame and counter arrays with '-' and 0 respectively
for (int i = 0; i < f; i++)
{
frame = '-';
counter = 0;
cout << frame; // verifies that the frame is empty
}
cout << endl;
// main loop that is going through the reference string
for (int i = 0; i < l; i++)
{
if(!analyzeFrames(i)) // attempts to find page or blank cell to load it to
{
replace(i); // not found, no empty cells, replace least recent page
}
// displays contents of frame as the program runs
for (int l = 0; l < f; l++)
cout << frame[l];
cout << endl;
}
cout << "Total page faults = " << numfaults << endl;
return 0;
}
Like I used to say, as a lab aide, when finishing attending to a student:
"Good luck!"
