I'm banning Get, Set, and other method and class names

I originally posted this post on my blog a couple of weeks ago.

I'd like to make a pause from my ongoing Unit Testing 101 series here on dev.to to share a past thought about naming classes and methods.

Names are important in programming. Good names could be the difference between a developer nodding his head in agreement or making funny faces in a "Wait, whaaaat?" moment. Names are so important that books like The Clean Code and The Art of Readable Code devote entire chapters to naming things.

These are some words I'm banning from my method and class names.

1. Get and Set in method names

I wish I could remember what Kevlin Henney's presentation has this idea. He argues that "Get" and "Set" are some words with more meanings in an English dictionary. Then why do we use them in our code when our names should be the least ambiguous as possible? He has a point!

These days I reviewed a Pull Request that had a code block that reminded me about this point. It looked like this,

public record RoomCharge(
    ReceiptId ReceiptId,
    RoomId RoomId,
    ReservationId? ReservationId = null)
{
    public void SetReservationId(ReservationId reservationId)
    //          ^^^^^
    {
        ReservationId = reservationId;
    } 
}

Maybe WithReservationId() or simply ReservationId() would be better alternatives. Often an old auto-implemented property gets our backs covered here.

It says "Don't use Get or Set." No, I'm just kidding...Photo by ZeroQuattro Art on Unsplash

2. Utility and Helper classes

The next names I'm banning are the "Utility" and "Helper" suffixes in class names. Every time I see them, I wonder if the author (and I) missed an opportunity to create domain entities or better named classes.

In one of my client's projects, I had to work with a class that looked like this,

public static class MetadataHelper
//                          ^^^^^
{
    public static void AddFeeRates(Fee fee, PaymentRequest request, IDictionary<string, string> metadata)
    {
        // Doing something with 'fee' and 'request' to populate 'metadata'...
    }

    public static void AddFeeRates(Fee fee, StripePaymentIntent paymentIntent, IDictionary<string, string> metadata)
    {
        // Doing something with 'fee' and 'paymentIntent' to populate 'metadata'...
    }
}

It was a class that generated some payment metadata based on payment fees and input requests. Somebody took the easy route and dumped everything in a static MetadataHelper class.

Instead, we could write a non-static PaymentMetadata class to wrap the metadata dictionary. Like this,

public class PaymentMetadata
{
    private readonly IDictionary<string, string> _metadata;

    public PaymentMetadata(IDictionary<string, string> baseMetadata)
    {
        _metadata = baseMetadata;
    }

    public void AddFeeRates(Fee fee, PaymentRequest request)
    {
        // Doing something with 'fee' and 'request' to expand 'metadata'...
    }

    public void AddFeeRates(Fee fee, StripePaymentIntent paymentIntent)
    {
        // Doing something with 'fee' and 'paymentIntent' to expand 'metadata'...
    }

    public IDictionary<string, string> ToDictionary()
        => _metadata;
}

If a concept is important inside the business domain, we should promote it out of helper and utility classes.

Often, we use Utility and Helper classes to dump all kinds of methods we couldn't find a good place for.

3. Constants classes

This isn't exactly a name. But the last thing I'm banning is Constant classes. I learned this lesson after reading the Domain Modeling Made Functional book.

Recently, I found some code that looked like this,

public static class Constants
{
    public static class TransactionTypeId
    {
        public const int RoomCharge = 1;
        public const int PaymentMethod = 2;
        public const int Tax = 3;
    }

    public const string OtherConstant = "Anything";
    public const string AnythingElse = "AnythingElse";

    // Imagine a whole lot of constants here...
}

It was a class full of unrelated constants. Here, I only showed five of them. Among those, I found the types of transactions in a reservation management system.

On the caller side, a method that expects any of the TransactionTypeId uses an int parameter. For example,

public void ItUsesATransactionTypeId(int transactionTypeId)
//                                   ^^^
{
    // Beep, beep, boop...
}

But, any int won't work. Only those inside the Constants class are the valid ones.

This gets worse when Constant classes start to proliferate, and every project of a solution has its own Constants class. Arrrggggg!

Instead of Constants classes, let's use enums to restrict the values we can pass to methods. Or, at least, let's move the constants closer to where they're expected, not in a catch-all class. With an enum, the compiler helps us to check if we are passing a "good" value.

Using an enum, our previous example looks like this,

public enum TransactionTypeId
{
    RoomCharge,
    PaymentMethod,
    Tax
}

public void ItUsesATransactionTypeId(TransactionTypeId ledgerTypeId)
//                                   ^^^^^^^^^^^^
{
    // Beep, beep, boop...
}

Voilà! These are the names I'm banning in my own code and the code around me. And I wish I could ban them in code reviews too. Are you also guilty of any of the three? I've been there and done that.

---

Hey, there! I'm Cesar, a software engineer and lifelong learner. Visit my Gumroad page to download my ebooks and check my courses.

Happy naming!

Comments

[Jean-Michel 🕵🏻‍♂️ Fayard]

First point is second nature to me as a Kotlin developer because of Kotlin properties

class RoomCharge(
    var reservationId: ReservationId
)

The getXXX setXXX was motivated by the fact than in less than 5% of the cases you will do tricks like delegating and lazy initialization that motivated this verbose convention.

But you should not be too impressed by the "encapsulation" thing, in 95% of the cases we use properties as simple class members.

In Kotlin you can do the advanced things 5% of the cases, but they recognized that what you do 95% of the time should be the default. The syntax is the same in both cases, and it's translated as getReservationId() / setReservationId() at the bytecode level.

But the usage is as simple as it should be

val charge: RoomCharge = TODO()
val i = charge.reservationId
charge.reservationId = i

[Cesar Aguirre]

Good to know! I'm not that familiar with Kotlin

[Jean-Michel 🕵🏻‍♂️ Fayard]

I'm not objective but this is a good resource

How to learn Kotlin: browser vs IDE, books vs tutorials, for newbies and Java devs

Jean-Michel Fayard 🇫🇷🇩🇪🇬🇧🇪🇸🇨🇴 ・ Dec 15 '19 ・ 6 min read

#beginners #kotlin #career #android

[Anthony Fung]

Interesting.

Personally, I don't have a problem with Get... or Set... methods if they have more-than-trivial functionality, e.g. if it sets some state and some calculated state afterwards.

For the example shown, I think ReservationId is better off as a property, or non-nullable constructor parameter.

I'm also unsure how to name a repository method that simply retrieves data from a database without Get. Without specifying whether you are getting or setting data, it's not so obvious if the intention is to retrieve, or add/update a record.

For constants, I completely agree with using enums. But for strings, moving them to the caller side means potential string duplication: once on the caller side, once on the receiving side.

[Cesar Aguirre]

I think ReservationId is better off as a property, or non-nullable constructor parameter

Good point!

I'm also unsure how to name a repository method that simply retrieves data from a database without Get.

Find? :)

...moving them to the caller side means potential string duplication

Let me change the wording

[Anthony Fung]

Find? :)

Ok - that works: it's similar to searching an array in JavaScript. Don't know why I didn't think of that :)

[Kelly Brown]

Can't really get behind #1 at all. Whether you like it or not, "get" and "set" are widely understood terms, especially considering they are keywords for properties. I have seen the talk by Kevlin Henney you are referring to. He does point out that those two words have an abundance of definitions in the English language, but the definitions on the code side are largely settled. You can pick whatever pair of words you want; they're just going to mean the same thing. Read/write. Load/store. Banning them will do nothing but alienate yourself from all teams who would otherwise work with you. You will not open up some new utopia of clearer naming here.

Can't get behind #2 either. Nothing wrong with helper/utility classes. If you're going to introduce a light wrapper, at least make it a struct and avoid pointless heap objects.

I generally agree with #3. Even though you said that the name itself wasn't the problem, I have seen way too many code bases that have a static class named Constants. It's dumb. Separate your code by domain/component/module, not by C# concept. It'd be like having a class for all ints, a class for all strings, a class for all enums, etc. In general, though, I'm fine with a bunch of constants living together as long as they're related.