Single Responsibility Principle

The Partridge Family were neither partridges nor a family. Discuss.
Post Reply
albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Single Responsibility Principle

Post by albinopapa » November 12th, 2019, 12:11 am

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 wanted to have a source to make a point that I'm not making this up.

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() ) )
		};
	}
Current version:

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() ) )
		};
	}
I left the ThrowSystemErrorIf macro there in the old code example just because I was lazy, but the number of lines of code would be a lot more if I hadn't.

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()
		);
	}
Something like this where you can get some code reuse seems like a good case, but what about when there is no code reuse?

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() );
	}
This code makes a font sheet. It uses all three APIs I mentioned earlier; WIC, DirectWrite and Direct2D to make the font sheet. DirectWrite is used to load in a font family. Direct2D uses an IWICBitmap as a render target as the target in which to render the fonts. The IWICBitmap is then copied into a dim2d::surface<Color> from the dim2d library I wrote.

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;
	}
If you can imagine all that code in the Fontsheet constructor, it'd be pretty long and probably even more confusing on what is actually happening.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

User avatar
chili
Site Admin
Posts: 3948
Joined: December 31st, 2011, 4:53 pm
Location: Japan
Contact:

Re: Single Responsibility Principle

Post by chili » November 16th, 2019, 1:56 pm

Yo, this is actually similar to something that I emphasize two videos from the current point in HW3D. You ever read Clean Code by Uncle Bob?
Chili

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Single Responsibility Principle

Post by albinopapa » November 16th, 2019, 4:06 pm

Nope. I generally don't read books.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

Post Reply