current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

I'm brand new to programming (in general) and C++ (in particular). I'm learning the basics of classes and objects.

I'm trying to do the following:

  1. Create a class called X. It has 4 member functions, with 0 arguments, 1 argument, 2 arguments and 3 arguments, respectively.

  2. In main(), make an object of this X class that calls each of these 4 member functions.

  3. Modify the class so that it has a single member function with all of the arguments defaulted.

#include "stdafx.h"
#include <iostream>
#include <cmath>
using namespace std;

class X
{
public:

    void setNum_1(int);
    void setNum_2(int);
    void setNum_3(int);
    void setNum_4(int);

    double getNum_1();
    double getNum_2(int num_1);
    double getNum_3(int num_1, int num_2);
    double getNum_4(int num_1, int num_2, int num_3);

private:
    int num_1;
    int num_2;
    int num_3;
    int num_4;
};

int _tmain(int argc, _TCHAR* argv[])
{
    X testObject;

    int lNum_1 = 0;
    int lNum_2 = 0;
    int lNum_3 = 0;
    int lNum_4 = 0;

    cout << endl;
    cout << "Please enter an integer: ";
    cin >> lNum_1;

    cout << "Please enter an integer: ";
    cin >> lNum_2;

    cout << "Please enter an integer: ";
    cin >> lNum_3;

    cout << "Please enter an integer: ";
    cin >> lNum_4;

    testObject.setNum_1(lNum_1);
    testObject.setNum_2(lNum_2);
    testObject.setNum_3(lNum_3);
    testObject.setNum_4(lNum_4);

    cout << endl;
    cout << "The 1st number returned is: " << testObject.getNum_1() << endl;
    cout << "The 2nd number returned is: " << testObject.getNum_2(lNum_1) << endl;
    cout << "The 3rd number returned is: " << testObject.getNum_3(lNum_1, lNum_2) << endl;
    cout << "The 4th number returned is: " << testObject.getNum_4(lNum_1, lNum_2, lNum_3) << endl;
    cout << endl;

    return 0;
}

void X::setNum_1(int n_1)
{
    num_1 = n_1;
}

void X::setNum_2(int n_2)
{
    num_2 = n_2;
}

void X::setNum_3(int n_3)
{
    num_3 = n_3;
}

void X::setNum_4(int n_4)
{
    num_4 = n_4;
}

double X::getNum_1()
{
    return num_1;
}

double X::getNum_2(int num_1)
{
    return num_1 + num_1;
}

double X::getNum_3(int num_1, int num_2)
{
    return num_1 + num_2;
}

double X::getNum_4(int num_1, int num_2, int num_3)
{
    return num_1 + num_2 + num_3;
}

I was wondering if you can recommend some ways that I can improve this code (or let me know if I've totally botched something, which is likely the case!).

share|improve this question

1 Answer 1

up vote 6 down vote accepted

First of all, you should avoid "getters" and "setters" whenever possible. This is because they can be bad for encapsulation as they reveal the internal members. This would be an ideal example if you were merely trying to understand getters and setters, but it's still not the best approach to learning how classes should work, especially when you start using them for more complex programs.

You should become aware of object construction. That is, when you first create an object to hold some data at the start, it should be given to that object upon construction.

Instead of using setters, you should construct the object after getting the input:

X testObject(lNum_1, lNum_2, lNum_3, lNum_4);

This will construct the object right away with the initial values. However, you'll first need to provide a constructor that will handle this.

You can use an initializer list:

X::X(int num_1, int num_2, int num_3, int num_4)
    : num_1(num_1)
    , num_2(num_2)
    , num_3(num_3)
    , num_4(num_4)
{}

As for the getter, it would only be useful if you need the value. If you just need to display the members, then you can create a display() function:

void X::display() const
{
    std::cout << "num_1: " << num_1 << "\n";
    std::cout << "num_2: " << num_2 << "\n";
    std::cout << "num_3: " << num_3 << "\n";
    std::cout << "num_4: " << num_4 << "\n";
}

(Notice that I've made this function const. Since it only displays data members and doesn't modify them, adding this keyword will prevent any accidental modifications.)

It would then be called by itself, rather than calling the getter for each member:

testObject.display();

There's much that can be said about all of this, but I've given you a basic idea on where to start.

Miscellaneous:

  • Try not to get into the habit of using using namespace std. If you start writing larger programs, you may run into name-clashing issues if this is misused. More info about that here.

  • Since you're not using <cmath>, it should just be removed.

  • _tmain() should just be main(); the former does not exist in C++.

  • You could leave out the command line parameters if you're not going to use them.

    Otherwise, you could certainly replace your input with command line arguments:

    ./program_name 1 2 3 4

    If you do this, you should also be sure to check the value of argc so that the program won't try to run with too few arguments:

    if (argc < 5)
{
    std::cerr << "You must input four numbers!";
    return EXIT_FAILURE;
}

    The program name is counted as an argument, so it should be 5.

share|improve this answer
    
thank you for these great suggestions. Is there an optimal way to modify the class so that it has a single member function with all of the arguments defaulted? –  user3814362 Jul 10 '14 at 15:12
    
@user3814362: You can provide an initializer list (no arguments) that can set the data members to default values. Is that what you mean? Also, please be sure to upvote this answer if you've found it helpful, and you may also accept it if it has helped you a lot. –  Jamal♦ Jul 10 '14 at 15:16

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.