Boost :

Date view	Thread view	Subject view	Author view

Subject: Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?
From: Pierre Morcello (pmorcell-cppfrance_at_[hidden])
Date: 2009-09-08 17:00:02

Next message: Kenny Riddile: "Re: [boost] [warnings] Are warnings acceptable artifactsfrom builds?"
Previous message: troy d. straszheim: "Re: [boost] [cmake] revision 50756 failed to merge some file properties"
In reply to: Stewart, Robert: "Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?"
Next in thread: Stewart, Robert: "Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?"
Reply: Stewart, Robert: "Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?"

Stewart, Robert wrote :
>>>"You said before that splitting things into multiple functions
reduced readability. My version is more readable for several
reasons. Each function has a single purpose rather than one
function doing many things. Notice that I removed some comments
in your version because the function names obviated them (self-
documenting code). Finally, the high level logic, in the initial
block, is clearer, while the low level logic is segregated into
separate functions."

Please don't misunderstand me : I don't think that splitting things into multiple functions reduce readability. I think that in some case it reduces readability and also maintanability of the code. You can increase readibility in some case. As an example, with BOOST_FOREACH macro, you don't need to split the code into some functors to make it run (this is explained more simply on the page of the library).

Your function proposition has also some side effects, that you think are nice, and that I think are not nice. This is due to a misunderstanding between us on the kind of work we suppose the 'subfunctions' will do. I explain myself in the following paragraphes.

0/ just a side note.
In the case of the "find Loaded Meshes", in fact I was thinking to a very special case of a memory manager of a 3D game that is -during loading threaded resources- updating some data before final cleaning of its state. It just means that there is already a findLoadedMeshes() function, for the user of the class, and it uses the cleaned set of information, not the 'dirty' one being loaded as supposed in my example. So the name of the function, must be chosen carefully to suppress any doubt from the reader.

1/
One side effect in your approach is that it forces the user to declare the function in the .h. If there is no use to reuse the code (because it is extremely specific to the function), then you are polluting the .h with things that are not information specific to the class, but only to one function.

2/
If I have to declare the function in the .h, I may end writing : "void _myfuntion_findLoadedMeshes()" in private, with a doxygen description on top of it. Why such a description if the code is 'self explanatory'? The reason is : each of my function got a set of preconditions to tests, sometimes post conditions, and I would not ask someone to maintain a system without these informations. Without these comments, I suppose I will also end asserting everything in the code I already already tested back in the 'parent' function. As a matter of fact I have already seen guys who were testing a boolean member as a precondition to see if the function was called just after the 'supposed previous call'.
So there is a need to restrain the use of the 'sub-function' to the 'parent function' only.

My conclusion on this 'keep the h clean' part is : if the 'sub-part' is big or to be reused, 'function split' (as you proposed) or even aggregation is the way to go. If the sub-part is very specific and 'middle-big', a local struct with methods is much nicer. In the last case, if the 'sub-part' is little and very specific, you will appreciate the 'Breakable' idiom.

3/ possible errors creation during subfunction writing.
When you write the 'Breakable', you are only writing 1 line. It is very hard to write something wrong at that level.
When you declare the subfunctions, you have to declare the parameters, also gives them meaningfull names. Most of the time, let say I have between 2 or 3 parameters to give to the subfunction. 1 time out of 3 I have a std container to transmit. Someone with less experience than me (or myself when I am tired) could write "bool find_loaded_meshes(std::set<Mesh*> _meshes)" and forgetting the & in the definition. Then copy/pasting the result to write quickier its declaration, it stays the same. Then, if the function is just checking things inside the container, you got a real performance hit for free. Otherwise you will find this error soon when it bugs.
So conclusion on that point is : this will take you more time and length to code, and is more prone to some errors (even if they are rare / not tragic).

4/ People write a function when they need one, not in order to manage a syntax problem. You supposed I don't know when I should write a function. Well, as a matter of fact, the problem is the other way round : I am studying the case when the user knows he does not need any function at that point, and he only needs syntactic sugar.

I will answer more specifically to the "handy embedded if{}else{}" solution in another mail, to make things a little clearer on that point (because there is much to say).

Thanks for your time and propositions,

Best regards,

Pierre Morcello

--- En date de : Mar 8.9.09, Stewart, Robert <Robert.Stewart_at_[hidden]> a écrit :

    De: Stewart, Robert <Robert.Stewart_at_[hidden]>
    Objet: Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?
    À: "boost_at_[hidden]" <boost_at_[hidden]>
    Date: Mardi 8 Septembre 2009, 7h14

    Pierre Morcello wrote:
    >
    > Thanks Michael, I did not think sooner to the 'else if'. But

I like the "else if" version better, too.

    > there is one problem with the 'else if' : you can not do
    > calculations between the tests.
    >
    > When I work often my code looks like :
    >
    > // retrieve meshes from prototype
    > Breakable
    > {
    >     // find loaded meshes
    >     std::set<Mesh*> lLoadedMeshes;
    >     {
    >        // some calculations that fill lLoadedMeshes
    >     }
    >
    >     if(lLoadedMeshes.empty())
    >     {
    >        break;
    >     }
    >
    >     // find loadable scenes that uses the already loaded meshes
    >     std::set<Scene*> lLoadableScenes;
    >     {
    >        // some calculations that fill lLoadableScenes
    >        // it uses the result of lLoadedMeshes
    >     }
    >
    >
    >     if(lLoadedScenes.empty())
    >
    >     {
    >
    >        break;
    >
    >     }
    >
    >     // and so on..
    > }

The whole point of your scheme is to skip subsequent code if you
encounter some sort of error. Therefore, this will do nicely:

    {
       std::set<Mesh*> lLoadedMeshes;
       if (!find_loaded_meshes(lLoadedMeshes))
       {
          // complain and return or throw
       }
       std::set<Scene*> lLoadableScenes;
       if (!find_loadable_scenes(lLoadableScenes, lLoadedMeshes))
       {
          // complain and return or throw
       }
       // and so on..
    }

    bool find_loaded_meshes(std::set<Mesh*> & _meshes)
    {
       // some calculations that fill _meshes
       if (_meshes.empty())
       {
          return false;
       }
       // do more
       return true;
    }

    bool find_loadable_scenes(std::set<Scene*> & _scenes,
       std::set<Mesh*> const & _meshes)
    {
       {
          // some calculations that fill lLoadableScenes
          // it uses the result of _meshes
       }
       if (_scenes.empty())
       {
          return false;
       }
       // do more
       return true;
    }

    You said before that splitting things into multiple functions
    reduced readability. My version is more readable for several
    reasons. Each function has a single purpose rather than one
    function doing many things. Notice that I removed some comments
    in your version because the function names obviated them (self-
    documenting code). Finally, the high level logic, in the initial
    block, is clearer, while the low level logic is segregated into
    separate functions.

If you'd prefer to avoid early returns, "else" is handy:

    {
       std::set<Mesh*> lLoadedMeshes;
       if (!find_loaded_meshes(lLoadedMeshes))
       {
          // complain and return or throw
       }
       else
       {
          std::set<Scene*> lLoadableScenes;
          if (!find_loadable_scenes(lLoadableScenes, lLoadedMeshes))
          {
             // complain and return or throw
          }
          // and so on..
       }
    }

    _____
    Rob Stewart                           robert.stewart_at_[hidden]
    Software Engineer, Core Software      using std::disclaimer;
    Susquehanna International Group, LLP http://www.sig.com

    IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
    _______________________________________________
    Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Then concerning

It also supposes that the find_loaded_meshes function name is a good one.

PS : --In the case of the "find Loaded Meshes", in fact I was thinking to a very special case of a memory manager of a 3D game that is -during loading threaded resources- updating some data before final cleaning of its state. It just means that there is already a findLoadedMeshes() function, for the user of the class, and it uses the cleaned set of information, not the 'dirty' one being loaded as supposed in my example.-- So the name of the function, must be chosen carefully to suppress any doubt from the reader.