New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Excess parameters in methods #44

Closed
julianosaless opened this Issue Nov 19, 2014 · 14 comments

Comments

Projects
None yet
6 participants
@julianosaless
Copy link
Contributor

julianosaless commented Nov 19, 2014

When the method has more than three parameters , suggesting a new class.

Before

public void MyMethod(string name,int age,string city,string neighborhood)
{
}

After:

public void MyMethod(NewClass address)
{
}

public class NewClass 
{
    public string Name {get;set;}
    //plus others for age, city, neighborhood
}

Category is Style.
Diagnostic id is CC0044.
This is essentially a refactoring so we will use a Hidden diagnostic.
@julianosaless is working on it.

@carloscds

This comment has been minimized.

Copy link
Contributor

carloscds commented Nov 19, 2014

@julianosaless Hi Juliano, can you explain using a sample,like:

this:
...
become this:
...

@julianosaless

This comment has been minimized.

Copy link
Contributor

julianosaless commented Nov 19, 2014

OK,

When the method has more than three parameters, suggesting a new class.

Before

public void MyMethod(string name,int age,string city,string neighborhood)
{

}

after

public void MyMethod(NewClass address)
{
}

new class.
public class NewClass 
{

}
@marcellalves

This comment has been minimized.

Copy link

marcellalves commented Nov 19, 2014

This could induce the developer to create several VO's without thinking better about the design of the classes.

By the way, it's a interesting feature, and I'd like to see the opinion of the other contributors.

I suggest include the parameters as properties in the generated class, like this:

    public class NewClass 
    {
        public string Name { get; private set; }
        public int Age { get; private set; }
        public string City { get; private set; }
        public string Neighborhood { get; private set; }
    }

Observe that I'm putting the first character of the property name capitalized and with private set.

And, in your example, you put the generated code like this:

public void MyMethod(NewClass address)

But the "address" name is specific for the context of the application. It's impossible for the compiler to know about this. It have to be a more generic name, like this:

public void MyMethod(NewClass newClass)
@julianosaless

This comment has been minimized.

Copy link
Contributor

julianosaless commented Nov 19, 2014

I´m developing this functionality in my fork only to my learning.

@andersonaap

This comment has been minimized.

Copy link

andersonaap commented Nov 20, 2014

Would it be applied to constructors?

@giggio

This comment has been minimized.

Copy link
Member

giggio commented Nov 20, 2014

I see this as a refactoring, not a analysis/code fix. It is a good idea!

@giggio

This comment has been minimized.

Copy link
Member

giggio commented Nov 22, 2014

I updated the description, I think it is Ready. It is up for grabs now. @julianosaless if you want to work on it let us know.

@julianosaless

This comment has been minimized.

Copy link
Contributor

julianosaless commented Nov 23, 2014

Yes giggio, I want to work :)

@giggio

This comment has been minimized.

Copy link
Member

giggio commented Nov 23, 2014

It is yours, @julianosaless. I will move it to Working then.
As this is your first one, don't forget to read the rules for working on the project. If you need help let us know.

@giggio

This comment has been minimized.

Copy link
Member

giggio commented Nov 23, 2014

BTW, @code-cracker/owners this will be our first refactoring. Let's watch closely if there is something there that we need to learn and understand. This PR will have to be closely watched.
No pressure on you, @julianosaless. :)

@giggio giggio added 2 - Working and removed 1 - Ready labels Nov 23, 2014

@viniciushana

This comment has been minimized.

Copy link
Contributor

viniciushana commented Nov 25, 2014

Good to hear it, actually I'm also going to do a refactoring in my current issue. Please let us know any findings or problems you eventually face, @julianosaless !

@giggio

This comment has been minimized.

Copy link
Member

giggio commented Nov 28, 2014

@julianosaless How is this coming? Any idea when it will be done?

@julianosaless

This comment has been minimized.

Copy link
Contributor

julianosaless commented Nov 28, 2014

@giggio I believe that in Saturday .

On Fri, Nov 28, 2014, 2:14 AM Giovanni Bassi notifications@github.com
wrote:

@julianosaless https://github.com/julianosaless How is this coming? Any
idea when it will be done?


Reply to this email directly or view it on GitHub
#44 (comment)
.

@giggio giggio added the code-fix label Dec 1, 2014

@giggio

This comment has been minimized.

Copy link
Member

giggio commented Dec 1, 2014

@julianosaless we are not using refactorings anymore (see issue #66), so this is a Hidden diagnostic. I also updated the description and added category and diagnostic id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment