7

I have a "tracker" class. This class tracks objects from a given input image. But in addition to this, there is another parameter that corresponds to a value used in generating the image. The tracker may want to suggest a new value for this parameter to get better results on the next iteration. It would be suggesting this new value for what ever generates the image.

I currently implement this via:

struct TrackResults{
    double new_scene_generation_coefficient;
    std::vector<Track> tracks;
}

class Tracker{
public:
    TrackResults update(const Image& image, double scene_generation_coefficient){
        // generate tracks and a new_scene_generation_coefficient 
        // ...
        return {new_scene_generation_coefficient, tracks}
    }
}

My intuition is that we don't want to modify the value directly and we want to avoid having the Tracker depend on knowing about a class it only needs one value from, and could potentially change. I feel if I instead returned by reference here, I would potentially cause a cascading effect of anything that uses the tracker to depend on something it otherwise wouldn't need to, or pass around a mutable reference dangerously.

I don't find any issues in terms of maintainability with this, but there are some very slight ergonomic issues, for example, when I want tracks, I have to first go through TrackResult object.

I'm concerned however, since this seems like a code smell, the values in the data class have nothing to do with each other, and there are no methods that would use both in tandem. In other posts people argue that there should be no data classes, since you should just be able to move functions that work on the members of the class into a method.

Another thing I should mention, is that Tracks is maintained internally any way. Tracks are returned by value to avoid messing with the internal structure of the class from outside actors (like loggers which exist in the real system). This makes me think that tracks should be queried after update, and the new_scene_generation_coefficient is the only thing that should be returned. Potentially however, there are more values that would need to be used to edit the object which generated the Image in the first place, but at least these are related. An issue with the separate track query after update is that it becomes dangerous for the user, as there is no guarantee that they will try to query directly afterwards, and that even if they do, that they will get the state of the track list as it was when update() was called. This seems like a code smell to require a user to call another method first in order use a separate method in this way.

  • Is there a clear indication of "start of scene" and "end of scene"? I would imagine that the scene generation coefficients would be revised at the end of scene point. – BobDalgleish Nov 29 '18 at 22:23
  • Divorce that coefficient from the tracks, they're in no way related. Give the Tracker itself the internal coefficient state and a set of semantic methods to tweak the state, something along the lines of "tracker.TightenTracking()" or "tracker.LoosenTracking()" so that no one outside needs to know you're using a double, only that they can ask for it to do something different. Now, whenever an outsider calls Tracker it uses the current state and your returns are clean and consistent. – Patrick Hughes Nov 29 '18 at 23:09
  • @BobDalgleish The scene is something like an Image. You update the parameters for retrieving the image. scene generation parameters are NOT revised at the end of the "image stream". Think of the thing that retrieves the image like a camera, if you can barely see anything because it is too dark in one image, you'll change the settings. If it's too bright, you'll also change it. – opa Nov 30 '18 at 14:13
  • 1
    @PatrickHughes the scene coefficient is a property of the scene generator, are you suggesting that in order to use the tracker, i have to give it a reference variable in its constructor? Note the scene generator also modifies this coefficient internally independent to what the tracker does. The need for this is necessitated because the scene generator is modeled on real world apparatus where this actually happens. – opa Nov 30 '18 at 14:20
  • It sounds like you're trying to force an API on top of the wrong abstraction layer. Wrap up the messy parts and let the messy class(es) handle the actual work while one layer upward can enjoy a clean interface. The part where code meets reality will always be less than CS Pure and Clean, reality is like that I've found while working on embedded hardware systems. – Patrick Hughes Dec 1 '18 at 0:20
3
+50

It looks like this can be cleaned up easily. You have a procedure that learns over iterations. Every time it is called, the quality of a parameter gets a little better. The first time that procedure is called you need some ballpark figure to start with.

So, you could pass initial_scene_generation_coefficient to the constructor of Tracker and store it as a member value scene_generation_coefficient. Remove the argument from Update's argument list and use the member instead, which you update on each call.

The generation coefficient is private to the Tracker, there is no reason to make it available outside of it apart from satisfying your curiosity (debugging).

You will want to lock the code in Update if you want to make it thread safe.

[Edit]

OK, I get it now after your comment. The mistake I see is that the tracker is supposed to have an understanding about image generation. That makes no sense and violates SRP. Tracker should just recognize objects in the image. There is no way it could assess the quality of the image, that requires either a human eye or a number of iterations that may yield a different number of recognized objects. If we rule out false positives, you could say the more recognized objects you get, the better the image quality must have been. It seems you want to feed the tracker a series of images generated with different lighting conditions and combine the tracking results. As you consistently get no results with some images, you may no longer bother to generate those (to use that generation coefficient because it is unrewarding). But the coefficient is meaningless in the context of the tracker so you would not pass it in or get it back.

You get back the recognized objects and you may try newly generated images to see if you get back more objects from the tracker. You could feed it both darker and lighter images and see if either yields more objects. That is the direction to move into with your coefficient until you get less objects again. Then you will have passed your optimum.

  • Agree with this but also want to point out that if you need multiple scene generation coefficients to be used independently of each other (in separate threads or simply separate nested functions) then utilize a different Tracker instance for those cases. There's no need to make an uber Tracker which handles all sorts of disparate coefficients at once. If the real-world case calls for more state in a tracker, then maybe the state could be shared in some way, but the coefficient doesn't need to be, so to speak, if there is a practical need to utilize multiple coefficients at any given... – Dragon Energy Dec 3 '18 at 9:47
  • ... time. That could be handled by multiple Tracker instances instead of trying to figure out all the different ways you might deal with that coefficient state externally to the class. A stateless class is kind of pointless (not entirely in C++ but sorta) -- at that point a procedural or functional design might suffice, but if objects are to benefit from being objects, then it often helps to associate some state for them to manage instead of trying to defer that responsibility to the client to constantly pass in as a parameter and get back as a return value only to pass it back in again. – Dragon Energy Dec 3 '18 at 9:47
  • I find this question interesting though rather on purpose or not since it illustrates an interesting clash between the concerns of pure functional programming avoiding side effects and object-oriented programming to encapsulate and centralize side effects to a particular state. Here an object encapsulating the state does the trick very effectively if we are to use objects. The current sort of design in the question utilizes an object without all the benefits while this sort of answer is about utilizing objects for what they're good at doing. – Dragon Energy Dec 3 '18 at 9:54
  • @Martin Maat It isn't learning, the tracker is basically saying "Hey, it might be too bright, track objects are getting saturated/ it might be too dark I don't see much!" It doesn't use previous state at all. This parameter could be something like Integration time. The scene generator needs integration time and can and does exist with out a tracker involved at all. It also updates this value internally independent of the tracker. – opa Dec 3 '18 at 19:36
  • @Martin Maat Okay, your edit gets closer (though this is not about optimizing for tracks), but where then do I put the logic to change values in the image generator based upon the tracks? Should it just be a function called after the tracks are generated? To me that seems like the best solution then. – opa Dec 4 '18 at 17:18
0

First of all I think the idea of "unrelated" here is a bit fuzzy if you need to constantly retrieve and pass that parameter back in to get the latest results. I don't know exactly what scene_generation_coefficient is supposed to do except that I'm seeing that you're basically making the client have to keep track of that state and constantly pass it into the object, get back a result, only to pass it back in again and repeat.

That's basically externalizing the responsibility of state management to the client (the person using your Tracker). And there are legit reasons to do that in some cases but I think most of the legit reasons tend to be in more procedural or functional contexts.

If we're utilizing OOP, then one of the key points of objects when it serves a practical need is to avoid this sort of thing. So one alternative design is to just make the Tracker store and update coefficient directly, and instantiate multiple trackers if you need to use these objects in disparate contexts. If you want to reuse the same tracker while completely disparate coefficients are being used simultaneously in different calling functions or threads at one time, then perhaps you might make TrackerResults responsible for that:

class TrackerResults
{
public:
    TrackerResults(...) {}

    // Even the image parameter might be a member passed through ctor
    // depending on design requirements.
    void update(const Image& image)
    {
        *this = tracker->update(image, scene_generation_coefficient);
    }

    const Track& operator[](size_t n) const 
    {
         return tracks[n];
    }

private:
    // Can use something like shared_ptr for robustness alternatively.
    Tracker* tracker;

    // Manage this internally in the object itself so the client doesn't
    // have to constantly retrieve and pass it back in.
    double scene_generation_coefficient;

    std::vector<Track> tracks;
};

... something like this for a crude illustration. Then the user just constructs one or more of these TrackerResults objects and calls update repeatedly to keep getting an improved result without having to manually manage this coefficients state. Something like this. If the client is free to ignore the "suggestion" for a new coefficient, then they might want to track the coefficient externally and you could program that feature into the TrackResults with some overloading, but mostly I'd try to make it so the client doesn't have to manage that state in a way that constantly requires passing in coefficient via parameter and capturing the resulting coefficient.

There's a balancing act here and it's not always so simple but a practical rule to start with before violating is don't make the client manage more state than they have to when it comes to OOP. You don't want to design a container class which requires the client to maintain the size of the container externally to the class itself, having to constantly pass it in and get back the new size in any function which can modify the size of the container as a very blatant and extreme example.

I can understand the concerns of avoiding side effects here but with objects a lot of the convenience comes from encapsulating the sort of state for which side effects are inevitable. To avoid overlap/overwrites and race conditions you end up using more than one object of that type in the same way you don't use one integer variable to capture every single integer output.

An issue with the separate track query after update is that it becomes dangerous for the user, as there is no guarantee that they will try to query directly afterwards, and that even if they do, that they will get the state of the track list as it was when update() was called.

I'll go ahead and list another concern in the same vein of thought which is that the shared state of the coefficient would make the tracker no longer thread-safe without locking. But that's a concern that's easily addressed by using multiple object instances with each instance maintaining its own separate coefficient. That's the OOP way where these types of functions do often incur a single side effect but the class makes it easy to maintain invariants and so forth. You could also throw a lock around this without modifying any client code if you need to utilize the same object as shared state for whatever reason.

I don't find any issues in terms of maintainability with this, but there are some very slight ergonomic issues, for example, when I want tracks, I have to first go through TrackResult object.

Blargh, ergonomics be damned in a language like C++. Trying to do things like minimize boilerplate too much is generally counter-productive here to favoring more straightforward code (if that's the alternative) that's less likely to be misunderstood. Still if we go with the proposed answers, then you get both a more ergonomic solution and one which shouldn't be too prone to misuse provided the client understands that the scene coefficient is part of the object's state being updated with each update call.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.