Code Critique Please

Discussion in 'C++' started by Rv5, Nov 16, 2003.

  1. Rv5

    Rv5 Guest

    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, Nov 16, 2003
    #1
    1. Advertising

  2. Rv5

    Rv5 Guest

    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
    Rv5, Nov 16, 2003
    #2
    1. Advertising

  3. Rv5

    Phlip Guest

    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)
    > {


    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!"

    --
    Phlip
    Phlip, Nov 16, 2003
    #3
  4. Rv5

    Benny Hill Guest

    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
    Benny Hill, Nov 16, 2003
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Michael Strorm
    Replies:
    26
    Views:
    752
    J. Campbell
    Nov 10, 2003
  2. gorda
    Replies:
    16
    Views:
    761
    Karl Heinz Buchegger
    Jul 29, 2004
  3. Adrian

    Code critique please

    Adrian, Oct 30, 2004, in forum: C++
    Replies:
    2
    Views:
    338
    Adrian
    Oct 31, 2004
  4. Rv5

    Code Critique Please

    Rv5, Nov 16, 2003, in forum: C Programming
    Replies:
    2
    Views:
    325
    -berlin.de
    Nov 16, 2003
  5. Brian Blais

    critique my code, please

    Brian Blais, Feb 6, 2006, in forum: Python
    Replies:
    3
    Views:
    325
    Frithiof Andreas Jensen
    Feb 8, 2006
Loading...

Share This Page