Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • 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 here.

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

  • _tmain() should just be main(); the former does not exist in C++ _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.

  • 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.

  • 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.

added 356 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

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

Inside your class declaration, youYou 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.:

(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.)

  • 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()_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.

Instead of using setters, you should do this after getting the input:

Inside your class declaration, you can use an initializer list:

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.

  • 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().

  • 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)
     {
     return EXIT_FAILURE;
     }
    

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

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

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:

(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.)

  • 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.

added 182 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

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 do this 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.

Inside your class declaration, you can use an initializer listinitializer list:

X(int num_1, int num_2, int num_3, int num_4)
 : num_1(num_1)
 , num_2(num2num_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";
}

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.

  • t_main_tmain() should just be main(). You

  • You could also 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)
     {
     return EXIT_FAILURE;
     }
    

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

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 do this 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.

Inside your class declaration, you can use an initializer list:

X(int num_1, int num_2, int num_3, int num_4)
 : num_1(num_1)
 , num_2(num2)
 , 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";
}

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

testObject.display();

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.

  • t_main() should just be main(). You could also leave out the command line parameters if you're not going to use them.

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 do this 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.

Inside your class declaration, you can use an initializer list:

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";
}

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().

  • 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)
     {
     return EXIT_FAILURE;
     }
    

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

Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /