Velocity Reviews

Velocity Reviews (http://www.velocityreviews.com/forums/index.php)
-   C++ (http://www.velocityreviews.com/forums/f39-c.html)
-   -   Code Critique Please (http://www.velocityreviews.com/forums/t279258-code-critique-please.html)

Rv5 11-16-2003 12:20 AM

Code Critique Please
 
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

Thanks
Rv5



Rv5 11-16-2003 12:24 AM

Re: Code Critique Please
 
One thing I forgot to mention is that I need not be concerned with input
errors. The teacher said not to bother programming any catches and such to
trap input errors, so please keep that in mind when viewing.

Thanks
Rv5



Phlip 11-16-2003 06:28 AM

Re: Code Critique Please
 
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



Benny Hill 11-16-2003 03:25 PM

Re: Code Critique Please
 
On Sat, 15 Nov 2003 16:20:33 -0800, Rv5 wrote:

> http://www.69chargerrt.com/comp322.htm
>
> Thanks
> Rv5


This URL is not accessible. I get the error message:

"Access to the port number given has been disabled for security reasons"


--
Benny
Remove your rose colored glasses before e-mailing me



All times are GMT. The time now is 06:12 AM.

Powered by vBulletin®. Copyright ©2000 - 2014, vBulletin Solutions, Inc.
SEO by vBSEO ©2010, Crawlability, Inc.