[haiku-3rdparty-dev] Re: Request code review: inter-application messaging
- From: Michael Pfeiffer <michael.w.pfeiffer@xxxxxxxxx>
- To: haiku-3rdparty-dev@xxxxxxxxxxxxx
- Date: Sun, 19 Dec 2010 18:45:20 +0100
Am 19.12.2010 um 00:20 schrieb hudsonco1@xxxxxxx:
> I am posting the code for 2 reasons. First reason is a request for a code
> review.
I haven't looked closely at the functionality as it seems there is
something missing (the application class) and I also didn't take the
time to think about what is the best solution to that problem.
Remarks:
- use the layout API instead of hard coded positioning of buttons
- instead of hard coded screen size use BScreen to
get the current screen size
- don't use static variables in MessageReceived
use member variables in class MainWindow instead
and it is not necessary to dynamically allocate
them
- the variable appTeams leaks memory
Allocate it on the stack:
BList appTeams;
- it is not necessary to wait for A_GET_CONNECTED
in MessageReceived() to load the sound files;
could be done in the window constructor;
if you allocate BSimpleGameSound dynamically
you have to delete the object in the window
destructor
- in method MessageReceived in case M_QUIT:
You define bmsg on stack but never use it.
Instead later it is declared some lines later
as a pointer to a BMessenger, that is leaked
in the error case.
The line
BMessenger *bmsg = new BMessenger(mySig, t_id, &err);
can be replaced:
bmsg = BMessenger(mySig, t_id, &err);
and you don't have to care about deallocation anymore.
Code style:
See http://www.haiku-os.org/development/coding-guidelines
- use camel case for variable names; don't use an underscore
instead of "t_id" use "team" or "teamID"
- order include statements alphabetically
- there is always one space after a comma
- there is one space before and after the
assignment operator "="
- usually there is one space before and after
an arithmetic operator: a + b; a / b
- there is a space between "switch" and the opening "("
switch (...)
- one variable per definition
BMessage* msg1 = new BMessage(M_APP_1);
Not:
BMessage *msg1, *msg2, ...
- pointer belongs to type not to variable
BMessage* message = ...
instead of
BMessage *message
- put variable definitions as close as possible to the first usage
BMessage* msg1 = ... in front of
BButton* button1 = ...
- variable names start with lower case letter
btn1 not Btn1
- don't use abbreviations in variable names
(exceptions established abbr. e.g. HTML, FTP, ...)
button1 not btn1
- consider to use an array instead of variables like button1 - 4
- give variable names meaningful names
What is/does msg1-4, Btn1-4?
- in switch statement the case blocks are indented one level
switch (...) {
case ...:
break;
...
}
- don't put comment after a statement
instead put it at the same level in front of it
// comment
next statement;
- MessageReceived method is quite large separate it into
several methods, for example put the code of each "case" block
into a method; the repeated code for M_APP_1 to M_APP_4
should be handled by one method only, pass appropriate parameters to
it to handle the different cases.
Bye,
Michael
Other related posts:
