Code review comment for lp:~jpakkane/mediascanner/immutable-mediafile

Revision history for this message
James Henstridge (jamesh) wrote :

[1]
Since the aim of this branch is to make MediaFile act like a value type, would it make sense to declare its data members to be const? That would ensure that the the MediaFile code itself doesn't start mutating the instance.

[2]
Since the big long argument list for the MediaFile constructor is a problem, would it be worth getting rid of it all together? At present, it is only being used in (a) MediaStore.make_media() and (b) tests, which could both be converted to MediaFileBuilder. This would also mean those pieces of code wouldn't immediately need updating whenever a new data member is added to MediaFile.

If we followed the example from the C++ FAQ, we could have a MediaFile constructor taking a Builder instance.

[3]
I'm on the fence on this one, but would it make sense for the MediaFileBuilder constructor to take the file name directly? This is the one field that is mandatory in a MediaFile, so it doesn't seem so bad to require it when creating the builder.

« Back to merge proposal