Whole Tomato Software Forums
Whole Tomato Software Forums
Main Site | Profile | Register | Active Topics | Members | Search | FAQ
User name:
 Password:
Save Password
Forgot your password?

 All Forums
 Visual Assist
 Feature Requests
 'Encapsulate Field' issue&improvement		  New Topic  Reply to Topic
 Printer Friendly
Author Previous Topic Topic Next Topic  

azdroppy
New Member

Cyprus
8 Posts
Posted - Dec 18 2015 :  1:56:12 PM  Show Profile  Reply with Quote
Lets say we have:

class Foo
{
public:
Foo(std::string name)
: _name{ std::move(name) }
{
}
private:
std::string _name;
};

After applying 'Encapsulate field' to _name member resulting code looks the following way:

class Foo
{
public:
Foo(std::string name)
: Name(){std::move(name)}
{
}
std::string Name() const { return _name; }
void Name(std::string val) { _name = val; }
private:
std::string _name;
};

1. Name(){std::move(name)} - is incorrect
2. void Name(std::string val) { _name = val; } - is sub-optimal. It would be great to have a possibility to configure default behavior - either pass argument as const ref (if the object being encapsulated is a class) or use _name = std::move(val) if the object is movable.

Due to the issues mentioned 'Encapsulate Field' is not usable now as it requires too many manual code changes after apply

azdroppy
New Member

Cyprus
8 Posts
Posted - Dec 18 2015 :  1:57:53 PM  Show Profile  Reply with Quote
Forgot to mention third issue:
std::string Name() const { return _name; }

it would be great to have an option to generate

const std::string& Name() const noexcept { return _name; }

instead or even

constexpr std::string Name() const noexcept { return _name; }

if the class ctor is 'constexpr' too
Go to Top of Page

feline
Whole Tomato Software

United Kingdom
19147 Posts
Posted - Dec 19 2015 :  10:44:50 PM  Show Profile  Reply with Quote
Thank you for the clear example, I am seeing the problem with the constructor being updated incorrectly, and I have put in a bug report for this:

case=93992

When you trigger Encapsulate Field, the bottom portion of the Encapsulate Field dialog shows you a list of all of the references that are going to be updated. If you can easily spot the problem constructor initilizer list entry in the list, you can simply untick it in this list, and then this reference won't be updated, but the other changes will be made. Or you can simply untick the project or file nodes, to stop any code updates, just leaving you with VA generating the new functions, but not actually calling them.


For your second point, if you look in the VA Snippet Editor, set the type to Refactoring, and look at the Encapsulate Field snippet, you can make some edits to the code that VA will generate. You can certainly add const to the setter here. Adding "var" here might be unwise, since any changes you make here will always be applied. I cannot think of any reason not to always make the setter parameter const, so this change seems safe enough.

Accessing the snippet editor - http://docs.wholetomato.com/default.asp?W392
the new snippet editor - http://docs.wholetomato.com/default.asp?W476

We are considering passing items by reference for Encapsulate Field, but need to be careful, so that we are reliable in deciding when it is correct to do this:

case=7382

Working out if the object is movable, is there actually an easy way of working this out? I have done a bit of searching for this, since it's not something I know about, but am disappearing down a rabbit hole here.

The concept makes sense, but it does not seem that its safe to just blindly add std::move to everything, but I am not seeing anything so far to help guide me in working out when it is safe to use.


For your second post, you can add the extra const to the getter by simply editing the Encapsulate Field snippet, as above

I am not sure about the "constexpr" but though. As soon as you consider template types, you can have multiple constructors. If you start thinking about std::move, then it may not be just the main class constructor you need to consider either. I don't want VA adding something that it should not be adding, so we would need to be sure.
zen is the art of being at one with the two'ness
Go to Top of Page

sean
Whole Tomato Software

USA
2817 Posts
Posted - Mar 16 2016 :  12:14:15 AM  Show Profile  Reply with Quote
case=93992 is fixed in build 2093
Go to Top of Page
  Previous Topic Topic Next Topic  
 New Topic  Reply to Topic
 Printer Friendly
Jump To:
© 2023 Whole Tomato Software, LLC Go To Top Of Page
Snitz Forums 2000