How to improve my small class ?

[GodIsDead]

Hello, I'm not really beginner in C++ but I don't know everything yet... And it's my first try at writing a class.

1) Can you take a look at this working example code and tell me if there is anything that need changes or could be improved ?

#include <iostream>
using namespace std;


class MyConsole
{
	public:

		MyConsole()
		: _inputCallback( NULL )
		{
		}

		void bindInputCallback ( void (*func)( const MyConsole &, const char * ) )
		{
			_inputCallback = func;
			_inputCallback( *this, "callback bound" ); // test it
		}

		bool operator == ( const MyConsole & obj ) const { return this == &obj; }

	private:

		void (*_inputCallback)( const MyConsole &, const char * );
};


MyConsole console1, console2;

void onInput( const MyConsole & object, const char * input )
{
	if ( object == console1 )
	{
		printf( "console1 says: %s\n", input );
	}
	else if ( object == console2 )
	{
		printf( "console2 says: %s\n", input );
	}
}

int main()
{
	console1.bindInputCallback( onInput );
	console2.bindInputCallback( onInput );
	return 0;
}

2) I have a doubt on line
bool operator == ( const MyConsole & obj ) const { return this == &obj; }
I have read somewhere that it should be "return *this = obj" but it crashes program.


3) Also, I am not sure if I should use the "&" or not, on these lines:

void bindInputCallback ( void (*func)( const MyConsole &, const char * ) )
void (*_inputCallback)( const MyConsole &, const char * );

. It works with or without those "&", but which one is the best/correct way?


4) Finally, I would like to add another prototype for the "callback" (not sure if it's the correct word):
void (*_inputCallback)( const char * );
But I have no idea how to do that properly so that the same bindInputCallback method can be used for both prototypes.


Any help appreciated! Thanks in advance :)

[JLBorges]

How is this class going to be used in a program? Trying to write a small hypothetical program that uses this class would help clarify what its interface ought to look like.

[Thomas1965]

Here is a different way.

#include <iostream>
#include <vector>

using namespace std;

class MyConsole; // forward declaration

class IConsoleCallback
{
public:
  virtual void OnInput(const MyConsole*, const char *) = 0;
  virtual void OnInput(const char *) = 0;
};

class MyConsole
{
public:

  MyConsole(const char *name): mName(name)
  {
  }
  
  void bindInputCallback(IConsoleCallback *callBack)
  {
    if(callBack != nullptr)
      mCallbacks.push_back(callBack);

    notify();
  }
  const char *name() const
  {
    return mName;
  }

  bool operator == (const MyConsole & obj) const { return this == &obj; }

private:
  const char *mName;
  void notify()
  {
    for (size_t i =0; i < mCallbacks.size(); i++)
    {
      mCallbacks[i]->OnInput(this, "Hello");
      mCallbacks[i]->OnInput("Hello");
    }
  }
  vector<IConsoleCallback *> mCallbacks;
};

class Demo: public IConsoleCallback
{
public:
  virtual void OnInput(const MyConsole* con, const char *input) override
  {
    cout << "MyConsole " << con->name() << " says: " << input << "\n";
  }
  virtual void OnInput(const char *input) override
  {
    cout << "Input " << input << "\n";
  }
  virtual ~Demo(){}
};

int main()
{
  MyConsole con1("Console 1"), con2("Console 2");
  Demo demo;

  con1.bindInputCallback(&demo);
  con2.bindInputCallback(&demo);
  return 0;
}

[GodIsDead]

There is the example program at line 28. You can run this code, it works.

But maybe the example is not obvious, I should explain. The "onInput" function is to be called when the user press enter after typing some text in a text field. In this function, the user will be able to create his own commands, etc... This is for making a simple console, where user can see messages and send commands. This is not related to Windows or other OS, it's for a microcontroller.

Essentially I need help with 3) and 4). What is the proper way to implement this second callback, so that in case the program only have one instance of this class, the user can pass a function without the "object" parameter, like so:

MyConsole console;

void onInput( const char * input )
{
    if (!strcmp( input, ...
}
...
console.bindInputCallback( onInput );

Yes I name this "callback" because some game have scripting systems with such "callbacks" (onPlayerConnect, etc...), but when I search "C++ callback" I can't find much info about what I want to do.



Thomas1965, thanks but I have seen this already and there are many things I don't like with this way. I want it to be as simple as possible for the user of my class, that is, I dont want the user to have to create that Demo class.


My current solution is to have another function pointer member:

void bindInputCallback ( void (*func)( const char * ) ) { _inputCallback2 = func; }
...
void (*_inputCallback2)( const char * );

And then in the main code of MyConsole class I do

if ( _inputCallback != nullptr )
	_inputCallback( *this, _inputTextBox );
if ( _inputCallback2 != nullptr )
	_inputCallback2( _inputTextBox );

It's simple and works, I think I will keep it that way unless you can improve it without changing the syntax for the end user ?

I don't need to store these callbacks in a list, at least not yet.

[JLBorges]

> Essentially I need help with 3) and 4).
> What is the proper way to implement this second callback, so that in case the program only
> have one instance of this class, the user can pass a function without the "object" parameter

Something like this, perhaps:

#include <iostream>
#include <string>
#include <functional>
#include <algorithm> // for std::swap

struct command_processor
{
    // using a templated call backs (via a polymorphic call wrapper) is idiomatic
    // it gives a great deal of flexibility to client code; it allows us to think of
    // functions as logical entities without being shackled by syntactic rigidity
    // http://en.cppreference.com/w/cpp/utility/functional/function
    // making it a predicate (return true if the call back handled the command, false otherwise)
    // is a good idea; it would enable us to implement a chain of responsibility
    // which is likely to become a necessity if the not too far future
    // (having a god like component which knows everything about every command introduces very tight coupling)
    // chain of responsibility: http://www.oodesign.com/chain-of-responsibility-pattern.html
    using generic_callback = std::function< bool ( const command_processor&, std::string ) > ;
    using singleton_callback = std::function< bool ( std::string ) > ;

    command_processor() = default ;

    // generic callback
    explicit command_processor( generic_callback fn ) : process_command( std::move(fn) ) {}

    // singleton callback: just ignore the first argument and call the function
    explicit command_processor( singleton_callback fn )
        : process_command( [ fn = std::move(fn) ]( const command_processor&, std::string cmd )
                           { return fn( std::move(cmd) ) ; } ) {}

    ~command_processor() = default ;

    // non-copyable
    command_processor( const command_processor& ) = delete ;
    command_processor& operator= ( const command_processor& ) = delete ;

    // moveable
    command_processor( command_processor&& that ) noexcept
    { swap( process_command, that.process_command ) ; }
    command_processor& operator= ( command_processor&& that )
    { swap( process_command, that.process_command ) ;  return *this ; }

    // bind generic callback
    void bind_callback( generic_callback fn ) { process_command = std::move(fn) ; }

    // singleton callback: just ignore the first argument and call the function
    void bind_callback( singleton_callback fn )
    { process_command = [ fn = std::move(fn) ]( const command_processor&, std::string cmd )
                        { return fn( std::move(cmd) ) ; } ; }

    // for testing: process the command by forwarding it to the call back
    bool accept_cmd( std::string cmd ) const { return process_command( *this, std::move(cmd) ) ; }

    // making the command processor default constructible may be a good idea. here, the default call back
    // does not do anything and returns false to indicate that the command was not processed
    private: generic_callback process_command = [] ( const command_processor&, std::string ) { return false ; } ;
};

bool process_cmd( const command_processor& cp, std::string cmd )
{
    return bool( std::cout << "#1. recd command '" << cmd << "' from command processor @ "
                           << std::addressof(cp) << '\n' ) ;
}

int main() // simple test_driver
{
    const auto process_cmd_alt = [] ( std::string cmd )
    { return bool( std::cout << "#2. recd command '" << cmd << "' from an unknown command processor\n" ) ; };

    command_processor cp1(process_cmd) ;
    command_processor cp2(process_cmd_alt) ;

    std::string cmd ;
    while( std::cout << "command? " && std::getline( std::cin, cmd ) )
    {
        cp1.accept_cmd(cmd) ;
        cp2.accept_cmd(cmd) ;
        std::cout << '\n' ;
        using std::swap ; swap( cp1, cp2 ) ; // swap the two command processors
    }

    cp1.bind_callback( [] ( std::string cmd )
    { std::cout << "1. command '" << cmd << "' is ignored\n" ; return false ; } ) ;

    cp1.accept_cmd( "*** ignore this ***" ) ;
}

http://coliru.stacked-crooked.com/a/48a6917d331479a1

[GodIsDead]

I'm surprised how it looks so similar to my code in what it does... Did you just wrote this?

Anyway thanks, I will study this alternative because I am curious but honestly, my code runs on small microcontroller and using std::function etc adds code, which I am trying to avoid.

Thank you ;)

[JLBorges]

> Did you just wrote this?

Yes; it is not production quality code.

[Thomas1965]

@JLBorges,
to me it looks professional.
How would you write it for production - if you don't mind?

[JLBorges]

To me, the design of the class as posted appears to be flakey.

For one, having these two different kinds of callbacks (one for singletons and another for non-singletons) is dubious: how is the user of the class who gets a reference to an object of this kind expected to know if it a is singleton not?

For another, there must surely be a mechanism to bind a component-specific callback, process the commands and restore the previous callback once we are done. Or, in similar vein, install a a custom filter to pre-process the commands and then forward it to the original callback for handling.

Much more context is required to do a tenable design of something like this.

In addition to routine programming hygiene like: the class should not be polluting the global namespace.

[Thomas1965]

Thanks