Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm working on a project where exist one method with title

string ValidateNewPasswordExpireCurrentPasswordAndCreateNewPassword(...)

I'm sure the method name must be changed. But can't found good and descriptive alternative. Also I can't separate method in multiple methods, like ValidateNewPassword(), ExpireCurrentPassword() and CreateNewPassword().

How can I rename the method and the same time to not lose self describing ability ?

share|improve this question

closed as not constructive by Walter, Glenn Nelson, MichaelT, GlenH7, Eric King Feb 1 '13 at 15:12

As it currently stands, this question is not a good fit for our Q&A format. We expect answers to be supported by facts, references, or expertise, but this question will likely solicit debate, arguments, polling, or extended discussion. If you feel that this question can be improved and possibly reopened, visit the help center for guidance.If this question can be reworded to fit the rules in the help center, please edit the question.

3  
How about UpdatePasswordIfValid ? –  NikolaiDante Feb 1 '13 at 13:52
9  
I have to ask the question...Why can't you seperate the logic into seperate methods? If A method does multiple things then it is a bad method and should be designed the correct way. If you must keep a single method then call it UpdatePassword then call the three seperate methods within it. I could accept that since the "single" action is updating and creating a password. –  Ramhound Feb 1 '13 at 13:52
7  
If the name is proper for what it does, a too long name simply means it does too much. –  Thijs van Dien Feb 1 '13 at 14:05
2  
@Arterius: As a code reviewer, I'd say your responsibility here is to identify a method that does too much. If that's not acceptable for the organization, then what's wrong with a long name so long as it accurately self-describes? Is the length of the name causing any specific problems that are greater than the problems generated by a method that doesn't follow SOLID principles? –  Joel Etherton Feb 1 '13 at 14:40
1  
@Arterius Agree 100% with Joel Etherton - as a code reviewer, you should not be failing this code based on the name itself, but what the name connotes; that is, violating an aspect of the Single Responsibility Principle. I think you should point the author to this great Uncle Bob article: sites.google.com/site/unclebobconsultingllc/… –  Jesse C. Slicer Feb 1 '13 at 14:45
show 5 more comments

3 Answers

up vote 7 down vote accepted

I expect this method to call three other methods:

public string ValidateNewPasswordExpireCurrentPasswordAndCreateNewPassword(string password)
{
    AccountManager.ValidatePassword(password);
    AccountManager.ExpirePasswordForAccount(this.CurrentAccount);
    AccountManager.AssignPasswordToAccount(this.CurrentAccount, password);
}

When a method aggregates different things, it's done because:

  1. Either it's too cumbersome to call those different things in order every time you need them, i.e. the caller don't want to remember that the password should be validated, then the old one should be expired and only then the new one should be assigned.

    The caller probably needs to know what's happening under the hood, since the same caller can call those three methods separately. You can try to shorten the method name, but not too much: sadly, you still need to be very specific and detailed.

    You can also move the hint that the password is validated to the password argument itself.

    public string ExpireAndReplacePassword(string passwordToValidate)
    {
        AccountManager.ValidatePassword(passwordToValidate);
        AccountManager.ExpirePasswordForAccount(this.CurrentAccount);
        AccountManager.AssignPasswordToAccount(this.CurrentAccount, passwordToValidate);
    }
    
  2. Or you don't trust the caller: what if the caller just assigns an invalid password?

    In this case, the caller doesn't have to know the internals, so be short. You don't need to specify that there is input validation: you're expected to validate inputs from an unstrusted caller.

    public void ReplacePassword(string newPassword)
    {
        Contract.Requires(AccountManager.IsPasswordValid(newPassword));
    
        AccountManager.ExpirePasswordForAccount(this.CurrentAccount);
        AccountManager.AssignPasswordToAccount(this.CurrentAccount, newPassword);
    }
    

    or, if there is still a reason to tell that the old password will expire:

    public string ExpireAndReplacePassword(string newPassword)
    {
        Contract.Requires(AccountManager.IsPasswordValid(newPassword));
    
        AccountManager.ExpirePasswordForAccount(this.CurrentAccount);
        AccountManager.AssignPasswordToAccount(this.CurrentAccount, newPassword);
    }
    
  3. Or business-wise, separately, those actions don't make sense.

    In the third case, if only the aggregation makes sense business-wise, you can simply use the verb which is used in this domain. For example, if the users don't expire their passwords, nor validate them, nor assign them, but simply change them, the change implying the validation, the expiration and the assignment, call the method simply:

    public string ChangePassword(string password)
    {
        Contract.Requires(AccountManager.IsPasswordValid(password));
    
        AccountManager.ExpirePasswordForAccount(this.CurrentAccount);
        AccountManager.AssignPasswordToAccount(this.CurrentAccount, password);
    }
    
share|improve this answer
add comment
string ChangePassword(...)
{
    if(ValidateNewPassword(...))
    {
        ExpireCurrentPassword();
        CreateNewPassword();
        return ... //Method signature is string?
    }
    else
    {
        //Handle invalid password
    }
}

Much cleaner IMO!

share|improve this answer
add comment

As mentioned by others I would go with UpdatePassword or RenewPassword as both imply that a password mus exist and that actions are being carried out on it which may or may not change it. The best approach for me personally in such situations has been to rename the function then wrap the function in the old name as well, leaving legacy compatibility in place for a while, and having a log message outputting the new name of the modernised function. That way you can allow programmers time to get accustomed to the new Name without breaking the codebase, and coincidentally saving you having to make plenty of refactoring code changes yourself.

share|improve this answer
2  
Note that in .NET, [Obsolete] attribute is used to mark a method as obsolete and generate a warning when compiling. Using logs for that is unexpected and should not be done in .NET. –  MainMa Feb 1 '13 at 14:43
    
Agreed, the log functionality is purely an additional step to ensure that everything is documented. Cheers for pointing that out. –  GMasucci Feb 1 '13 at 15:04
add comment

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