Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a newbie to C++ programming and am currently reading this book called Jumping to C++ by Alex Allain. I have finished the pointers chapter and I am doing the exercises at the end of the chapter.

The following paragraphs are the exercises and included are my own solutions:

Write a function that prompts the user to enter his or her first name and last name, as two separate values. This function should return both values to the caller via additional pointer (or reference) parameters that are passed to the function. Try doing this first with pointers and then with references. (Hint: the function signature will look be similar to the swap function from earlier!)

// pointersExercise01.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "iostream"
#include "string"

using namespace std;

void printName(string *pointFirstName, string *pointLastName){

    cout << endl << "Hello " << *pointFirstName << " " << *pointLastName;
}

void printName(string &pointFirstName, string &pointLastName){
    cout << endl << "Hi " << pointFirstName << " " << pointLastName;
}


int _tmain(int argc, _TCHAR* argv[])
{
    string firstName, lastName;

    cout << "Enter First Name ";
    cin >> firstName;
    cout << endl << "Enter Last Name ";
    cin >> lastName;

    /*FUNCTION POINTERS*/
    printName(&firstName, &lastName); // print out using pointers

    /*REFERENCE POINTERS*/
    printName(firstName, lastName);

    system("pause");
    return 0;
}

The next exercise is to modify the program I wrote for exercise 1 so that instead of always prompting the user for a last name, it does so only if the caller passes in a NULL pointer for the last name.

// nullPointerExercise3.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "iostream"
#include "string"

using namespace std;

void userNames(string *pointerFirstName, string *pointerLastName){

    string firstN = *pointerFirstName;
    string LastN = *pointerLastName;
    bool flag;

    do{
        if(LastN == "NULL" ){
            cout << "Again what is your last name? ";
            cin >> LastN;

            flag = true; 
        }else{
            flag = false;
        }

    }while(flag != false);
}

int _tmain(int argc, _TCHAR* argv[])
{
    string firstName, lastName;

    cout << "FIRSTNAME: ";
    cin >> firstName;
    cout << "\n" << "LASTNAME: ";
    cin >> lastName;

    userNames(&firstName, &lastName);

    system("pause");
    return 0;
}

If possible, I'd like to know if I did right for both of the exercise and if I am in the right direction. Comments and tips will be helpful.

share|improve this question
1  
You are not implementing pointers, you are merely using them... –  K-ballo Jan 5 '13 at 22:10
    
Hi thanks for the comment, I don't quite get what your saying. can you please point out which are the flaws of my solutions, it will help me a lot to understand pointers much better. –  Rojee Jan 5 '13 at 22:16
add comment

2 Answers

up vote 2 down vote accepted

I'm not sure whether this should be reviewed yet or not. I can't decide what critiques are things that you haven't learned yet but will, and things that you haven't learned yet and might not. (Also, there's a handful of errors that should have been found with basic testing.)

Anyway here's a few things that jumped out at me:

  • There's no notion of const correctness at all in your code
  • system("pause") is bad -- it's slow, and implementation dependent
    • if you want to see the output, run it in a shell directly instead of using system("pause") as a hack
  • When pointerLastName is NULL, string LastN = *pointerLastName; is undefined behavior and will probably crash.
  • if(LastN == "NULL" ) makes no sense
    • a NULL pointer being dereferences does not turn into the string "NULL" -- it crashes (typically)
  • Your do {} while() loop in the second program is broken
  • using namespace std; pollutes the global namespace. It's typically better to just qualify it directly like std::string or import specific things using std::string;.
    • In small, simple programs, it tends to not matter. It can become a bad habit though.
    • #include "iostream" should be #include <iostream> -- I'm actually a bit surprised that compiles.

Also, for what it's worth, I wouldn't use pointers in either of these two programs. References are a better choice. Obviously this is a learning exercise though, so this critique doesn't really apply.


Based on our comments back and forth, there seems to be some confusion surrounding pointers. I'm not sure if it's me misunderstanding what your saying, or a misunderstanding of pointers, but either way, I figure I'll elaborate a bit.

if(LastN == "NULL" )

That is checking if LastN is equal to the string "NULL". That is not the same thing as checking for a NULL pointer:

string* pNullStr = NULL;
string nullStr = *pNullStr; //undefined behavior (almost certainly a crash)

The only way a string is going to have a value of NULL is if it is actually set to NULL:

string ns = "NULL";
if (ns == "NULL") {
    //this will happen
}

In other words, your userNames function isn't prompting the user for a last name only "if the caller passes in a NULL pointer for the last name" (as your instructions say). What it's doing is prompting for a last name only if the caller passes in a string (not a pointer to a string) equivalent to "NULL".

Based on your code, the only way a last name is going to be prompted for is if you type "NULL" in when prompted for a last name (in the main code).

Perhaps this was by design, but it directly contradicts your exercise instructions.


Also, while I'm at it with the editing, if you wanted to be really careful, you could check all of the input operations. In fact, really you should always check, but in a program this simple, it would be a bit of a pain.

To clarify:

cout << "FIRSTNAME: ";
cin >> firstName;

That can fail. What if the user writes EOF or if the user pipes in an empty file? That means the read is going to fail.

if (!(std::cin >> firstName)) {
    std::cerr << "Fatal error: reading last name failed";
    return EXIT_FAILURE; //(defined in cstdlib)
}

On a stylistic note, not sure why I didn't spot this before: if your loop in userNames is actually what you meant it to be, you can write it much more cleanly:

while (lastN == "NULL") {
    std::cout << "Again what is your last name? ";
    std::cin >> lastN; //Once again, this should be checked
}
share|improve this answer
    
Hi thanks for the answer I'm using Visual C++ 2012, I notice that if I do not put in the 'system("pause")' when I compile it the shell itself run for a 1 second then exits, very hard to see the output. I'd like to know why my do while loop broken. –  Rojee Jan 5 '13 at 22:33
    
@Rojee You can navigate to the folder in a cmd prompt shell to execute it, and you can see the output without using pause. There may also be some kind of VS feature. Not sure. You might find: stackoverflow.com/questions/1107705/systempause-why-is-it-wrong helpful. As for why the do-while loop is wrong: it's more of the entire function is wrong. If pointerLastName is NULL as the exercise said, that function will crash. It actually won't ever be NULL though since the way it's called in main. (Or did you interpret the exercise to mean the string "NULL"? Maybe we read that differently.) –  Corbin Jan 5 '13 at 22:49
    
It says in the book that you should always initialize pointers or else the program crashes is it same as what your saying about NULL, I put in that NULL to test if the do while will actually work and am I reading an old tutorial or are there any newer versions out there which has new techniques and implementations. –  Rojee Jan 5 '13 at 22:58
    
It's perfectly fine from a technical perspective to not initialize a pointer. You just can't dereference a pointer that doesn't point to memory the program owns (and an unitialized pointer has an extremely low chance of that actaully magically working -- and non-determinism tends to be bad). From a best-practices point of view, it is widely standard to initialize a pointer to a known value (either NULL or to actually point to something). The issues are vaguely related, but not the same. Are you trying to test for a NULL pointer, or a string containing "NULL"? –  Corbin Jan 5 '13 at 23:02
    
yes I'm trying to test a string containing NULL, I guess I need to read that chapter again so I can understand it much better, so far my knowledge about C++ isn't enough I need to learn it for my final project about network programming. thanks man. –  Rojee Jan 5 '13 at 23:32
show 6 more comments

You haven't done what the first exercise asked. It asks you to write a function that prompts the user for the name. You have written a function that prints the user's name. Effectively, you have done the opposite of what you were asked to do. As a result you've missed the point of the excersize.

The second one does what the excersize says, but the excersize did not ask you to keep asking. To simply follow the excersize you should not have a loop.

share|improve this answer
    
thanks man, next time I should read and understand the question much carefully. –  Rojee Jan 5 '13 at 23:41
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.