[haiku-development] Re: Extending BMessage
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Fri, 07 Sep 2012 12:14:24 +0200
Am 01.09.2012 23:40, schrieb Fredrik Modèen:
Now I have extend the BMessage with Save essage stuff from showimage. This
are how it would look like. Sorry for posting this here and not on track
but I think it's easier to comment things here :). What I would like to
know are.
I would be even easier to discuss the changes if you just pasted the
diff in the mail, though :-)
> + //Get Data
Missing space after //.
> + bool GetBool(const char* name,
> + bool defaultValue) const;
> + bool GetBool(const char* name, int32 index,
> + bool defaultValue)
const;
Indentation looks broken.
> + bool HasChanged(){ return fUpdated;}
[...]
> + bool fUpdated; // used for easy checking Set##
methods.
This is a poor solution for a number of reasons:
1) fUpdated changes the size of the object, and is thus not binary
compatible.
2) Why do only the Set*() methods change this, anyway?
3) Changed since when?
4) This has nothing to do in this class, and should be removed.
5) Instead, as Ingo pointed out, for the purpose of detecting changes,
you should use a copy of the originally read settings message around,
and compare them via Equals()/== -- and then only store it if something
changed.
And a few style issues as well:
- at least a space between ){, and between ;}
- the comment doesn't follow the style guide (should not appear in the
same line).
> * Authors:
> * Michael Lotz <mmlr@xxxxxxxx>
> + * Michael Pfeiffer, laplace@xxxxxxxxxxxx (using his code for
get/set)
This comment does not belong here, but in the commit message.
+ * Fredrik Modéen, [firstname]@[lastname].se
Interesting address. If you don't want to put yours in there, just leave
it out instead.
+
+
+// #pragma mark -
+// Get Data //
+template<typename T>
Two blank lines after the comment. "Get Data" and the pragma should
appear in a single line like this:
// #pragma mark - Get Data
This way, it will appear in Pe's function overview as a named separator.
> +status_t
> +BMessage::_FindType(const char* name, type_code type, int32 index,
> + T* value) const
> +{
> + const void* data;
> + int32 size;
> + status_t error = FindData(name, type, index, &data, &size);
> + if (error != B_OK)
> + return error;
> +
> + if (size != sizeof(T))
> + return B_BAD_DATA;
> +
> + *value = *(T*)data;
> +
> + return B_OK;
> +}
That looks more like taken from KMessage, but I see no copyright
attribution here.
In any case, the existing code from KMessage is better:
> +void
> +BMessage::SetRect(const char* name, BRect value)
> +{
> + if (HasRect(name))
> + ReplaceRect(name, value);
> + else
> + AddRect(name, value);
> +
> + fUpdated = true;
> +}
See KMessage::SetData().
> * Authors:
> * Michael Pfeiffer, laplace@xxxxxxxxxxxx
> * Fredrik Modéen, [firstname]@[lastname].se
> */
> #ifndef SAVE_MESSAGE_H
If this is supposed to become a public header some day, it should not
list the authors.
In any case, SaveMessage is not a good name for this class, something
like BMessageStore would be better.
You could also have a BSettingsMessage providing the functionality
instead, subclassing BMessage, and automatically provide the "has
changed" feature.
> static BMessage* Load(char* appname);
What's that good for?
It should either be just Load() and use the be_app's Signature(), or you
should be able to define a partial settings path yourself, to have more
control over the file. Both variants should be added, same for Save().
> bool
> SaveMessage::Save(BMessage* settings, char* appname)
> {
> BFile file;
> if (_OpenSettingsFile(&file, false, appname)) {
> settings->Flatten(&file);
> return true;
> } else
> return false;
> }
First of all, why eat the error code? How is the application supposed to
give meaningful error messages this way?
Second of all, the 'else' is superfluous, and should be removed for clarity.
Bye,
Axel.
Other related posts:
