How do I design a class so that methods must be called in a certain order?
Suppose you have a class with some methods, and the methods must be called in a certain order. For example, suppose your class is for importing photos from a camera. First, you have to specify which camera you are importing from. Then you discover the pictures. Then you have to select which pictures you want to import, and set the options for how you want the download to occur. And finally, you download them.
class PhotoImporter
{
public:
PhotoImporter(const Camera& camera);
// Optionally configure the discovery.
void SetFindOnlyNewPhotos(bool newPhotosOnly);
// Call this first to get some photos.
std::vector<Photo> DiscoverPhotos();
// Then decide which photos you want to download by calling
// this for each photo you want to download.
void SelectPhoto(Photo photo);
void SelectAllDiscoveredPhotos();
// Configure the download. You must set a download folder.
// The other settings default to false.
void SetDownloadFolder(const path& downloadFolder);
void SetRenumberPhotos(bool renumber);
void SetDeleteOriginalsAfterDownload(bool deleteOriginals);
// And then download them.
void Download();
};
The problem is that there is nothing preventing the caller from calling the methods out of order or omitting some methods altogether.
void confused()
{
PhotoImporter importer(mainCamera);
importer.SelectAllDiscoveredPhotos();
importer.Download();
importer.SetRenumberPhotos(true);
importer.DiscoverPhotos();
// never specified a download folder
}
One trick for making it harder to call the methods in the wrong order is to represent each state of the import process as a separate class.
class PhotoImporter
{
public:
PhotoImporter(const Camera& camera);
// Optionally configure the discovery.
void SetFindOnlyNewPhotos(bool newPhotosOnly);
// Call this first to get some photos.
DiscoveredPhotos DiscoverPhotos();
};
class DiscoveredPhotos
{
public:
// Not publically constructible.
const std::vector<Photo>& GetPhotos();
// Decide which photos you want to download by calling
// this for each photo you want to download.
void SelectPhoto(Photo photo);
void SelectAllDiscoveredPhotos();
// Configure the download. You must set a download folder.
// The other settings default to false.
void SetDownloadFolder(const path& downloadFolder);
void SetRenumberPhotos(bool renumber);
void SetDeleteOriginalsAfterDownload(bool deleteOriginals);
// And then download them.
void Download();
}
Breaking it up this way means that it is impossible to call Download before calling DiscoverPhotos, because in order to download the photos, you need to get a DiscoveredPhotos object, and the only way to get one of those is by calling DiscoverPhotos.
And then there’s the issue of requiring a download folder. For that, we could make the mandatory portion an explicit parameter to the Download method. We used this trick when we required the Camera to be passed to the PhotoImporter constructor.
class DiscoveredPhotos
{
public:
...
void Download(const path& downloadFolder);
}
And to ensure that people don’t try to do things like call SetRenumberPhotos after Download, you could put all of the download options into an options class.
class DownloadOptions
{
public:
void SetRenumberPhotos(bool renumber);
void SetDeleteOriginalsAfterDownload(bool deleteOriginals);
};
class DiscoveredPhotos
{
public:
// Not publically constructible.
const std::vector<Photo>& GetPhotos();
// Decide which photos you want to download by calling
// this for each photo you want to download.
void SelectPhoto(Photo photo);
void SelectAllDiscoveredPhotos();
// And then download them.
void Download(const path& downloadFolder,
const std::optional<DownloadOptions>& options = std::nullopt);
}
While this technique forces the programmer to satisfy prerequisites before calling a method, it doesn’t prevent the programmer from trying to go backwards. For example, after calling Download, the programmer could go back and select some more photos and then call Download a second time.
If you want to disallow that, then I’m stumped. I can’t think of something that prevents an object from being used after a particular method is called, with enforcement at compile time. Maybe you can think of something?
Light
Dark
34 comments
Aplogies if I mix C# and C++ terminology but I’d think you ensure that the class has a delegate it can call when the new object is created but you move the creation of your new object into this class’es destructor. So you only get the new object when the old has been destroyed. (I’m sure this is all kinds of levels of evil and may even not work for non-obvious-to-me reasons at the moment.
This would be Very Bad as destructors are Not Allowed to fail, and absolutely must not throw exceptions for any reason – including out of memory (eg when the next delegate object is being constructed)
There’s plenty of really good reasons for this, but basically boil down to the fact that a failed destruction is fundamentally impossible to recover from, so the process must die.
I’m a little hesitant to suggest this (since someone might go and use it), but you could use a ref-qualifier. Something like this:
class Step1
{
public:
Step2 DoIt() &&;};
So calling DoIt() gives you a Step2 object and leaves Step1 in a moved from state (where presumably you can no longer do anything with it). But honestly, I think the best answer is to not require that your clients call methods in a particular order.
Why do you think this approach is not suitable? Move semantics is the closest thing we have to model ownership in C++. If calling a particular function “consumes” the object, then what’s wrong with having that function take an rvalue reference?
I agree, it’s essentially implementing affine types in C++.
Move semantics are one way to do this.
Accept a std::unique_ptr<> for each step, and eat it.
Use inversion of control. Instead of having the client code assign properties and call methods on the PhotoImporter, have the client register callback functions (or async events…) to the importer, then call the single PhotoImporter::import() method. This in turn performs the operations in the correct way and invoke the callbacks in the appropriate order. The callbacks cannot “do” something, they can just tell what they want to be done.
This. It also satifies re-entrant problem as posed. If you take a lock when the call begins and release it when the call completes or if cancelled during one of the callbacks then concurrent calls will be blocked until the process is complete or cancelled.
IOC seems like the right approach to solve the problem of guiding the developer into doing the “right” thing, but of course doesn’t actually prevent them from doing weird things that you’re trying to avoid as any one of those callbacks could pretty much do whatever it wants. It’s probably the closest thing to a “pit of success” design though.
Yes you’re right that if the callback does a silly thing such as unmounting the USB device, no amount of magic in the CameraImporter will be able to recover. But at least your design can guarantee that the unmount does not occur while a copy is in progress…
I don’t know about C#. In a language with dependent types, it’s certainly possible: http://docs.idris-lang.org/en/latest/st/introduction.html
We’ve handled this situation in our C# apps through the Builder Pattern.Create a series of interfaces that define the individual steps as methods that return the interface that defines the next step. Then call them through a builder that internally constructs an object that implements all of the interfaces. Something like this:
<pre>
public interface IStage0{
IStage1 MoveOnToStage1();
}
public interface IStage1
{
IStage2 MoveOnToStage2();
}
public interface IStage2{
IStage3 MoveOnToStage3();
}
public interface IStage3{
void Done();
}
internal class Foo : IStage0, IStage1, IStage2, IStage3
{
public Foo(InitialData data)
{ // set internal state }
public IStage1 MoveOnToStage1()
{
// do work
return this;
}
public IStage2 MoveOnToStage2()
{
// do work
return this;
}
public IStage3 MoveOnToStage3()
{
// do work
return this;
}
public void Done()
{
// do work
}
}
public class FooBuilder{
public FooBuilder();
public IStage0 GetInitialObject(InitialData data) // this could also be a static method on Foo if you want to make Foo public and give it a private constructor.
{
return new Foo(data);
}
}
And to call it: new FooBuilder().GetInitialObject(initialData).MoveOnToStage1().MoveOnToStage2().MoveOnToStage3().Done();
</pre>
Wow… formatting got butchered…
18 years (at least) of Microsoft developer blogs, over at least 4 platforms and they still haven’t found one where you can post formatted code in the comments.
Yeah, System.Reflection.Emit is very much married to this concept (anything Fluent fits in well with this requirement). Every construct has an associated builder that must be created via its parent construct’s builder.
You could use a modified passkey idiom. Each function would return a non-copiable key, that has to be moved into the next function:
class Camera;class PhotoImporter;struct DiscoveredPhotos{ friend class PhotoImporter;private: DiscoveredPhotos(){}};struct SelectedPhotos{ friend class PhotoImporter;private: SelectedPhotos() {}};class PhotoImporter{public: PhotoImporter(const Camera& camera); DiscoveredPhotos DiscoverPhotos(); SelectedPhotos SelectPhotos(DiscoveredPhotos&& discovered_photos); void Download(SelectedPhotos&& selected_photos);};
This reminds me of how InitializeSecurityContext() API works.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375507(v=vs.85).aspx
Just pass the returned token to the next function until one of the stop conditions.
> I can’t think of something that prevents an object from being used after a particular method is called, with enforcement at compile time
— Rust move semantics offer a neat solution here. A function like download could accept its DiscoveredPhotos argument as a value. If DiscoveredPhotos is not a `Copy`able type, this means transfering ownership, preventing the previous owner from using the value afterwards. This is quite commonly applied as part of the builder pattern, where constructing the final type can consume the builder by taking it as a value.
In a sense Rust implements an affine type system.
// Edit: the formatting of comments here is awful. Everything gets ripped out, even paragraphs and line breaks!
I would have create a static method .CreatePhotoImporter(), with parameter that is an object that wraps all required parameters in, then calls these “must be called in certain order” methods for the user before returning the object. Those methods could be changed to private if there’s no good reason to expose them.
Essentially it’s similar idea to what “parameter parsers” do.
Temporal coupling like this should be avoided if possible, but oftentimes its hard to do in real world with the clock ticking. One way we have mitigated the issue is by using fluent class builders. They do not bloat your class’s constructor, and allow for lots of flexibility, it still constrains you to having only 1 public method on your class. Everyhing else had to be setup by builder/factory. You can call the builder methods in any order, but to get an configured object, you have to call build on factory.
PhotoImporter importer = new PhotoImporter.Builder() .SetFindOnlyNewPhotos(true) .SetDownloadFolder(Path.GetTempPath()) .SetRenumberPhotos(true) .SetSelectPhotosFunction(x => x.Name.Equals(“1.jpeg”)) .Build(camera); importer.Download(); // only public method on importer. Configuration was done by builder.
public class PhotoImporter { private readonly Camera camera; private bool newPhotosOnly; private string downloadPath; private bool reNumberPhotos; private bool deleteOriginals; private PhotoImporter(Camera camera)//private constructor forces use of Builder. { this.camera = camera; } public void Download() { // state has been managed by builder. } public class Builder // nestled class has access to private fields/methods { private bool newPhotosOnly; private string downloadPath; private bool reNumberPhotos; private bool deleteOriginals; private Func<Photo, bool> selectPhotosFunction; public Builder SetFindOnlyNewPhotos(bool newPhotosOnly) { this.newPhotosOnly = newPhotosOnly; return this; // builder methods always return this instance so further configuration is possible. } public Builder SetDownloadFolder(string downloadPath) { this.downloadPath = downloadPath; return this; } public Builder SetRenumberPhotos(bool reNumberPhotos) { this.reNumberPhotos = reNumberPhotos; return this; } public Builder SetDeleteOriginalsAfterDownload(bool deleteOriginals) { this.deleteOriginals = deleteOriginals; return this; } public Builder SetSelectPhotosFunction(Func<Photo, bool> selectPhotosFunction) { this.selectPhotosFunction = selectPhotosFunction; return this; } public PhotoImporter Build(Camera camera) { PhotoImporter importer = new PhotoImporter(camera); importer.newPhotosOnly = this.newPhotosOnly; importer.reNumberPhotos = this.reNumberPhotos; importer.deleteOriginals = this.deleteOriginals; if(string.IsNullOrWhiteSpace(this.downloadPath)) throw new InvalidOperationException(“No path specified.”); if(!Directory.Exists(this.downloadPath)) throw new InvalidOperationException(“Download Path does not exist.”); importer.downloadPath = this.downloadPath; if (this.selectPhotosFunction == null) throw new InvalidOperationException(“Photo Selection function must be set.”); return importer; } } }
I already commented on the new site being awful in the post about the migration, but the new site is really so terrible and gets in the way so much, that I will comment one more time – and then shut up. The new site is (a) very slow (8 seconds to open a page is complete bonkers), (b) breaks comment formatting (see comments in this post for example), and (c) lost old comments which were 80% of the value of the blog. It also looks worse on mobile, lost archive links (yes, I know that you can type URLs manually), etc, etc, etc. None of this is Raymond’s fault, but since the braindead decision to migrate isn’t my fault either and I have been a reader since the very beginning, I am going to say that the migration was stupid, that the old site was much better, and request that someone wakes up to this and maybe, I don’t know, considers doing something about it. The awful performance and awful qualities of the new site make visiting this blog unpleasant. Yes, I submitted this as a feedback as well and if you think the same, I encourage you to do it, too. Shutting up now.
Agreed, this blog is more or less equivalent to a brand new blog. Many times I’ve gone here thanks to Hacker News, for example https://news.ycombinator.com/item?id=4781372
But of course the links in that post are now defunct. If Hacker News can preserve contents from 7 years back, why cannot Microsoft?
I’ll say more – I have no idea why the heck they decided to migrate at all. Just why? I don’t understand it. I guess they did have some reason, but I legitimately have no clue as to what it is. It cannot be better user experience, because it became worse, not better, visibly and on multiple fronts. I am not being facetious here. If the reason for the migration *was* better user experience – then why nobody cared enough to test what it is going to be??? Because if they did test it, they’d immediately see that yeah, the new site is slow, yeah, the comments now don’t have formatting, and by the way, the old comments do not carry through, etc. And if they did test it and did see the effect and decided that this is somehow fine, I am just at a complete loss for words… Really, the end result of the migration so far for me is that I am now no longer reading MSDN blogs on the phone at all and am considering just stopping visiting the blogs in the browser on the desktop as well and instead blow the dust off the RSS readers that were all the rage circa 2008 (not so much now) and move to reading some of the blogs that way – because the new site is, again, slow, and actively fights comments. Until they break the RSS feed as well, that is. Guess that’ll happen next migration.
One strategy would be to make all of your functional methods private but expose function pointers. Set the function pointers only when it is appropriate. So the constructor will initialise the pointer to phase1() and NULL the others. When phase1() completes it sets the pointer to phase2() and NULLs the phase1() pointer. etc.
Of course you could more easily permit the programmer to call any method, but return fail status unless the object is in the correct state.
Why not just use a state machine within the class instances?Only allow appropriate state-transitions. A client that violates them gets a clear indication of what went wrong, albeit at runtime.Similarly, one could block clients from “going backwards” by setting a state in the previous state-class (from your example Raymond – of a class for each state accessible only from the previous state) – by having each one supply a back-ref or a shared pointer to a live-state flag that simply isn’t cleared unilt the subsequent instance is closed. Some sort of chaining so that until the supplied next-state-instance is done, this-state-instance is invalidated in terms of reusing it (or maybe it is simply marked as dead the second it gives up the next-state instance).I don’t see how you can enforce any of the backwards blocking at compile time, only at runtime, unless as some posters have mentioned – using move semantics to consume the previous state instance when generating the new-state-instance.Although – that just seems like a fine solution anyway. A is consumed to produce B is consumed to produce C …
I believe Rust’s tracking of ownership provides this:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=daf6b400e46038b296a46941397bdf60
I’d try with functions, that take a rvalue reference to the object, that should not be used any more after the call. So the code could look like
“`photo_importer::download(photo_importer::discover_photos(photo_importer::chose_camera(main_camera)), target_dir); “`
Since the return value of chose_camera and discover_photos will be moved in, there is no way to use these objects afterwards.
You might also consider to overload the |-Operator like the ranges-TS does:
“`photo_importer::chose_camera(main_camera) | photo_importer::discover_photos() | photo_importer::download(target_dir);“`
Then the return value of `chose_camera()` can only be used in a `|`-opreator on the left hand side with the return type of `discover_photos()` on the right hand sinde. The return value can only be used on the right hand side of a `|`-operator with the return value of the former `|`-operator on the left hand side, etc. All the left hand side objects will be passed as rvalue references, so they can not be reused as well.
But there is nothing that prevents you at compile time from trying to use a moved-from object.
If a solution isn’t obvious, wouldn’t that be an unnecessary complication? What about implementing the limitation via asserts?
The goal here is to make it impossible even to write the code that does the wrong thing. Sure, you could do all this with asserts, but those aren’t checked until runtime, and it might be in a code path that is rarely-triggered and eluded testing.
A good point. Maybe in that case we just need new language facilities to enable that pattern?
Another approach would be to use a Finite State Machine with a method passing the “message” of the next step to be performed, a bit like:bool Perform( const char * message, …);Using va_arg, you get the appropriate parameters and the FSM ensure the user respects the proper import order.
But how do you enforce this at compile time?
Shouldn’t DiscoveredPhotos::GetPhotos() return by value instead of reference? Not sure if that’s a typo or if I’m missing something in the example.