C++ Styling Tips

[jdgregson]

I just started messing around with C++ yesterday, and now I think I should get some input on how to style my code. So if you have any input, please feel free to give it. I think I'm over commenting for one thing...

My code:

#include <fstream>
#include <iostream>
#include <stdlib.h>

/**************************

         jgregson 
        3rd project
    written on my first
    and second day of 
        C++ coding
    02-01-11 - 02-02-11
 
 http://www.jdgregson.com 
**************************/
	
using namespace std;
// functions

int newUser() {
// variables for newUser()
  char uname[30];
  char passwd[30];
  char home[30];
// getting new user information
  cout<<"username: ";
  cin>>uname;
  cout<<"password: ";
  cin>>passwd;
  cout<<"Users home directory: ";
  cin>>home;
// adding new user
  ofstream write("db.txt", ios::app);
  ofstream pwd("pwdb.txt", ios::app);
  write<<uname<<":x:"<<home<<"|||";
// this part is supposed to convert a plain text password
// into its ASCII value, but it doesn't work right.
// all passwords, whether 1 or 15 characters, are writing
// to pwdb.txt as "2359040"
  int pw=(int)passwd;
  pwd<<uname<<":"<<pw<<"|||"; 
  return 0;
}

int listUsers() {
// variables
  char list[100];
// read list from database file
  ifstream read;
  read.open("db.txt");
  read>>list;
  cout<<list;   
  return 0;
}

// MAIN
int main() {
// declaring variables for main
// I'm over-commenting 
int action;

// main output (instructions)
  cout<<"1 = list users\n";
  cout<<"2 = new user\n";
  cout<<"(1/2): ";
         // get input from user
  cin>>action;
  cin.ignore();
        // prevent invalid variable usage
    if (action > 2 || action < 1) {
      cout<<"Error: "<<action<<" is not a valid switch";
      return 1;
    }
// switches for main   
  switch(action) {
    case 1:  
             listUsers();
             break;
    case 2:  
             newUser();
             break;
    default: 
             cout<<"ERROR: unknown error";
             return 1;
  }   
  return 0;
}

[PanGalactic]

I think you are doing very well using functions that are a reasonable size. And I like your function naming conventions. For someone who is in their second day of C++ programming you are doing extremely well.

I have a few recommendations for you:

17: Avoid "using namespace std;". Prefix your std namespace items with std:: or, within function and class scope, use using declarations (e.g. "using std::string;"). Here's why: if you ever decided to use the std::list container to store user information in this program, std::list (now just "list") would conflict with the variable declared on line 47.

20: Document the purpose of the function, its parameters, return values, preconditions, post-conditions and side effects. You may know what the function does today, but you won't a month from now. And what it is supposed to do is a lot more important than how it does it. Your instincts are correct. I think you are over-documenting the how. You can see the "how" by reading the code. You can never determine "why" that way. That's what really needs to be documented in comments.

22-24: Avoid fixed-length character arrays, and never used them for unbounded input. Prefer std::string instead.

26-31: I prefer more whitespace, especially around operators: cout << "username: ;"

27,29,31: Never used fixed-length buffers for unbounded input. It is the surest way to crash a program.

33,34,35,41: Check the results of your IO operations. You can't tell if the I/O operations really worked or not.

In general, I recommend a more consistent indentation style. Your code and comments, especially in main(), seem to vary quite a bit.

Line 40 is using a cast to convert a character pointer to an integer. I'm not sure what exactly you want to do here.

[jdgregson]

I'm not sure how to use std::string It looks like it works the same way that char does, but I'm not sure how to work with it.

on line 40 I was basically trying to change each character of the the plain-text data (password, in this case) that the user enters into its ASCII value. So, for example, if they entered 'abc' as their password, it would convert it to '979899', or '97 98 99', and write it to the pwdb.txt file. It was an attempt to create some sort of password hash for the password entered, although, simply converting a password to its ASCII value is no secure way to run things. But instead of doing what I wanted, it just changed it to '2359040' no matter what was entered.

[3hM2Nwbp]

I have to agree with PanGalactic regarding your public interface. It's one of my biggest pet peeves when abbreviations are abused to the point where they make code unreadable.

void newUser(...); //good
void nw_usr(...); //bad 

[Duthomhas]

I disagree about the "using namespace std" bit. (There is nothing wrong with it here, and saying otherwise is a matter of opinion. Opinions vary on the usefulness of "using" statements, but what really matters is where you must not use them -- and that is a lesson for another post [or at least another response in this thread].)


You cannot change a string type to an integer type with a simple cast.

  char passwd[30];
  ...

  int pw=(int)passwd;
  ...

Line 40 takes the address of your character array, casts it to an int, and assigns the result to the variable pw...

The input stream can be used directly to get an integer from the user:

  int usersPassword;
  ...

  cout<<"password: ";
  cin>>usersPassword;

This code assumes that the user does input only a valid number... You will have to learn how to validate the user's input, but again that is a future lesson.

[edit]
I just reread your second post... An array is a list of numbers, as char is an integer type. The computer just displays it differently. You can convert to a numeric array easily enough using the STL string and vector types:

vector <int> getUsersPassword()
  {
  string s;  // the string to store the user's character input
  cout << "password: ";
  getline( s );
  return vector <int> ( s.begin(), s.end() );  // copy it to a list of integers using a fancy constructor
  }

  vector <int> usersPassword = getUsersPassword();

[/edit]


I would note that while you are naming variables better than the usual new C++er, you are using too many variables to represent the same thing, and/or using the same name to represent different things. Look at the password stuff again:

  char passwd[30];
  ...

  ofstream pwd("pwdb.txt", ios::app);
  ...

  int pw=(int)passwd;
  ...

The variables on lines 23 and 40 both represent the same data. There is no need to keep both around. The variable on line 34 declares a similarly named but completely different thing. Notice how many times you are trying to find variations on the theme of "password" to keep all your variables.

How about just "usersPassword" and "passwordsFile", or something like that?

Also watch for specifically vague names, like "write". That's a nice verb, but it doesn't tell you anything about what you are writing to... Be explicit!


On line 71 you say "Error: foo is not a valid switch". Be careful not to tell your users about the inner working of your program. Yes, I know that in this case you are your own user, but it would have been better to simply say "Error: foo is not valid" or "Error: foo is not a valid choice" or something like that.


Finally, watch out for redundancies. Your switch statement handles all possibilities quite nicely, including invalid ones, but you also check for invalid possibilities on lines 70 through 73. Why not just put the stuff on lines 71 through 72 in the default case? That way if more options ever become possible (or if they otherwise change), then you don't need to synchronize more than two things: the textual prompt to the user and the switch cases.

Hope this helps.

[pwnedu46]

You could modify this code a little for the encryption and decryption.

  http://www.cplusplus.com/files/encrypt.zip 

Also, I rewrote it, reusing some of your code to fix a bug or two I found, and commented on things I changed.

#include <iostream>
#include <fstream>
#include <cstdlib> // aka stdlib.h
#define EXIT false // always true for the do-while loop.
using namespace std;

inline void pauseconsole() // better than system("Pause")
{
    std::cout << "\nPress ENTER to continue... " << flush;
    std::cin.ignore( std::numeric_limits <std::streamsize> ::max(), '\n' );
}



void newUser(){
    char username[30];
    char pass[30];
    char home[30];
    
    cout << "Username: ";
    cin >> username;
    cout << "Password: ";
    cin >> pass;
    cout << "Home: ";
    cin >> home;
    
    ofstream write("dbase.txt", ios::app);
    ofstream pwd("ascii_convert.txt", ios::app);
    write<<username<<":x:"<<home<<"\n";
  
    int pw=(int)pass;
  pwd<<username<<":"<<pw<<"\n"; 
}


void listUsers() {
// variables
  char list[100];
// read list from database file
  ifstream read;
  read.open("dbase.txt");
  read>>list;
  cout<<list;
  pauseconsole(); // before it would just display for a fraction of a second and close the console. This gives user time to read it.
}

int main() {
    int action;
    
    do{
        system("CLS"); // Yeah, i know. system(blah) is bad. I'm using it anyway.
        cout<<"1 = list users\n";
        cout<<"2 = new user\n";
        cout<<"3 = exit program\n"; // new option. Self explanitory.
        cout<<"(1|2|3): ";
       
        cin>>action;
        cin.ignore();
        
        switch(action) {
            case 1:
                listUsers();
                break;
            case 2:
                newUser();
                break;
            case 3:
                return 0; // I decided return 0; is better than abort();.
                break;
            default:
                cout << "ERROR: INVALD INPUT! \n"; // notice the if statement is gone because of redundancy, as stated in other posts.
                return 1; // Experiment with try/throw/catch blocks. Console shutdown isn't always necessary. Again, better than abort();. 
        } // end switch
    } while (EXIT == false); // end do-while infinite loop. A habit from lua scripting. (while true do --code end) 
    return 0;
}

Edit: Commenting is a good thing. There's no such thing as 'over commenting.'
(within reason, for those that take things too literally. 100 lines of

// hi 
// hi
...

is 'over commenting.'
/edit

Hope it helps.

[jdgregson]

Thank you all for the input and advice, it is quite helpful! I'll have to read all of these again later when I have more free time. Thanks again.