Author Topic: A Necessarily Long Function  (Read 9002 times)

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
A Necessarily Long Function
« on: August 26, 2018, 03:05:45 PM »
Since we're on the subject, here's an example of a necessarily long function:

Code: [Select]
void FactoryReport::init()
{
    addControl("btnShowAll", &btnShowAll, 10, 10);
    btnShowAll.font(*MAIN_FONT);
    btnShowAll.size(75, 20);
    btnShowAll.type(Button::BUTTON_TOGGLE);
    btnShowAll.toggle(true);
    btnShowAll.text("All");
    btnShowAll.click().connect(this, &FactoryReport::btnShowAllClicked);

    addControl("btnShowSurface", &btnShowSurface, 87, 10);
    btnShowSurface.font(*MAIN_FONT);
    btnShowSurface.size(75, 20);
    btnShowSurface.type(Button::BUTTON_TOGGLE);
    btnShowSurface.text("Surface");
    btnShowSurface.click().connect(this, &FactoryReport::btnShowSurfaceClicked);

    addControl("btnShowUnderground", &btnShowUnderground, 164, 10);
    btnShowUnderground.font(*MAIN_FONT);
    btnShowUnderground.size(75, 20);
    btnShowUnderground.type(Button::BUTTON_TOGGLE);
    btnShowUnderground.text("Underground");
    btnShowUnderground.click().connect(this, &FactoryReport::btnShowUndergroundClicked);

    addControl("btnShowActive", &btnShowActive, 10, 33);
    btnShowActive.font(*MAIN_FONT);
    btnShowActive.size(75, 20);
    btnShowActive.type(Button::BUTTON_TOGGLE);
    btnShowActive.text("Active");
    btnShowActive.click().connect(this, &FactoryReport::btnShowActiveClicked);

    addControl("btnShowIdle", &btnShowIdle, 87, 33);
    btnShowIdle.font(*MAIN_FONT);
    btnShowIdle.size(75, 20);
    btnShowIdle.type(Button::BUTTON_TOGGLE);
    btnShowIdle.text("Idle");
    btnShowIdle.click().connect(this, &FactoryReport::btnShowIdleClicked);

    addControl("btnShowDisabled", &btnShowDisabled, 164, 33);
    btnShowDisabled.font(*MAIN_FONT);
    btnShowDisabled.size(75, 20);
    btnShowDisabled.type(Button::BUTTON_TOGGLE);
    btnShowDisabled.text("Disabled");
    btnShowDisabled.click().connect(this, &FactoryReport::btnShowDisabledClicked);

    addControl("cboTestBox", &cboFilterByProduct, 250, 33);
    cboFilterByProduct.font(*MAIN_FONT);
    cboFilterByProduct.size(200, 20);

    cboFilterByProduct.addItem("Clothing");
    cboFilterByProduct.addItem("Maintenance Supplies");
    cboFilterByProduct.addItem("Medicine");
    cboFilterByProduct.addItem("Robodigger");
    cboFilterByProduct.addItem("Robodozer");
    cboFilterByProduct.addItem("Roboexplorer");
    cboFilterByProduct.addItem("Robominer");
    cboFilterByProduct.addItem("Road Materials");
    cboFilterByProduct.addItem("Truck");
}

For those whose eyes have crossed, it's a basic UI init function which sets up a UI interface and populated the UI items with necessary information. This isn't finished either but this sort of function is, by necessity, long and annoying. Sure, I could break the function apart into individual functions that add each control but now we're creating a bunch of functions that effectively do the same thing.

Anyway, I suspect that this function will get a lot longer before it's finished. Many more UI elements to put in place.  :'(

Offline lordpalandus

  • Banned
  • Hero Member
  • *****
  • Posts: 825
Re: A Necessarily Long Function
« Reply #1 on: August 26, 2018, 04:44:29 PM »
Each addControl looks like variable declarations for a class. IIRC in C++, can't you declare them all on a single line, comma separating each declaration/initialization? If so that would be more space conservative and shrink the overall size of the function down.
Currently working on Cataclysm of Chaos, Remade.
Link to OPU page = http://forum.outpost2.net/index.php/topic,6073.0.html

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: A Necessarily Long Function
« Reply #2 on: August 26, 2018, 08:52:38 PM »
?? What are you talking about? There are no declarations here, only function calls.
« Last Edit: August 26, 2018, 09:01:22 PM by leeor_net »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: A Necessarily Long Function
« Reply #3 on: August 26, 2018, 11:10:38 PM »
Quote
... but now we're creating a bunch of functions that effectively do the same thing.

Exactly! ;)

So now that we know the common parts versus the parameters, we can begin writing a parameterized initialization function.  :D

In particular, 3 fields are initialized exactly the same in most cases:
Code: [Select]
    btnShowIdle.font(*MAIN_FONT);
    btnShowIdle.size(75, 20);
    btnShowIdle.type(Button::BUTTON_TOGGLE);

Then you have 2 fields which are initialized according to the same pattern, but different parameters:
Code: [Select]
    btnShowAll.text("All");
    btnShowAll.click().connect(this, &FactoryReport::btnShowAllClicked);
You could pass in the text and a handler field. Depending on where the helper method is declared, you might also need to pass in this. I'd recommend putting the helper method on the same class or perhaps more likely, a base class. That would eliminate the need to pass this.

It appears you also have a regular grid layout for the buttons, which could potentially be calculated:
Code: [Select]
x = 10 + 77 * columnX;
y = 10 + 23 * columnY;
This would however require more extensive refactoring. In particular, you could initialize the buttons in a loop, which gets the parameter values from a static const array.

Finally there is duplication with the string ID and the variable name:
Code: [Select]
"btnShowAll", &btnShowAll
This one isn't so bad, and you might want to just leave it in. If it bothers you though, and if you don't find macros to be too evil or forbidden, you could get rid of it using the string pasting operator.

As for the end of the method, with the one off irregular item, you can just leave it as is at the bottom of the intialization method:
Code: [Select]
    addControl("cboTestBox", &cboFilterByProduct, 250, 33);
    cboFilterByProduct.font(*MAIN_FONT);
    cboFilterByProduct.size(200, 20);



You are quite correct in there being no declarations in that code. Though I do admit to having half expected to see a few declarations in there, simply because initialization is often done as part of a declaration. I assume in this case, these are member variables declared in a class header file, where it would have been rather difficult to properly initialize them.
« Last Edit: August 27, 2018, 06:21:03 AM by Hooman »

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1267
Re: A Necessarily Long Function
« Reply #4 on: August 27, 2018, 09:56:33 AM »
If it's auto-gen'd code by the WYSIWYG editor though, then yeah, I don't think anyone should particularly care of it's more verbose than it needs to be, and trying to clean it up will most likely just break the editor anyway.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: A Necessarily Long Function
« Reply #5 on: August 27, 2018, 01:19:44 PM »
String identifier being the same as the variable name is a convention I use and nothing more. It could just as well be button1 - button10 or whatever. Last line is like that due to copy/paste from some demo code I did and forgetting to change the string identifier but it ultimately has no effect on the functionality.

Combining parameters into an initializer function is exactly what I wanted to avoid. There are so many possible configurations that creating a 15 paramter initializer function would make peoples' eyeballs bleed. Additionally I modelled the design from Windows Forms.

All I was trying to illustrate is that some functions are, by necessity, way longer than a page and not due to poor undertanding of a problem or poor programming skills. It doesn't do any real logic, it just sets up a UI. Visual Studio's WYSIWYG editor generates very similar, even more verbose code -- it just hides it better with C# applications (not so much with C++ applications).

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: A Necessarily Long Function
« Reply #6 on: August 28, 2018, 04:38:16 AM »
Hah, I didn't even notice the name discrepancy in the last one. I was mostly looking at the size discrepancy. It also lacks setting type, as well as any code to set text or the click handler.



I would also recommend not using initializer functions with 15 parameters. That would be horrible to use. I was thinking about your class hierarchy a bit yesterday though, and it got me thinking about a few possible alternate solutions.

For instance, you have a Button class, which handles different types of buttons. The type of button is controlled by the mType member variable, which is set by the type method. It seems highly unlikely the type of a button would change after construction. Rather than set the type with a type method, I think it should be set during construction only, with no way to change it afterwards. Now you could do that by moving the initialization to the constructor as a prameter, or you could eliminate the field entirely by creating subclasses of button. Using the subclass approach, the type is then encoded in the type system, using the subclass name, rather than a member variable.
Code: cpp [Select]

ToggleButton irradiateColonists;
Button closeReactorControlWindow;


Another thought I had on a slightly different dimension, is some buttons have only text set, while others have only image set. Every button seems to have at least one of those set. I'm not sure it would make sense to create a button without one of those fields, as otherwise it has no way to indicate to the user what the intent of the button is. I'm not sure if you ever set both, though it seems reasonable. For most buttons, the text or image would remain constant for the lifetime of the button. For a few, it seems reasonable for either the text or an image to be updated after the button is created, perhaps in response to changing conditions.
Code: cpp [Select]

class Button {
// ...
// Makes sense to allow update after construction
void text(const std::string& text);
void image(const std::string& path);
// ...
};


Now, if a button needs at least one of text or image set (and probably only has one set), it makes sense to set it in the constructor. You might even argue that you don't really have a valid button without one of those fields set. To set one of those two fields, the class could offer two constructor overloads. If you're taking the subclass approach, these constructors would likely go in the base class. One complication, is both text and image are currently being set as simple text strings, and so you can't overload on the type alone. That could be fixed though by wrapping one of them in a type, which makes a lot of sense for the image. In particular, the image is already wrapped in a type. When you pass an image path, the Button doesn't directly store the path. Instead, it constructs an Image object using the path. An alternative then, is to accept an Image object as a parameter, and that would then allow for overloaded constructors.
Code: cpp [Select]

class Button {
Button(std::string text);
Button(Image image);
};


Note: The Image object doesn't look overly heavy, so whether you accept it by value (copy constructed), by rvalue reference (move constructed), or by lvalue reference (borrowed) probably doesn't matter.

Another required bit of functionality for a button, is the click handler. If there's no click handler, there's also no point in having a button. This handler is unlikely to change after construction, so it's an ideal field to set in a constructor. I'm not entirely sure of the type you're using here, so I'm just going to pretend it's ClickHandler. I would recommend updating the previous two constructors to the following:
Code: cpp [Select]

class Button {
Button(std::string text, ClickHandler clickHandler);
Button(Image image, ClickHandler clickHandler);
};


Side note: I think the existing click handler setting code needs some slight refactoring to bring it inline with the Law of Demeter. In particular, the button should take a pointer to the handler function directly, and assign that to whatever internal object will call the handler when a click event arises. I don't think it should give direct access to the complicated looking Signal object from NAS2D that raises events. This Signal object is directly exposed by the return value of the click() method, leaving the caller to call connect or disconnect on it themselves. I think that connect and disconnect call should be wrapped into the button itself.

Finally, since it relates, I think purely presentational aspects, such as size and position should not be set in the constructor. Or at the very least, should not be required construtor parameters. There are many ways to set both size and position. They might be hardcoded constants, read from an XML file, calculated by some kind of layout manager, or maybe even uses some kind of CSS like language. They may need to be updated at runtime, particular if window resizing is supported (or will someday be supported). Finally, reasonable values might not be know at the time of control creation. You might need to know how many controls will be displayed before calculating the size and position to display them, and that may require constructing all the controls first. A dynamic display can't update until it knows how many components it has.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: A Necessarily Long Function
« Reply #7 on: August 28, 2018, 04:57:13 AM »
Assuming the above constructor methods, the code from the original post might be rewritten something like this:

FactoryReport.h:
Code: [Select]
class FactoryReport {
private:
  ToggleButton btnShowAll;
  ToggleButton btnShowSurface;
  ToggleButton btnShowUnderground;
  ToggleButton btnShowActive;
  ToggleButton btnShowIdle;
  ToggleButton btnShowDisabled;
  ComboBox cboFilterByProduct;
};

FactoryReport.cpp:
Code: [Select]
FactoryReport::FactoryReport() :
  btnShowAll("All", &FactoryReport::btnShowAllClicked),
  btnShowSurface("Surface", &FactoryReport::btnShowSurfaceClicked),
  btnShowUnderground("Underground", &FactoryReport::btnShowUndergroundClicked),
  btnShowActive("Active", &FactoryReport::btnShowActiveClicked),
  btnShowIdle("Idle", &FactoryReport::btnShowIdleClicked),
  btnShowDisabled("Disabled", &FactoryReport::btnShowDisabledClicked)
{
  // ...
}

void FactoryReport::init()
{
    addControl("btnShowAll", &btnShowAll, 10, 10);
    btnShowAll.font(*MAIN_FONT);
    btnShowAll.size(75, 20);
    btnShowAll.toggle(true);

    addControl("btnShowSurface", &btnShowSurface, 87, 10);
    btnShowSurface.font(*MAIN_FONT);
    btnShowSurface.size(75, 20);

    addControl("btnShowUnderground", &btnShowUnderground, 164, 10);
    btnShowUnderground.font(*MAIN_FONT);
    btnShowUnderground.size(75, 20);

    addControl("btnShowActive", &btnShowActive, 10, 33);
    btnShowActive.font(*MAIN_FONT);
    btnShowActive.size(75, 20);

    addControl("btnShowIdle", &btnShowIdle, 87, 33);
    btnShowIdle.font(*MAIN_FONT);
    btnShowIdle.size(75, 20);

    addControl("btnShowDisabled", &btnShowDisabled, 164, 33);
    btnShowDisabled.font(*MAIN_FONT);
    btnShowDisabled.size(75, 20);

    addControl("cboTestBox", &cboFilterByProduct, 250, 33);
    cboFilterByProduct.font(*MAIN_FONT);
    cboFilterByProduct.size(200, 20);

    cboFilterByProduct.addItem("Clothing");
    cboFilterByProduct.addItem("Maintenance Supplies");
    cboFilterByProduct.addItem("Medicine");
    cboFilterByProduct.addItem("Robodigger");
    cboFilterByProduct.addItem("Robodozer");
    cboFilterByProduct.addItem("Roboexplorer");
    cboFilterByProduct.addItem("Robominer");
    cboFilterByProduct.addItem("Road Materials");
    cboFilterByProduct.addItem("Truck");
}

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: A Necessarily Long Function
« Reply #8 on: August 28, 2018, 07:21:31 PM »
You'd still need a 'this' pointer in order to make said call and have it behave properly -- it's why I use Delegates behind it all.

In general I agree with a lot of what you're saying. Keep in mind that a lot of this I modeled off the code that would be generated by the Visual Studio Forms editor with the idea of 'properties' in mind.

That stated, I'm not opposed overhauling the Control interface but it is a rather lengthy interface to accommodate a lot of potential functionality in derived controls.

Splitting Button into different types makes sense to me -- ToggleButton and ImageButton come to mind here. Eliminates a lot of if/else if/else type of code and reduces overall state size so I'm game. :)

Will slate it for future development though. Button is a relatively simple object and it does the job.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: A Necessarily Long Function
« Reply #9 on: August 29, 2018, 12:50:57 AM »
Good point about this. The Button class is distinct from the FactoryReport class.

Now you've got me thinking if there's a nice handy way to wrap a this pointer with a member function pointer, and maybe even remove the class name duplication. Perhaps it's possible using templates. But perhaps that is getting a bit out of scope.

You're right though, in that this wouldn't be a high priority update. You're also right that the button hierarchy would probably be a good place to start. Should be a reasonably simple and easy to verify change, though it could end up touching quite a few files with minor updates.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: A Necessarily Long Function
« Reply #10 on: August 29, 2018, 06:05:25 PM »
For real-world updates, look at the v0.7.9 branch for OPHD, specifically src/Ui/Core/UIContainer.h and src/Ui/Core/UIContainer.cpp. I've shortened it in a number of ways.

The other stuff you were mentioning would be a different issue, it would basically be an interface overhaul of the entire Control class... which I'm not opposed to but still would do very little to 'shorten' the code that really only sets things up and is called once so does it really matter if it's a little on the long side?

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: A Necessarily Long Function
« Reply #11 on: August 31, 2018, 02:41:13 AM »
I took a look at some of the recent refactoring. I approve of the changes. Some of them really help to cleanup the interface.

And you've got a lot of new commits being added.  :)

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: A Necessarily Long Function
« Reply #12 on: August 31, 2018, 07:07:42 PM »
Yus. After being screamed at and accused of being an asshole by someone I considered a friend (he's the one I was working on that Quake 2 modification for), I opted to cut my losses, avoid the drama (got enough of that in my life) and focus on my own projects instead of being stretched thin across a bunch of projects without any clearly defined goals. I've also taken to heart your suggestion of committing more frequently in smaller chunks so while I'm working for several hours I may commit 10 times or more as I make changes and decide that they're good to go.

And woot! I noticed some ways I could improve interfaces and make code more readable or at least more efficient so I kinda went for it. It's why I take breaks from coding now and then. Sometimes it helps to come back to it with fresh eyes.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: A Necessarily Long Function
« Reply #13 on: November 02, 2018, 10:29:28 PM »
So it's been awhile but I figured I'd chime in here as this is related.

Remember this statement?

Quote
And woot! I noticed some ways I could improve interfaces and make code more readable or at least more efficient so I kinda went for it. It's why I take breaks from coding now and then. Sometimes it helps to come back to it with fresh eyes.

Yeah. That's what resulted in the problem addressed in this post. I still think it was a good idea but with the limitations of the underlying design it blew up in my face.  :o