Wednesday, April 6, 2022

Code Smells Listicle

 After many times looking up various resources on code smells and code smell taxonomies, I finally decided to make a listicle (list article) of these. 

Enjoy:

On the "positive" side of the ledger, we also have virtues:
  • The original Code Virtues article from Pragmatic Programmers, republished at Medium.
  • The explainer article (feel free to start here) at Industrial Logic.

Monday, March 14, 2022

What does Tim have against "private" methods?

 A big thread erupted, all full of misunderstandings and miscommunications, about the idea of "private" methods. 

It all started quite innocently (I maintain) when someone asked how we felt about testing private methods. Some people jumped in with "Absolutely Not! Never! That's wrong! Test via public interfaces."

I thought a little longer, and said "I'm not sure" and then later "I'm not sure that 'private' is even needed."

This is where the problems started, and maybe here I can clarify what I meant by it all. 

People assume (and insist) that I could only possibly mean that they should substitute 'public' for all protected and private members, polluting the interface, and inviting the violation of a class' internal state. 

That was never my intention and still isn't. Still, this is what people insist that I must have meant from the start. I suppose this is because that's what they imagined me to mean and it's hard to admit that you're wrong, or perhaps because this is social media so people never miss an opportunity to double-down and pile on if at all possible. It's like a sport. 

It's also entirely possible that I didn't communicate well at all in short tweets. Maybe that's all on me. I can't say.

Well, you've come this far, so let's see if I can't communicate better in blog than in twitter, since I can write in peace without people accusing me of meaning the wrong thing while I try to explain what I actually mean.

If I've managed to make sense with this post, let me know. 

If I've not, the forum is open for clarifying questions below.

Autocomplete is a big deal.

The good thing about 'private' and 'protected' -- the thing that actually makes programming easier/better, is that the methods aren't offered by auto-complete. The idea of locking people out of calling the method? Not even remotely a second-degree concern.

People program by autocomplete. You type the name of a thing, press the dot, and you get a list of things you can do. You may never look at the documentation, or read the example code, but you will see the autocomplete hundreds of times a day.

When you want to disable an account, there is likely to be a 'disable' method attached to an account. You choose that method, run the tests, ship it. That's how you normally will navigate.

When you don't see a method you want, you might pop up a level and use the module or some management API interface. Again, you type the name and a dot, and you look for 'disable' in the recommended names. 

Private and Protected are two of the ways to keep functions out of that list, and if they're not in the list, you're not likely to even consider calling them.

Arbitrarily Located Functions

A developer is implementing a class and realizes that they need to compare date ranges. They write that up, drop it into a private method, and all is well.

Or is it? 

Why is that method located in this class? Does it have high cohesion? No. Does it support the purpose of this class directly? No. 

Why then? Because this class calls the function, and this is the file I was editing when I wrote the function.

In other words, the function doesn't belong here according to any rule of design, but it wasn't found anywhere else either. The developer drops it into the class with 'private' and moves on.

What I've found is that often the private methods in one class are repeatedly reinvented in private methods of other classes. Sometimes they're even copied and pasted directly from one class to another.

Why? Because this is the class they had open in the editor when they realized they needed the function, and it's not available for calling because it's private in the other class.  So they copied it.

Arbitrarily located functions, hidden from view, repeated by invention and copying, likely hiding the opportunity for Single Point Of Truth (SPOT) abstractions and libraries that would make everyone's job a little easier.

This isn't a rare occurrence. 

By being hidden and arbitrarily located, they invite duplication and reinvention. 

Testability of Private Methods? None

When we talk about testing private methods, it's a non-starter. If you put the word 'private' on the front of a method in most languages, you cannot call that method from a test. That means you can't possibly test private methods.

Okay, there is a way, but it involves reflection and that's an obscenity. If you ever use reflection to crack into some legacy code and test hidden methods, make sure you remove that hack as soon as humanly possible (or sooner) because it's a travesty. It's awful. It will fail you if the method is renamed or moved. Reflection is generally a bad idea, and I wouldn't do that (much, or for long).

If you are doing TDD (and why wouldn't you?) you test the public methods, and if they're using private methods then the private methods are being exercised. If you have meaningful tests and assertions, then the private methods are being tested through the public interface and that's probably okay.

If you're not doing TDD (and why not?) then you may not have code that tests all the private behaviors. You may have to read all the code in order to exercise it properly, which is a pain.

If you're doing test-after development (but why?) you've written the code and now you have to write tests around it. Every private method is a called from some public method and you're going to have to figure out how to get down into that private method code and recognize if it's working correctly or not.

By being private, those methods have cut off the easiest route to testability -- calling the method directly.


Should You Test Implementation?

You should: implementation is how code behaves and you have to test behavior.

There are caveats here:

  • If you're structure-aware in your tests, they'll resist structural refactoring
  • If you're time-sequence-aware in your tests, they'll resist time-order refactoring
  • If you're using reflection, you're poisoning our future and must be stopped (half-grin)

There are rules, of course. You don't lock down things that you want to change, and you don't leave unspoken the things that must be true. Your tests specify how your world behaves at a certain level.

Sometimes that's too high a level and there are mini-behaviors you need to check too.

I've seen cases where testing only at the interface required hundreds of tests, but testing at the internal level requires barely a dozen. This is because at the higher level, you need all the combinations of all the paths of all the subordinate functions. At lower levels, you need one for each path in a function.

High-level end-to-end and integration tests take a long time to run, and microtests are cheap and fast.

We tend to do microtesting (or unit testing) so that we can afford to run the tests in a tight TDD cycle. 

To argue against testing implementation functions is to argue against unit testing and microtesting on principle.

If you should only use the most-public interface, then shouldn't you only test at the UI level? Yeah, this is not a good idea. 

So how can we get at the lower-level functions? If they're all private, we can't but it is totally possible if they are public methods on lower-level classes that are composed ("encapsulated") by the API classes. 


Interfaces and Implementations

In many languages, you can declare a public interface, and that interface can be implemented by other classes.

It's accepted that one should always program to the interface in those languages. This way, one can only use the declared, public interface of the implementations. This keeps the implementations substitutable (via Liskov Substitution Principle). 

Non-interface methods of the implementers are already hidden: callers have no access to them via the interface. 

If you're using the interface, and you press the dot, then only methods in the public interface are presented. The other methods of the implementer may as well not exist, because they are not available here.

If you have segregated interfaces, this is even nicer. It may be that two or three public interfaces are the right way to think and design interactions, but combining a few of those interfaces together is the better way to implement the behaviors. No user of one interface has any awareness that the other interfaces exist on the object, nor of the public methods of the other interfaces, nor any of the public methods of the implementers.

In order to reach the public methods of the implementer, a caller would have to down-cast from the interface to the implementation (a no-no that justifies a sharp crack across the knuckles with a yardstick) in order to even know that other methods exist. 

This is convenient. This means that all methods of the implementers can be public without exposing those methods to callers.  Since they're public, you can write tests directly against them. Because the implementation is being tested also through the interface, it's easy to ensure that it behaves as a whole implementation.

Why not make them private? Because it's redundant and limits testing. 

Without polluting the public interface, one has a fully open and testable class.


Thinking a little more deeply

Private methods, when we choose to use them, may signal a need for us to think more deeply about our situation and strategies.

  1. In a Rat's Nest:
    sometimes people are doing test-last (after coding) and they have a complex method. You shouldn't have big, complex methods if you have any other choice. To try to manage the complexity, they may extract some private methods.
    In order to test through the API, you have to navigate the rat's nest. You will have to set up deep data structures and some combination of boolean conditions that will allow you to get to the private method's function call, and then to exercise the code inside that method.
    It's easier to make the method's access less restricted and test it directly than to have dozens of long and complicated tests. Perhaps package-public, perhaps protected so you can cheat with inherit-to-expose. 
    It's better to test the code easily than to write awful, fragile, internals-aware tests.
  2. Missed Abstractions
    Where one class has a bunch of private methods, often there is some cohesion between those methods. If one were to set the private methods side-by-side, one might recognize that there have been missed abstractions.
    Maybe some of the methods are generic string, date, or math functions. These could be public methods in a utility package or more-primitive type. If you move them to the "right" place, they can be fully testable and can be reused within the codebase. They would not be public methods of the class you're working on.
    Perhaps some of those represent a lower-level concept that is munged into the current class. If they were pulled out, they could become testable, public methods on the new class. They would still not appear in the public interface of the class you're working on.
  3. In Languages without Private
    In Python, smalltalk, and similar languages there's no 'private' and we've been okay with that for a long time (since the 70s for Smalltalk). 
    While people say that encapsulation is a core feature of OOD (and it is) it isn't the "private" keyword that causes encapsulation. It's done via composition.
    In a Python module, you can choose what classes you expose and which you don't. You have a class with a public API, which is composed of classes/functions that aren't part of the public API. They're tested directly within the module and don't have a "private" keyword associated with them. They might not have underscore-decorated names or any other semblance of access protections. 
  4. Other APIs
    In any language, the Model class or the API class may have a simple interface into the module (as described above) but the module may have a lot of complicated functions and logic divided into multiple classes and functions. 
    The model class doesn't have any 'private' methods at all - it just calls the public methods of other classes in the module. Those classes have copious tests and may not have any methods declared Private or Protected, because only the API and tests call those methods - they're not visible to the outside world (protection from Hyrum's Law)

So What Do You Do Instead

  • Don't pollute the public interface of a class with private methods. If you simply flip the private methods to be public, you'll lose understandability of the interface and create unwanted dependencies. This is what I recommend not doing. Keep interfaces clear and clean.
  • Move your methods to the places where they belong (where they have cohesion) and can be easily found by others.
  • Try not to need private methods. They should be rare. Remember, many OO languages don't even have the concept of private and they're just fine without them.
  • in some legacy code test-after situations, you might raise accessibility of a method to public, protected, or module-private in order to support refactoring via better tests (in the short term)
  • Consider using encapsulation properly: by composing behaviors under an API, rather than by housing all behaviors in the API's class.
  • In some languages, you have 'interface' or 'protocol' classes that declare an interface to use. Do that when there is a clear public interface. 
And, of course, I would be remiss if I didn't admit that I do sometimes create private methods. I try not to, and when I find a better way than settling on private, I am usually happier.  I sometimes leave a little cruft like private methods until I have more information and can see a better design form; it may take weeks or months. 

So yeah, there are private methods in my code. I just don't see that as "good coding" and a "solution." It's temporary.

Thursday, March 3, 2022

Splitting Stories - A Resource List

 I've noticed that for several years now, one of the most frequently asked questions in agile forums deals with the splitting of stories. 

We simply can't create a feature, epic, or improvement in a single gesture. If we are going to make progress, we have to start somewhere and build in small pieces. One way or another, whether we release after each piece or not, we have to make progress in bits and pieces. 

In the bad old waterfall days, the analysts and designers would come up with a system with functional decomposition. It would essentially describe a tree with upper-level modules calling lower-level modules all the way down to individual functions.

After the designers have done a top-down design, the developers would start building the system starting from the bottom-up. They would build the pieces and the higher-level pieces that use those pieces until they had components, sub-assemblies, subsystems, and eventually the entire product. 

This "top-down design, bottom-up implementation" had the advantage that pieces could be farmed out or assigned to a lot of individuals and as the parts started to integrate errors could be discovered. Sadly, full integration was one of the last activities possible in the system, and really large errors could be discovered late in the process. 

We tend to see that whole system - a master designer surrounded by minion "brick-makers" -- to be fraught with peril. There are just too many ways for the work to be wrong or late, and too little engagement of the intelligence of all the brick-building developers. These kinds of systems fail much more often than agile ways of working, according to Standish reports.

We find working in end-to-end slices to be superior as far as engaging the intelligence of our people, getting integration (and therefore bug detection) early and often, and in our ability to demonstrate a running, working system at all times. Rather than having 100% of the system N% complete and little to demonstrate, inspect, or use to acquire feedback, we always have N% of the system 100% done and demonstratable. 

This is a political and technical improvement in the way of working. Where it is practiced, teams are more successful in building products that satisfy the needs of the product community.

But how does one do that?

Here are some writeups that I like and recommend:

Check out the 5 selected articles from Agile Alliance while you're at it.



Friday, February 18, 2022

Maximize Value, not Quantity

I was chatting with a manager who was once a PO on a team I coached many years ago. This is only my best memory of the conversation (I didn't record it at the time). I may have slightly embellished it with snippets from conversations that followed over the course of days or weeks, but I try to be faithful.

One day she took me aside and asked what she should be doing to accelerate the team and get more work pushed through.

“Nothing,” I said.

“Nothing? But I thought that I’m supposed to be getting maximum work and speeding up the team…?”

“Nope. Your job as PO is to maximize the value of the work, not the quantity of the work. Given that they’re doing roughly the same amount of work each week (barring emergencies and vacations), your job is to make sure that what they are working on is the most valuable work that they could do that week - that it is impactful and useful.”

The Product Owner is accountable for maximizing the value of the product resulting from the work of the Scrum Team. - the scrum guides

“I was told that I’m supposed to push for more, and there was a healthy tension between PO pushing for more and SM pushing back.”

“Yeah. That’s nonsense. You’re all on the same team and should be all working for the same results. If you push people beyond their capacity to work, they’ll turn in crappy work and that won't be the best value for time spent. Don’t do that.”

“OMG! That’s GREAT! I was so worried because I don’t know how to make people work harder. All I have to do is deal with content and not execution?”

“Yep. That’s it.”

“That’s incredibly good news. I just felt a lot of stress roll off of my shoulders. That’s great! I can totally do this job!”

And she did. Soon she was known for being the best PO in the building, having the best teams, having solid relationships with her teams, and making wise decisions.

Her career as a product manager was launched there and has progressed to this day.

PO/PM/etc:
Your job is not to fight your team. You’re all supposed to work together on this thing.

Tuesday, February 15, 2022

Can teams be accountable for delivery of features?



Including delivery/deployment in the definition of done I'm told is unfair because teams aren't in control of what gets delivered or when.
  • "Their work might be completed, but be only a fraction of some larger scatter-gathered effort, so it's not their fault."
  • "They may be dependent on work from another group, say FE or BE or database, or something, so it's not their fault the work isn't done."
  • "They may have worked on a dozen things, but only one was delivered. It's unfair that it doesn't represent all of their efforts."
  • "Releases aren't done every sprint/increment/week/whatever, so it will look like uneven velocity if we only count work actually delivered."
  • "They should be able to count the points for everything they worked on, whether it's delivered or not."
  • "A manager may decide not to release their feature and leave it in the branch -- maybe never release it. It's not their fault so they should be able to count the work they did that was canceled."
  • "There could be technical issues that make the feature un-deliverable. How will they count their points if the feature can't be completed?"
  • "The testing departments could return their code for rework, and that would keep them from counting all the work that they did on the feature."
  • "It's up to the customer to accept the work, and if they don't then none of the efforts will count."
All of these say "it seems unfair to count accomplishments when the system works against accomplishing things, so we want to honor and respect the busyness of the team members instead."
 
This is what I mean by "busyness accounting" -- focusing on how busy people are, not on whether we're reaching our goals. It seems more "fair" if we are evaluating people on how *much* they do instead of what value is delivered.
 
Busyness accounting arises naturally and organically when we're tracking people in the system instead of tracking the flow of work through the system, and when we're focused on people "scoring points" of velocity instead of delivering value.

Go back and notice the helplessness in each of the complaints/reasons to not count delivery.

"Our system is dysfunctional and resists our best efforts, so we count how hard we try instead."

A better system might change everything.
 
If only delivery/achievement counted, how would you rework the way you are doing software today?

What processes would you change, eliminate, or trade out?

What would you do instead?
 
What if it's not impossible?

Could you work in a world where the rules are different?


this post is recreated from a Twitter thread from December 2021

Monday, October 11, 2021

The inefficiency of tests

So a given web application has an architecture that involves a UI and an API and under that some domain objects, data, etc.

When a new feature comes up, there is a gherkin test that does NOT go through selenium, but directly to the API. In developing the gherkin test, the team drives out the changes needed by the API and gets it working.

The gherkin test is a "story test" and checks to see if the system (behind the UI) works correctly. It does data, persistence, etc in a safe way.

But to build the code, you are doing TDD directly on the API and the deeper domain objects. As you do, you are refactoring and committing (of course).

The microtests and the gherkin tests together are super-fast, so you run both in the auto tester. The auto tester re-runs the tests any time the code is changed and compile-able. This means the tests are run many sometimes more than once a minute. You're always clear where you stand.

But of course, there is a web page to deal with. You create render tests that will turn data (specifically the kind needed or returned by the API) into a web page. 

Now, these are back-end rendered old-school style. You get that working correctly.

It works, but is it ugly? Is it awful? Sigh.

Okay, you fire up the webserver and look at the page, and sure it's ugly. You work on the CSS and make it look better. You're using a development web server that reloads the page when the HTML or CSS changes so fixing the aesthetics and flow is fairly quick (if tedious).

But where is your automated test to be sure the wiring between the web page and the API is working correctly? 

Okay, you write some selenium tests. Just the necessary ones, maybe just automating tests you were doing by hand to check out the web page(s) rendering(s).

It just so happens that someone built a testing tool that pretends to be the webserver! You write some HTTP-level tests that do the GET, POST, PUT, DELETE and they run quite fast and give you confidence. You might eventually replace the selenium tests with HTTP-level tests but for now you leave some of them in place.

Hey, the app works! You deploy

You're ready to move on to the next feature, but your friend brings up a problem. 

Your friend mentions that some of the code exercised and checked by the UI test is also checked by other tests.

The render tests check the DOM, the API tests check message-passing, the microtests check underlying functions, the UI tests are end-to-end: while there are many different tests, many code paths are indeed covered by integration tests and end-to-end tests as well.

Your friend says "this is inefficient because the same code is getting coverage more than once."

You smile.

You know it's not inefficient, because you aren't measuring efficiency by total lines written or the number of times you had to run the tests, or by some standard of minimal coverage.

You know that human efficiency is high - it's a quick, reliable, and safe way to create a system that can be safely refactored, extended, modified.

You value safety over efficiency, and efficiency is a side-effect.

Friday, September 10, 2021

FIRST: an idea that ran away from home

Quite some years ago, Brett Schuchert and I invented the acronym FIRST for micro-tests, though we called them "unit tests" (as was common at the time). This was later included in Agile In A Flash and was also published in a Pragmatic Programmers' article.

It’s grown a following and has been widely repeated. At this point, the idea just exists in the aether, and authorship isn’t often considered or cited. I suppose it has become "common knowledge" at least in some circles. It's usually not even given a citation, so I guess it's become a thing in its own right.

I’m still proud of Brett’s work and my small contribution to it. I'm glad it has taken on a life of its own, but I'm aware of when it's poorly described or when it's corrupted and am offended when people present it as their own unique work (or take praise for it, knowing that it is not original).

I get it, though. I'm sure there are many people whose work I didn't know how to credit, and whose work I may have likewise twisted to my own ends or interpreted in my own context whether I realized it or not. 

The acronym is pretty simple

  • FAST - you can run all your microtests so quickly that you never have to decide to run them later or now.
  • Isolated - tests don't rely upon either other in any way, including indirectly. Each test isolates one failure mode only. 
  • Repeatable - The test always gets the same result. This is largely a matter of good test setup and cleanup, but also the consideration of things like time of day, network status, database, global space, configuration, etc never being changed. 
  • Self-validating - a test is pass/fail. You never have to examine the input, output, or system state to determine if a test has passed. It is either green (passed), red (failed), or else the test did not run to completion (error), and it states the fact very clearly.
  • Timely - microtests are not to be written in batches, either before or after coding. They are written with the code, preferably just before adding a line or just after finishing a tiny change to a function.
Some people change the T to Thorough because they don't want to encourage TDD. 

I think this is a mistake. It's not "just as good" to have high coverage from a batch of tests written after the code is finished. It's not even nearly as good.

Done correctly, the tests inform and guide the code. 

  • They help us to consider our code first from the point of view of a user of the code, which results in more sensible and cohesive APIs. 
  • The tests make acceptance criteria a primary concern. If we don't know what result we are expecting, we can't write the test for it.
  • They make testability a primary concern: we can't write a test if we can't call the method or can't observe the result of making the method call. There is no "untestable code" if the tests come first.
  • As we realize we have corner cases, we add more tests, so that it's clear when we have covered the corner cases in the production code.
  • Because tests are written first, they are a kind of standalone documentation - you read the tests to understand the code. When tests are written after the code, they tend to take the structure and algorithms of the written code for granted: you must read the code to understand the test.
  • Whatever code passes the tests, that code is sufficient. The tests help us recognize a complete state.
  • Since the invariants for the production code are tested automatically, we can refactor the code to a different shape and design with the confidence of knowing that all of the tests still pass so we haven't violated any of our design invariants.
  • Each time our tests pass (run green) we are invited to reexamine the production code and the tests for readability and maintainability. This allows us to practice code craft continuously.
  • Because our intentions for the code are captured in the tests, we have externalized our intentions. We can be interrupted and quickly regain our context in the application - something that can't happen if we're in the middle of transcribing a large plan from our heads into the codebase.

Done post-facto, the tests have no way to influence the organization and expression of code in any meaningful way. Writing tests after the code in order to increase coverage becomes drudgery.

Where to find it.
Any quick google search will turn out dozens of articles.