Moving a Method via Mikado

We could consider a WAV snippet, or possibly a WAV snippet writer, as an abstraction of its own. The name of our primary class, WavReader, already suggests that anything to do with writing snippets is an inappropriate responsibility. Further, we are making extensive changes to the snippet logic and want to isolate the changes to a class with smaller scope.

As a short-term Mikado Goal, we want to extract the writeSnippet function into its own class, Snippet. Once we complete the immediate goal, we plan to clean the Snippet code considerably. One goal at a time, however.

Given the space constraints of a book, this is a smaller example, but it’s no less real than your regular refactoring challenges. It’s also surprising how many discrete steps we’ll work through, even in a task this small. The Mikado Method will prevent us from dropping the ball (or missing it entirely) while juggling all those steps.

Following the first step of the Mikado Method, we represent our end goal as a couple concentric ellipses. We prefer a large whiteboard as our tool. We want everyone to see the Mikado graph, so it will likely require a lot of space, and we’ll be building it out incrementally, making changes as we go.

images/goal.png

Per the second Mikado step, we run straight to the goal line in an extremely naive manner. We move (cut and paste) the writeSnippet function across to a new file, Snippet.h, where we wrap it in a class definition.

wav/17/Snippet.h
 
#ifndef Snippet_h
 
#define Snippet_h
 
 
class​ Snippet {
 
public​:
 
void​ writeSnippet(
 
const​ ​string​& name, istream& file, ostream& out,
 
FormatSubchunk& formatSubchunk,
 
DataChunk& dataChunk,
 
char​* data
 
) {
 
// ...
 
uint32_t secondsDesired{10};
 
if​ (formatSubchunk.bitsPerSample == 0) formatSubchunk.bitsPerSample = 8;
 
uint32_t bytesPerSample{formatSubchunk.bitsPerSample / uint32_t{8}};
 
 
uint32_t samplesToWrite{secondsDesired * formatSubchunk.samplesPerSecond};
 
uint32_t totalSamples{dataChunk.length / bytesPerSample};
 
 
samplesToWrite = min(samplesToWrite, totalSamples);
 
 
uint32_t totalSeconds { totalSamples / formatSubchunk.samplesPerSecond };
 
 
rLog(channel, ​"total seconds %i "​, totalSeconds);
 
 
dataChunk.length = dataLength(
 
samplesToWrite,
 
bytesPerSample,
 
formatSubchunk.channels);
 
out.write(​reinterpret_cast​<​char​*>(&dataChunk), ​sizeof​(DataChunk));
 
 
uint32_t startingSample{
 
totalSeconds >= 10 ? 10 * formatSubchunk.samplesPerSecond : 0};
 
 
writeSamples(&out, data, startingSample, samplesToWrite, bytesPerSample);
 
 
rLog(channel, ​"completed writing %s"​, name.c_str());
 
 
auto​ fileSize = fileUtil_->size(name);
 
descriptor_->add(dest_, name,
 
totalSeconds, formatSubchunk.samplesPerSecond, formatSubchunk.channels,
 
fileSize);
 
//out.close(); // ostreams are RAII
 
}
 
};
 
 
#endif

From WavReader, we create a Snippet object and then invoke its writeSnippet member function.

wav/17/WavReader.cpp
 
#include "Snippet.h"
 
// ...
 
void​ WavReader::open(​const​ std::​string​& name, ​bool​ trace) {
 
// ...
 
auto​ data = readData(file, dataChunk.length); ​// leak!
*
Snippet snippet;
*
snippet.writeSnippet(name, file, out, formatSubchunk, dataChunk, data);
 
}

We compile (Mikado step 3: find any errors). A quick sift through the many compile errors suggests they are because of missing definitions in Snippet.h, which in turn prevent the call to writeSnippet from compiling.

We decide that there are two prerequisites to accomplishing our Mikado Goal (step 4: devise immediate solutions to the errors):

  1. Get a new Snippet class in place by copying code.

  2. Change WavReader to use that Snippet object (the code change we already tried).

We reflect our refinement to the plan as prerequisites in our Mikado graph (step 5: add immediate solutions as new prerequisites).

images/step1.png

We’ll return later to invoke snippet code from WavReader. For now let’s focus on creating Snippet, an independent class that we can incrementally grow.

Now for the fun part, Mikado step 6. We revert our changes, since we had errors. Yup, we discard the code. We’ve captured the result of our analysis and attempt in a graph, and it didn’t take us long to make the actual changes once we knew what to do. (We’ll see how this plays out later.)

If you’re using a good source repository tool like git, step 6 can be as simple as this:

 
git reset --hard && git clean -f

This is of course potentially data-destroying. <Insert standard legal disclaimer here.>

We move on. Our goal is to successively work our way out toward leaf nodes on the graph—goals that we can complete without requiring prerequisite actions—repeating the preceding Mikado steps for each node. We turn our focus to seeing whether create new Snippet class by copying code is a leaf. Our first naive attempt is essentially the same code we just tried: copy writeSnippet to Snippet.h, wrapping it in a class definition and removing WavReader:: from the declaration. We get the compiler to recognize Snippet.h by adding an #include statement to WavReader.

Choosing a naive goal that is likely to fail can be off-putting to some folks. However, it keeps us from endless up-front analysis without any concrete feedback. It also tends to promote building smaller substeps in the Mikado graph.

Again, we receive compiler errors about unrecognized std types, but this time the compiler output is not conflated with problems in WavReader. To expedite getting to the Mikado Goal, we decide to add a using directive to solve the problem. Yes, this is “bad.” We make a note to clean up the code once we complete the Mikado Goal. (We could add the cleanup as a node on the Mikado graph, but we’ll keep this example simple for now.)

Our resolution to the failure becomes a new Mikado Goal.

images/step2.png

We’re being brief on the graph, which suits our needs, though it might not suit yours. Our goal encompasses including the namespace declaration as well as Snippet.h and a class definition.

You might be wondering about the granularity of each step in the Mikado Method. Should you detail every code operation? The answer depends on the size of your challenge, whether you hope to enlist others, and what is implicitly understood by those involved. For most undertakings, you can summarize detailed steps and presume that you’ll remember the required specifics or that your teammates will know what to do. If you want someone else to be able to implement the change by reading your graph, you might want to include more detail.

Our graph represents a fairly low-level goal. In the context of a more typically larger Mikado Goal, we might represent our entire goal (“extract snippet code to new class”) as but a single node.

We revert once again because of failures that including a using directive should fix. You may still find the notion of reverting code still a bit disturbing, but remember that it’s helping you build a very incremental and well-documented road map to a solution.

Reverting will become fairly natural soon. The time it takes you to re-apply code changes will diminish dramatically. Occasionally you’ll attempt a naive solution that ends up being a complete dead end. Your comfort level with reverting and starting over will make this a stress-free event.

We now start at the “using namespace std;” goal and attempt to resolve it. We add an #include statement to WavReader. We then add Snippet.h, toss a class shell into it, and add the using directive.

wav/18/Snippet.h
 
#ifndef Snippet_h
 
#define Snippet_h
 
 
using​ ​namespace​ std;
 
 
class​ Snippet {
 
public​:
 
};
 
 
#endif

That works! Our tests all pass. We have completed the “using namespace std;” goal and could conceivably add it to the codebase without harming anything. It’s up to us to decide whether to commit at this point. Since we’re using a powerful source versioning tool, we will commit as often as is safe, never mind how small our change is. (Using git, you can squash a bunch of commits into one if having too many creates headaches.)

images/step2updated.png

We check the goal off on the Mikado graph. Also, we choose to document adding an #include statement to WavReader on the graph as a completed goal.

We make another attempt to implement the goal “create new Snippet class by copying code.” Our errors, smaller now, tell us that several members (both functions and variables) are undefined. We take a look at the functions, dataLength and writeSamples, and note that they don’t depend upon any member variables. Copying across these functions as the next prerequisite goal makes the most sense.

images/step3.png

Reverting again and copying across the functions...oops...fails. It turns out there was a member variable, channels. (Not only is it missing the trailing underscore that helps us recognize member variables, but the code also reuses the variable for a nested loop. Ugh. We add this to our “fix later” list.)

We revert and shrink our current goal to copying only dataLength.

wav/19/Snippet.h
 
class​ Snippet {
 
public​:
*
uint32_t dataLength(
*
uint32_t samples,
*
uint32_t bytesPerSample,
*
uint32_t channels
*
) ​const​ {
*
return​ samples * bytesPerSample * channels;
*
}
 
};

We commit the code and update the graph.

images/step3updated.png

Next, we try copying writeSnippets and now also writeSamples. We fail again. The error is undefined member variables. The resolution is to add constructor and member variables to Snippet.h. Can we make the change and commit? You bet! The end result is a checked-off prerequisite goal.

wav/20/Snippet.h
 
class​ Snippet {
 
public​:
*
Snippet(shared_ptr<FileUtil> fileUtil,
*
shared_ptr<WavDescriptor> descriptor,
*
const​ std::​string​& dest,
*
rlog::RLogChannel* channel)
*
: fileUtil_(fileUtil)
*
, descriptor_(descriptor)
*
, dest_(dest)
*
, channel_(channel) { }
 
// ...
*
private​:
*
shared_ptr<FileUtil> fileUtil_;
*
shared_ptr<WavDescriptor> descriptor_;
*
const​ ​string​ dest_;
*
rlog::RLogChannel* channel_;
 
};
images/step4.png

And, yay! After reverting, we’re able to successfully copy across writeSnippets and writeSamples and then commit. We’re also able to check off the final outstanding prerequisite step: “invoke snippet code from WavReader.” (The phrasing snippet.writeSnippet is awkward, sure, but we can rename the function once we complete the Mikado Goal.)

images/step4updated.png

Our code requires a couple small, reasonably safe manual tweaks. We rename the member variable channel to channel_. We also default the channels argument on writeSamples to 1, replicating its definition from WavReader.h. Despite being small, these changes still contain a small element of risk that we accept (and use to prod ourselves to run a few slower tests).

wav/21/Snippet.h
 
class​ Snippet {
 
public​:
 
// ...
 
void​ writeSnippet(
 
const​ ​string​& name, istream& file, ostream& out,
 
FormatSubchunk& formatSubchunk,
 
DataChunk& dataChunk,
 
char​* data
 
) {
 
uint32_t secondsDesired{10};
 
if​ (formatSubchunk.bitsPerSample == 0) formatSubchunk.bitsPerSample = 8;
 
uint32_t bytesPerSample{formatSubchunk.bitsPerSample / uint32_t{8}};
 
 
uint32_t samplesToWrite{secondsDesired * formatSubchunk.samplesPerSecond};
 
uint32_t totalSamples{dataChunk.length / bytesPerSample};
 
 
samplesToWrite = min(samplesToWrite, totalSamples);
 
 
uint32_t totalSeconds { totalSamples / formatSubchunk.samplesPerSecond };
 
*
rLog(channel_, ​"total seconds %i "​, totalSeconds);
 
 
dataChunk.length = dataLength(
 
samplesToWrite,
 
bytesPerSample,
 
formatSubchunk.channels);
 
out.write(​reinterpret_cast​<​char​*>(&dataChunk), ​sizeof​(DataChunk));
 
 
uint32_t startingSample{
 
totalSeconds >= 10 ? 10 * formatSubchunk.samplesPerSecond : 0};
 
 
writeSamples(&out, data, startingSample, samplesToWrite, bytesPerSample);
 
*
rLog(channel_, ​"completed writing %s"​, name.c_str());
 
 
auto​ fileSize = fileUtil_->size(name);
 
 
descriptor_->add(dest_, name,
 
totalSeconds, formatSubchunk.samplesPerSecond, formatSubchunk.channels,
 
fileSize);
 
 
//out.close(); // ostreams are RAII
 
}
 
void​ writeSamples(ostream* out, ​char​* data,
 
uint32_t startingSample,
 
uint32_t samplesToWrite,
 
uint32_t bytesPerSample,
*
uint32_t channels=1) {
*
rLog(channel_, ​"writing %i samples"​, samplesToWrite);
 
// ...
 
}
 
};
wav/21/WavReader.cpp
 
void​ WavReader::open(​const​ std::​string​& name, ​bool​ trace) {
 
// ...
 
auto​ data = readData(file, dataChunk.length); ​// leak!
 
 
writeSnippet(name, file, out, formatSubchunk, dataChunk, data);
 
}

Are we done? Not quite. We need to delete the three snippet functions from WavReader. That’s a step not yet represented on our Mikado graph. We can add it as a direct prerequisite of the primary goal (“extract snippet code to new class”).

images/step5.png

Inserting a new prerequisite without a failure isn’t wrong, per se, but it’s similar to writing a test that automatically passes. Earlier we chose to copy across functions to Snippet. Perhaps we should have forced the issue and insisted that moving the methods was a more direct goal.

No matter, we choose to move forward this time. Our attempt to remove the functions from WavReader fails—this time because of the tests. We add a prerequisite goal, fix the tests, check off the prerequisite, successfully delete the functions from WavReader, and we are done! (We could create a few prerequisite steps for fix the tests, but no doubt you get the picture by now.)

You can find the final code in the distribution for the book. But it’s always important to see that last check mark applied to the Mikado Goal on the graph! (See Figure 1, The Mikado Goal with the Last Checkmark.)

images/final.png

Figure 1. The Mikado Goal with the Last Checkmark

From a design stance, we are in a happier place. WavReader worries primarily about reading WAV files, and Snippet worries primarily about writing out snippets from WAV file data. Previously, we had a mild concern about exposing the otherwise-private functions (such as writeSnippet). They now appear as part of the public interface for Snippet, and our concern diminishes. There’s more tweaking we could do, of course, but that’s always the case. It’s good enough for now, an incremental improvement, and we move on.

..................Content has been hidden....................

You can't read the all page of ebook, please click here login for view all page.
Reset
3.141.3.175