Re: Code Critique Please
From: Phlip (phlip_cpp_at_yahoo.com)
Date: 11/16/03
- Next message: P.J. Plauger: "Re: file creation problem in Windows using fstream"
- Previous message: Deming He: "Re: Virtual fnc Vs Pure fnc?"
- In reply to: Rv5: "Code Critique Please"
- Next in thread: Benny Hill: "Re: Code Critique Please"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Sun, 16 Nov 2003 06:28:40 GMT
Rv5 wrote:
> 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[i])
> {
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[i]; // 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[i]; // 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[i] = '-';
> counter[i] = 0;
> cout << frame[i]; // 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!"
-- Phlip
- Next message: P.J. Plauger: "Re: file creation problem in Windows using fstream"
- Previous message: Deming He: "Re: Virtual fnc Vs Pure fnc?"
- In reply to: Rv5: "Code Critique Please"
- Next in thread: Benny Hill: "Re: Code Critique Please"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
