Am I designing this right ? I feel like it is extremely hairy

Question

This is a question about the quality of the approach. I am attempting to get better at writing nicer code, however I can't tell if certain things are good to do / bad to do sometimes.

Here is the project I am currently working on. I can't post the whole thing as it is kinda big, however I'll post snippets and explain my thoughts.

The goal for this project was to be as modular as possible. This means that once I write the main part of it, my idea is that if I ever wanted to add functionality, I would write a new file/files (I call them modules) and put it in a directory, and it would be implemented in the program upon compilation.

The main program functions like this. It stores a list of Base * objects. the Base class consists of only virtual functions, the idea is that any module I want to add MUST inherit from the Base class. All the modules will have render() give_input() etc...

Then the base class simply renders everything in this big list. Here is the header file of the main class (STUSURF)

#ifndef STUSURF_H
#define STUSURF_H

#include "common.h"
#include "headers.h"

// this is the main class. It will contain all the objects we need. This is the class we will interface with in main. 
// It will deal with the other data types for us.
class STUSURF{
public:
    char * path_start_screen;
    FILE * file_start_screen;

    int * width;
    int * height;

    //store a list of pointers that point to the base class. 
    // This class is guaranteed to have the functions we have such as render() and the input function. This is defined in the file common.h
    Base * * main_list;
    int main_list_len;

    // constructor. Takes in the path of the file that the program will start to display from. If file does not exist it will create it. 
    STUSURF( char * _start );

    // setting the program up
    void give_window_size( int * _w, int * _h );

    void mouse_press( int _button, int _state, int _x, int _y );
    void mouse_move_passive( int _x, int _y );
    void mouse_move_active( int _x, int _y );
    void key_press( unsigned char _key, int _x, int _y );
    void key_press_special( unsigned char _key, int _x, int _y );

    void add( Base * a );

    void render( void );

    void toString( void );

};


#endif

Here is where my first few design questions come. I'll start with the headers.h file include in the top.

In order to guarantee that all the modules are included, I had to write a bash script that will automatically write the header.h file with the needed modules included. I run this bash script using makefile before compilation. The reason this makes me uneasy is because it is a hurdle for cross platform support. I use OpenGL and this feels like it is undermining the cross-platform power of it. How should I be doing this ? How would it be done in industry ?

I'll arrive at my other problem. If you notice, the add() function takes in a Base *. This means that objects I pass will loose any non Base functionality right ? Is this OK ? I can work around it, but it feels like patchy fixes and I dislike the direction it is heading. Here is an example of an issue:

When the mouse is pressed, the main program wants to check if the mouse if hovering over any of the objects in the main list So I can do a loop and traverse the list:

void STUSURF::mouse_press( int _button, int _state, int _x, int _y ){
    for( int i = 0; i < main_list_len; i++ ){
        if( check_inside( _x, _y, main_list[i] ) )
            main_list[i]->mouse_press( _button, _state, _x, _y );
    }
}

Here is the problem, it is the check_inside() function

bool check_inside( int _x, int _y, Base * b ){
    int pos_x = _x - WINDOW_WIDTH;
    int pos_y = _y - WINDOW_HEIGHT;

    // this is just a test..... I know its not checking collision....
    std::cout << b->x << "\n";

    return false;
}

The x value that is returned is the one assigned to the Base class which is always 0. Even if the value is different. I know the value is different because it is being rendered at the correct x position.

It would work if I do this.

bool check_inside( int _x, int _y, Base * b ){
    int pos_x = _x - WINDOW_WIDTH;
    int pos_y = _y - WINDOW_HEIGHT;

    std::cout << (( test_module * )b)->x << "\n";

    return false;
}

However this means I can't just add modules and completely forget about the main code. I also attempted to use a template, however that didn't work because the only parameters passed to the check_inside() function is of type Base* .

The workaround here would be just giving all the objects mouse info at every click, and the objects will decide whether they should do something or not. But again, this feels janky for some reason.

How can I make this program modular in a good way ? is there a way for an object to store a type for me to use that to cast ? for example the test_module class will know it is a test_module, and when I need to I can make it cast itself ?

sorry for the long question.

Answer 1

This needs a lot of work to bring it up to current standard practices. Currently I would not consider this C++ code, but rather C code that happens to use some C++ features.

All Upper Case Identifiers

These have always been reserved for macros (#define). Thus using them for something else is likely to cause identifier collisions. So don't do that. Also note that macros do not respect namespaces.

class STUSURF{   // Not a good idea.

Underscore as prefix in Identifiers

STUSURF( char * _start );
void give_window_size( int * _w, int * _h );

Don't use them as a prefix. Your usage is actually fine. BUT the rules are complex and I am sure you don't know them. The fact that you did not break the rules is just complete coincidence.

Pointers

It is very rare to see pointers in C++ code.

The problem is that pointers don't express ownership semantics. So you don't know who is the oner of the pointed at object (i.e. who is supposed to delete it). This is what caused most of the problems in C.

You should use types that express ownership. References for non ownership. Smart pointers for ownership. Pointers themselves should not be used except in the very depth of a class and highly protected (and never across a public API).

char * path_start_screen;  // Would it not be better to use a std::string

Automatic Vs Dynamic Objects

Most important. When you can you should be using automatic objects (rather than dynamic objects (pointers)).

int * width;      // Why are these pointers?
int * height; 

Why not:

int   width;    // These are just object.
int   height;   // No need to manually manage memory on most objects.

You don't seem to be using standard types

The standard library has a whole host of standard types that make your life easier.

FILE * file_start_screen;   // FILE?
                            // That's a C type C++ use std::fstream

If you are storing a list:

//store a list of pointers that point to the base class. 
Base * * main_list;

Then we have std::list<>.

Void parameter list

void render( void );
void toString( void );

In C++ an empty parameter list and a void parameter list is the same thing there is no difference. It is more idiomatic to specify this as:

void render();
void toString();

Part of Type * and &

In C++ (a big difference from C) is that * is considered part of the type information and thus usually placed with the type.

// Not very idiomatic (actually weird as it conforms to neither C++ ro C standards).
int * width;
int * height;


// Idiomatic C++ style
int*   width;
int*   height;


// Idiomatic C style
int   *width;
int   *height;

Resource Management

Your class contains so many pointers that I am certain you need to do memory management (which is a good enough reason to get rid of the pointers). But this means your class will be broken as you have not obeyed the rule of three (or five) or zero. Go look them up.

Basically the compiler will auto generate several methods for you that will not do what you want if you have owned RAW pointers.

Comments:

In order to guarantee that all the modules are included, I had to write a bash script that will automatically write the header.h

That's absolutely ridiculous.
The code is now already not maintainable.
Immediately throw away that script and solve the main problem. Why is your header file so complicated.

How should I be doing this ? How would it be done in industry ?

Each Header file should declare a single main type (class). You may have small helper classes if you absolutely must, but try and avoid.

Move all definition into the source file*.cpp. The header should be declaration only (for now).

In the header file:
* If a type is only used as a pointer or reference you can forward declare it.
* If it is used as a member or a base you need the full definition so include its header file.
* Don't include its header files you don't need.

In the source File:
Each source file should have a matching header file.
It should include this matched header file and only the header files it needs.

the add() function takes in a Base *. This means that objects I pass will loose any non Base functionality right ?

No.

Is this OK ?

I doubt it would be OK. That's why it does not.

Here is the problem, it is the check_inside() function

Well its hard to tell if there is a problem as you have not provided the definition of the base class or one of the derived types. But from your description it sounds like you have not marked the method as virtual. Unlike Java (where every method is virtual by default) in C++ we don't want to pay the cost of a virtual function call unless you need to, so functions are not virtual by default and need to be explicitly marked virtual and their overridden version marked override.

The rest of your points are based on your original incorrect assumption.