C++

Forum

 
Looking for a cleaner way.

lare26 (144)
I am still learning the very basics of programming in C++. I am asking if anyone is willing to tell me if and possibly how I can make the following code better.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
 
#include <iostream>

using namespace std;

class C_main
{
    public:
    void set_pwr(bool status);
    bool get_pwr(void);
    void set_cam(bool status);
    bool get_cam(void);
    void set_motion(bool status);
    bool get_motion(void);
    private:
    bool pwr;
    bool cam;
    bool motion;
}one, two;

void check(bool status)
{
    if(status)
    {cout << "on!";}
    if(!status)
    {cout << "off!";}
    cout << endl;
}

int main()
{
    cout << "Hello world!" << endl;
    cout << "....................................." << "Checking status of panel one!" << endl;
    one.set_pwr(true);
    one.set_cam(true);
    one.set_motion(true);
    one.set_cam(false);
    one.set_motion(false);
    one.set_pwr(false);
    cout << "....................................." << "Checking status of panel one!" << endl;
    two.set_pwr(true);
    two.set_cam(true);
    two.set_motion(true);
    two.set_cam(false);
    two.set_motion(false);
    two.set_pwr(false);
    cout << "....................................." << "Status checks complete!" << endl;
    return 0;
}

void C_main::set_pwr(bool status)
{
    pwr = status;
    cout << "Power is ";
    check(status);
    }

bool C_main::get_pwr(void)
{return pwr;}

void C_main::set_cam(bool status)
{
    cam = status;
    cout << "Cameras are ";
    check(status);
    }

bool C_main::get_cam(void)
{return cam;}

void C_main::set_motion(bool status)
{
    motion = status;
    cout << "Motion sensors are ";
    check(status);
    }

bool C_main::get_motion(void)
{return motion;}


I am not in a class. I just want to improve my skills little by little. Thanks ahead of time for any help. And please remember that I am a beginner's beginner. :o)
Last edited on
Galik (2254)
On line 18 you define two global variables one and two. Its best to avoid defining variable globally.

lare26 (144)
Yeah. Those do usually give me trouble when I declare and define classes in a seperate source file and header file. Thanks. Is there something I can do to make my code "slimmer"?
Duthomhas (13206)
Your code first sets all attributes to true, then immediately sets all attributes to false. Does this code have side effects we are unaware of?

A constructor will help:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
class C_main
{
public:
    C_main( bool power = false, bool cam = true, bool motion = true ):  // default arguments
        pwr( power ), cam( cam ), motion( motion )  // initialize private fields with the argument values
        { }
    ...
};
...
int main()
{
    C_main one( true, true, true ); // etc.
    ...
    return 0;
}
(Notice how I did not indent the public and private words, it makes it easy to glance and see which section is which.)

The other option is to make your setters return *this, so that you can chain them:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
 
class C_main
{
public:
    ...
    C_main& set_pwr(bool);
    //etc
...
};

int main()
{
    C_main one;
    one.set_pwr( true ).set_cam( true ).set_motion( true );
    ...
    return 0;
}

C_main& C_main::set_pwr(bool status)
{
    ...
    return *this;
}


Finally, you should make your class "const correct".
1
2
3
4
5
6
7
8
9
10
11
12
 
class C_main
{
public:
    void set_pwr(bool status);
    bool get_pwr(void) const;  // this does not change the object
    void set_cam(bool status);
    bool get_cam(void) const;  // this does not change the object
    void set_motion(bool status);
    bool get_motion(void) const;  // this does not change the object
private:
    ...
};
This assumes, of course, that the getters really do not change the object.

Hope this helps.
lare26 (144)
Thanks Duoas. I loved all your ideas and it tought me a few things. Okay...a lot of things. This is my new code. What do you guys think of this?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
 
#include <iostream>

using namespace std;

class C_main
{
public:
    C_main( bool power = false, bool cam = false, bool motion = false ):  // default arguments
        pwr(power), cam(cam), motion(motion)  // initialize private fields with the argument values
        { }
    ~C_main(){}
    C_main& set_pwr(bool);
    bool get_pwr(void) const;
    C_main& set_cam(bool status);
    bool get_cam(void) const;
    C_main& set_motion(bool status);
    bool get_motion(void) const;
    void check(bool status);
private:
    bool pwr;
    bool cam;
    bool motion;
};

int main()
{
    cout << "Start system check?" << endl;
    char resp = 'n';
    cin >> resp;
    cout << endl;

    if(resp == 'y')
    {
    C_main one(true, true, true), two(true, true, true);
    cout << "....................................." << "System check: panel one started!" << endl;
    one.set_pwr(true).set_cam(true).set_cam(false)
    .set_motion(true).set_motion(false).set_pwr(false);
    cout << "....................................." << "System check of panel one is complete!" << endl;
    cout << "....................................." << "System check: panel two started!" << endl;
    two.set_pwr(true).set_cam(true).set_cam(false)
    .set_motion(true).set_motion(false).set_pwr(false);
    cout << "....................................." << "System check of panel two is complete!" << endl;
    }
    if(resp == 'n')
    {return 0;}
}

C_main& C_main::set_pwr(bool status)
{
    pwr = status;
    cout << "Power is ";
    check(status);

    return *this;
    }

bool C_main::get_pwr(void) const
{return pwr;}

C_main& C_main::set_cam(bool status)
{
    cam = status;
    cout << "Cameras are ";
    check(status);

    return *this;
    }

bool C_main::get_cam(void) const
{return cam;}

C_main& C_main::set_motion(bool status)
{
    motion = status;
    cout << "Motion sensors are ";
    check(status);

    return *this;
    }

bool C_main::get_motion(void) const
{return motion;}

void C_main::check(bool status)
{
    if(status)
    {cout << "on!";}
    if(!status)
    {cout << "off!";}
    cout << endl;
}


Not amazingly, I had a blast refining this. I'm looking forward to other ideas. :o)

P.S. What is the best free way to do a gui version of this. I would like to start working on that too. I am using Code::Blocks at the moment. Thanks ahead of time.
Last edited on
Topic archived. No new replies allowed.