I wanted to have a source to make a point that I'm not making this up.Severin Perez on Medium.com wrote:The Single Responsibility Principle — Classes and functions should have a single responsibility and thus only a single reason to change
I was looking through some of my code and realized how much influence this particular design decision has changed my code over the years. I wasn't terribly bad about writing long functions, but I did have a lot of "imperative" style code in them. What I mean is the code described how to do something as opposed to what it was doing. Functions were just a list of steps to be taken to perform some task. Over the years though, I've gradually started putting steps in functions to give that portion of code some context and meaning. I'd like to share a few examples of what I mean.
Instead of using GDI+ to load/save images I decided to move on to the Windows Imaging Component as Microsoft COULD stop supporting GDI+ anytime they feel like since dealing with images, fonts and rendering those images and fonts to screen have been separated into WIC ( loading/saving images ), DirectWrite ( fonts ) and Direct2D ( rendering images and fonts ). For loading an image using WIC requires a few more steps than in GDI+. You have to create an instance of the IWICImagingFactory, create a IWICBitmapDecoder, then a IWICBitmapFrameDecode just to get to the pixel data. Then, to make sure the pixel format is in the correct order, you create an IWICFormatConverter. Now, you can either create an IWICBitmap and use that as your sprite or copy the pixel data to your own buffer. Now, when I first started using this API, I'd put all the steps into a single function, either in an Image/Sprite class constructor or maybe a static Image::Load( std::string filename ) function as a factory function. Now, I have switched to having smaller functions that create these IWIC interfaces which makes my Sprite::Load( filename ) or Sprite( filename ) functions cleaner and self documenting.
Code: Select all
static auto load( std::string _filename )
{
assert( !_filename.empty() && "No file name given." );
auto factory = Microsoft::WRL::ComPtr<IWICImagingFactory>{};
ThrowSystemErrorIf( CoCreateInstance(
CLSID_WICImagingFactory,
nullptr,
CLSCTX_INPROC_SERVER,
IID_PPV_ARGS( &factory )
) );
assert( !_filename.empty() );
auto wide_filename = std::wstring( _filename.begin(), _filename.end() );
auto decoder = Microsoft::WRL::ComPtr<IWICBitmapDecoder>{};
ThrowSystemErrorIf( factory->CreateDecoderFromFilename(
wide_filename.c_str(),
nullptr,
GENERIC_READ,
WICDecodeMetadataCacheOnLoad,
&decoder
) );
auto frame = Microsoft::WRL::ComPtr<IWICBitmapFrameDecode>;
ThrowSystemErrorIf( decoder->GetFrame( 0, &frame ) );
auto converter = Microsoft::WRL::ComPtr<IWICFormatConverter>{};
ThrowSystemErrorIf( factory->CreateFormatConverter( &converter ) );
ThrowSystemErrorIf( converter->Initialize(
frame.Get(),
GUID_WICPixelFormat32bppPBGRA,
WICBitmapDitherTypeNone,
nullptr,
1.0,
WICBitmapPaletteTypeCustom
) );
auto width = 0u;
auto height = 0u;
ThrowSystemErrorIf( converter->GetSize( &width, &height ) );
assert( width != 0u );
assert( height != 0u );
auto const stride = calculate_stride<std::uint32_t>( width );
auto const buffer_size = calculate_stride<std::uint32_t>( stride );
auto pixels = std::make_unique<std::uint8_t[]>( buffer_size );
ThrowSystemErrorIf( pConverter->CopyPixels( nullptr, stride, buffer_size, pixels.get() ) );
return Image{
int( width ),
int( height ),
std::unique_ptr<Color[]>( reinterpret_cast< Color* >( pixels.release() ) )
};
}
Code: Select all
static auto load( std::string _filename )
{
assert( !_filename.empty() && "No file name given." );
auto wic = Wic{};
auto wide_filename = std::wstring( _filename.begin(), _filename.end() );
auto decoder = wic.create_decoder( std::move( wide_filename ) );
auto frame = wic.create_frame_decode( decoder.Get() );
auto converter = wic.create_format_converter( frame.Get() );
auto width = 0u;
auto height = 0u;
ThrowSystemErrorIf( converter->GetSize( &width, &height ) );
auto buffer = wic.copy_pixels_to_buffer( width, height, converter.Get() );
return Image{
int( width ),
int( height ),
std::unique_ptr<Color[]>( reinterpret_cast< Color* >( buffer.release() ) )
};
}
Old code line count: 54 ( possibly another 24 lines if the macro wasn't used )
New code line count: 19
I understand that the new version still has more code overall, but the Image::load function is way more readable now. Separating the code like I did, I was able to reuse some things like the IWICImagingFactory is now created in the Wic constructor, the Wic::create_stream() function also gets reused during saving the image buffer to disk.
Code: Select all
static void save( std::string _filename, Image& _image )
{
assert( !_filename.empty() && "No file name provided." );
assert( _image && "Image not loaded." );
auto wic = Wic{};
auto stream = wic.create_stream( std::wstring( _filename.begin(), _filename.end() ) );
auto encoder = wic.create_encoder( stream.Get() );
auto frame = wic.create_frame_encode( encoder.Get() );
wic.write_pixels_to_file(
std::uint32_t( _image.width() ),
std::uint32_t( _image.height() ),
reinterpret_cast< std::uint8_t* >( _image.m_pixels.get() ),
encoder.Get(),
frame.Get()
);
}
Code: Select all
FontSheet( TextFormat text_format_ = TextFormat{} )
{
const auto fontsize = calculate_font_size( text_format_ );
m_charWidth = fontsize.width;
m_charHeight = fontsize.height;
m_nCharsPerRow = 32;
// Okay, almost no code reuse
auto pBitmap = Wic{}.create_bitmap( m_nCharsPerRow * m_charWidth, 3 * m_charHeight );
auto pFactory2d = create_D2D_factory();
auto pRenderTarget = create_WIC_render_target( pFactory2d.Get(), pBitmap.Get() );
auto pBrush = create_D2D_solid_brush( pRenderTarget.Get() );
auto char_list = generate_char_list();
render_font_sheet( pRenderTarget.Get(), pBrush.Get(), char_list, text_format_ );
m_pPixels = create_surface( pBitmap.Get() );
}
And here's all the supporting functions:
Code: Select all
SizeI calculate_font_size( TextFormat const& text_format_ )const
{
auto pLayout_H = Microsoft::WRL::ComPtr<IDWriteTextLayout>{};
// Ignore the use of a singleton here, I'm still refactoring
ThrowSystemErrorIf(
DWriteInitter::Instance()->CreateTextLayout(
L"H", 1, text_format_.GetFormat(), 32.f, text_format_->GetFontSize(), &pLayout_H )
);
auto metrics = DWRITE_TEXT_METRICS{};
ThrowSystemErrorIf( pLayout_H->GetMetrics( &metrics ) );
return {
static_cast< int >( std::ceilf( metrics.width ) ),
static_cast< int >( std::ceilf( metrics.height ) )
};
}
std::string generate_char_list()const
{
auto charList = std::string( 127 - 32, '\0' );
auto next_char = [ c = ' ' ]()mutable { return c++; };
std::generate( charList.begin(), charList.end(), next_char );
return charList;
}
Microsoft::WRL::ComPtr<ID2D1Factory> create_D2D_factory()const
{
auto pFactory2d = Microsoft::WRL::ComPtr<ID2D1Factory>{};
ThrowSystemErrorIf(
D2D1CreateFactory( D2D1_FACTORY_TYPE_SINGLE_THREADED, pFactory2d.GetAddressOf() )
);
return pFactory2d;
}
Microsoft::WRL::ComPtr<ID2D1RenderTarget> create_WIC_render_target( ID2D1Factory* pFactory2d, IWICBitmap* pBitmap )const
{
auto pRenderTarget = Microsoft::WRL::ComPtr<ID2D1RenderTarget>{};
auto props = D2D1::RenderTargetProperties();
props.pixelFormat.alphaMode = D2D1_ALPHA_MODE_PREMULTIPLIED;
props.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM;
ThrowSystemErrorIf(
pFactory2d->CreateWicBitmapRenderTarget( pBitmap, props, &pRenderTarget )
);
return pRenderTarget;
}
Microsoft::WRL::ComPtr<ID2D1SolidColorBrush> create_D2D_solid_brush( ID2D1RenderTarget* pRenderTarget )const
{
auto pBrush = Microsoft::WRL::ComPtr<ID2D1SolidColorBrush>{};
auto const color = D2D1::ColorF( D2D1::ColorF::Black );
ThrowSystemErrorIf( pRenderTarget->CreateSolidColorBrush( color, &pBrush ) );
return pBrush;
}
void render_font_sheet(
ID2D1RenderTarget* pRenderTarget,
ID2D1SolidColorBrush* pBrush,
std::string charList,
TextFormat Format )
{
pRenderTarget->BeginDraw();
pRenderTarget->Clear( D2D1::ColorF( D2D1::ColorF::White,1.f ) );
const auto wCharList = std::wstring( charList.begin(), charList.end() );
for( int y = 0; y < 3; ++y )
{
for( int x = 0; x < m_nCharsPerRow; ++x )
{
const auto index = x + ( y * m_nCharsPerRow );
const auto c = charList[ index ];
const auto wc = wCharList[ index ];
const auto rect = RectF( char_rect( c ) );
const auto d2d_rect = D2D1_RECT_F{
rect.left(),
rect.top(),
rect.right(),
rect.bottom()
};
pRenderTarget->DrawText( &wc, 1, Format.GetFormat(), d2d_rect, pBrush );
}
}
ThrowSystemErrorIf( pRenderTarget->EndDraw() );
}
dim2d::surface<Color> create_surface( IWICBitmap* pBitmap )
{
auto width = 0u, height = 0u;
pBitmap->GetSize( &width, &height );
auto wr = WICRect{ 0, 0, std::int32_t( width ), std::int32_t( height ) };
auto pLock = Microsoft::WRL::ComPtr<IWICBitmapLock>{};
pBitmap->Lock( &wr, WICBitmapLockRead, &pLock );
auto bufferSize = 0u;
Color* pBuffer = nullptr;
pLock->GetDataPointer( &bufferSize, reinterpret_cast< WICInProcPointer * >( &pBuffer ) );
auto dst = dim2d::surface<Color>( int( width ), int( height ) );
dim2d::fill( dst.begin(), dst.end(), Colors::White );
auto const src =
dim2d::raw_pointer_wrapper<const Color>( dim2d::Offset{ 0, 0 }, int( width ), int( height ), int( width ), pBuffer );
auto is_not_white = [ & ]( dim2d::index _idx, const Color& _color )
{
return _color != Colors::White;
};
dim2d::replace_if( src.begin(), src.end(), dst.begin(), Colors::Black, is_not_white );
return dst;
}