If a instance method is used in one place only, should I move that method to that place?

Question

For example, suppose I have a ZipCode class:

public class ZipCode{
    public value String value;

    public boolean validateFormat(){ 
        ...
    }

    public otherMethod1(){
        ...
    }
}

I found many classes uses ZipCode instances, but validateFormat() is used in one place only : register page:

public class RegisterPage{
    public void submit(){
        ...
        ZipCode z=new ZipCode();
        z.value=zipCodeTextField.getText();
        if(z.validateFormat()){
        } 
        ...
    }
}

So my question is, should I move the codes in validateFormat() into submit(), in order to fit YAGNI rule?

Answer 1

The question you ask is generalized, but the issue here is not what you think it is. The majority of this answer focuses on the real issue, but I want to respond to your direct question first.

So my question is, should I move the codes in validateFormat() into submit(), in order to fit YAGNI rule?

Unless your validation logic is complex enough to warrant its own class, or you're specifically striving for an anemic data model, I don't see a need to separate the logic here.

It's not so much YAGNI, but more a consideration of why you have the validation. Based on your code, the validation exists specifically to prevent creating invalid zip codes. This means that the existence of an object and the validation of its input are closely tied together, and the validation merely exists to support the existence of the object.

If that is the case, then it's not wrong to keep these two together, as long as the validation does not far outweigh the complexity of the ZipCode class. For example, a simple "has 4 digits" check is fine. A complex algorithm that performs checksum calculations and queries external resources is not good to put in your class.

If a instance method is used in one place only, should I move that method to that place?

If Bob is the only one that drinks coffee in the office, should we put the coffee maker on Bob's desk? The good practice answer is "no". The coffee maker belongs in the kitchen for a good reason.

• We might hire a new employee who also drinks coffee (= new code that uses the validation).
• Bob needs his desk space for things related to his work (= SRP).
• If maintenance needs to repair the machine, they're going to be looking for it with the other drink machines. (= other developers won't always know where to look).

---

Your code is putting the cart before the horse. While I do admit this issue is currently more on principle, it is part of the root of your problem.

//Make a new zip code
ZipCode z=new ZipCode();
z.value=zipCodeTextField.getText();

//Is this zipcode a valid zipcode?
if(z.validateFormat()){

It doesn't quite make sense to first make something a zip code, and then retroactively check if it's actually a (valid) zip code. The consequence is that you've created a system where a ZipCode object can be valid or invalid. It's going to lead to you second-guessing ZipCode objects all over your codebase, often repeated the same validity check

You should invert that logic:

//Is this value a valid zipcode?
// YES => Then make it into a zipcode object.
// NO  => We refuse to make an invalid zipcode object.

This means that the existence of a ZipCode object inherently tells you that it's a valid zip code.

---

There are two ways to achieve this. I've heard arguments pro/con either way, so I'm just presenting them both.

1. Constructor validation.

Example:

public class ZipCode
{
    public string Value { get; private set; }

    public ZipCode(string value)
    {
        if(!validateFormat(value))
            throw new Exception($"{value} is not a valid zip code format.!");

        this.Value = value;
    }

    private boolean validateFormat(){ 
        ...
    }
}

Pro:

• You guarantee that no one can create a ZipCode object that contains an invalid zip code.
• Any subsequent code that handles a ZipCode object can blindly rely on having received a valid zip code.

Con:

• Some developers really dislike throwing exceptions. I have a more nuanced view, and consider exceptions appropriate here as they are a last resort refusal to create an invalid object.
• You can't easily (de)serialize objects when they don't have a parameterless constructor.

2. Property validation.

Example:

public class ZipCode
{
    private string _value;
    public string Value
    {
        get { return this._value; }
        set
        {
            if(!validateFormat(value))
                throw new Exception($"{value} is not a valid zip code format.!");

            this.Value = value;
        }
    }

    private boolean validateFormat(){ 
        ...
    }
}

Pro:

• You guarantee that no one can set a ZipCode's value in a way that it contains an invalid zip code.
• Any subsequent code that handles a ZipCode object can pretty much rely on having received a valid zip code. There are caveats here, which I will point out in the "con".
• Compared to the constructor validation, you are now able to (de)serialize this object since it has a parameterless constructor.

Con:

• If you do new ZipCode() and never set the property, you will still have an invalid value in your ZipCode.
  • If you're okay with having a "default null" value, you should really consider using a Nullable<ZipCode> (or ZipCode?) instead of a ZipCode with a possible invalid state.
  • If you're not okay with having a "default null" value, then you can combine constructor validation and property validation in order to allow people to change the value to other valid values.
  • However, if you need a parameterless constructor (e.g. for deserialization), you're either going to have to define a default value for the property, or accept that it may be null if no one has hever set the property.
• Some developers really dislike throwing exceptions. I have a more nuanced view, and consider exceptions appropriate here as they are a last resort refusal to create an invalid object.

3. Validate before you create.

This effectively makes it so that you separate your zip code from your zip code validation.

Example:

public static class ZipCodeValidator
{
    public static boolean Validate(string value){ 
        ...
    }
}

And then:

public void submit()
{
    string zipCodeValue = zipCodeTextField.getText();

    if(ZipCodeValidator.Validate(zipCodeValue))
    {
        var zipCode = new ZipCode() { Value = zipCodeValue };
    } 
}

Pro:

• The current code ensures that you only create a ZipCode when the supplied value is valid.
• This allows you to separate the object from its validation. However, I'm not quite convinced that that's necessary to do so, unless your validation logic is excessively massive (e.g. doing online lookups or complex calculations).

Con:

• Nothing is preventing a developer from (mistakenly) creating a ZipCode object and forgetting to validate the input at some other point in time (or when making a change to the existing code). It's no longer a guarantee that ZipCode objects are guaranteed to be valid.

For 3., you could've just as well put this static method in the class. It doesn't matter on a technical level. And the same argumetn applies here; if the validation exists purely to authorize the creation of a ZipCode, and it's not overly long winded or complex, then it's reasonable to keep it in the ZipCode class.

Answer 2

The point of functions and methods is not just to reduce repetition, it is to structure your code. Here you have some functionality that is not repeated. But since it is conceptually about ZIP codes, you may want to keep it near other ZIP-code related functionality. So having a validation method in a ZipCode class looks fine.

However, your particular design looks a bit weird: why do you have to create a new empty object, then assign a public field, before you can invoke that method? It might be much better to create a public static ZipCode.isValid(text) method. (This also reads much more naturally.)

Arguably, when you create an object it should always be in a valid state. In that mindset, creating an empty object or setting the object contents to an invalid state makes no sense. Instead, the constructor would validate its input, and you can only change the object state through methods that check validity. No non-final public fields! If validity is violated the methods can throw an exception.