Monday, June 20, 2016

Digest for comp.lang.c++@googlegroups.com - 8 updates in 3 topics

Lynn McGuire <lmc@winsim.com>: Jun 20 12:09PM -0500

XKCD: I hate reading your code
http://xkcd.com/1695/
 
From
http://www.explainxkcd.com/wiki/index.php/1695
 
"Ponytail: It's like you ran OCR on a photo of a Scrabble board from a game where Javascript reserved words counted for triple points."
 
Lynn
"Alf P. Steinbach" <alf.p.steinbach+usenet@gmail.com>: Jun 20 09:08PM +0200

On 20.06.2016 19:09, Lynn McGuire wrote:
> http://www.explainxkcd.com/wiki/index.php/1695
 
> "Ponytail: It's like you ran OCR on a photo of a Scrabble board from a
> game where Javascript reserved words counted for triple points."
 
:)
 
I tried to credit you when I reposted on Facebook, but it found only
female Lynn McGuire's, and I'm guessing, wildly, that you're one of the
male Michael Lynn McGuire's (not found by Facebook), yes?
 
 
Cheers!, and thanks,
 
- Alf
Lynn McGuire <lmc@winsim.com>: Jun 20 02:36PM -0500

On 6/20/2016 2:08 PM, Alf P. Steinbach wrote:
> of the male Michael Lynn McGuire's (not found by Facebook), yes?
 
> Cheers!, and thanks,
 
> - Alf
 
I thought others might appreciate the joke. I started Fortran programming at the tender age of 14 by keypunching fortran card decks
for Dr. Sun Fu Yang. He wrote the subroutines on the back of 132 column printout sheets. Even thought the paper was ruled every
five rows, he just kind of randomly wrote all over the place and would draw lines to where his code should go.
 
Yup, male as I currently understand it.
https://www.linkedin.com/in/michael-lynn-mcguire-0737a930
 
Lynn
Tobias Anderberg <no-email@replies.please>: Jun 20 09:42PM +0100

> http://www.explainxkcd.com/wiki/index.php/1695
 
> "Ponytail: It's like you ran OCR on a photo of a Scrabble board from a
> game where Javascript reserved words counted for triple points."
 
"It works for now" and "It's only a prototype" is a clear sign of
technical debt in the making. Unfortunately all too common.
 
Still funny though. :)
 
--
a rackety rogue talks code
http://anderberg.me
legalize+jeeves@mail.xmission.com (Richard): Jun 20 10:01PM

[Please do not mail me a copy of your followup]
 
Lynn McGuire <lmc@winsim.com> spake the secret code
 
>XKCD: I hate reading your code
> http://xkcd.com/1695/
 
Speaking of which... I'm subscribed to the Visual Studio YouTube
channel. They recently posted this video about Shell corporation
moving to cloud VCS and build services:
<https://www.youtube.com/watch?v=7hT3EhLrkpQ>
 
As I watched the video, which was mostly marketing puffery about cloud
service migration, I noticed some particularly disgusting C++ code
showing up on the screen:
 
0:33
A perfect example of "comment clutter". An empty destructor is
shown in 4 physical lines:
 
TravLoadZoomDG::~TravLoadZoomDG ()
{
 
} // TravLoadZoomDG::~TravLoadZoomDG
 
Above it is a boiler plate "documentation" comment block consisting
of 8 lines that describes this 4 line block of code as "The class
destructor."
 
This is a perfect example of "comments as a code smell". A
destructor is a special class method that can only take one form
as it's definition. There is no need to further write in a giant
comment block that this special function -- which can only be
written in one single way -- is a destructor. It is useless
clutter that is just getting in the way.
 
One could further argue that if you're going to write an empty
constructor you might as well let the compiler define it for you
and do seomthing like "~foo = default;" in the class declaration.
(For header file compilation isolation, there are times when it is
desirable to write an empty constructor in the implementation file
and only declare it in the header file.)
 
Furthermore, I wish to comment on two points of what I call ugly
style in code. First, there is the comment on the closing brace
that indicates that this brace ends the destructor body. There is
only one place where I consider commenting the nature of a closing
brace to be acceptable and that is the closing brace of a
namespace.
 
Otherwise, your method/function bodies should be simple enough
that they fit completely on the screen and in only a few lines.
Anything that doesn't fit in say 25 lines of code is doing too
much and should be broken up into smaller methods by Compose
Method or perhaps there are other abstractions waiting to be
pulled out of your code to eliminate lots of low-level reptition
and make clear the main path of the algorithm.
 
Second, there is the pointless whitespace before the parenthesis
denoting the arguments to the destructor. Again, a destructor is
a special method that can only be written in one special way.
Separating the parenthesis from the name of the destructor doesn't
server any purpose and just leaves those parentheses separated and
hanging in space as if they were important. It overemphasizes the
role of the parntheses to separate them with whitespace. However,
we'll see further on that this code has really poor use of
whitespace.
 
0:36
 
Here we see some pretty ugly code. It looks something like this:
 
SpBufferedFile::typeText);
if( !isValid()|| !ok) {
return;
}
 
char line[1024]
SpString next_line;
// ----------------------------------
// Read in the next line of text
// ----------------------------------
while (bFile.readString(line, 1024) > 0)
{
next_line = line;
next_line.trimRight('\n');
if (next_line.locateNth("#") == 0 || next_line.isEmpty())
continue;
 
SpStringArray nv_pair = next_line.split(":");
if (nv_pair.size() < 2) continue;
cerr << nv_pair[0];
nv_pair[0].trimEnds();
if (nv_pair[0] == ZoomKeys::crs){
setCRS(next_line);
} else if (nv_pair[0].numOccur("ZM_")){
setZoom(nv_pair[0],nv_pair[1]);
}else if (nv_pair[0].numOccur("NUM_SEGS")){
setNumSegs(nv_pair[1].trimEnds());
 
}
else if (nv_pair[0].numOccur("SEG")){
if (nv_pair.size() != 5 )cerr << "SEG WRONG!!";
addTraverseSegment(nv_pair[1].trimEnds(),nv_pair[2].trimEnds(),nv_pair[3].trimEnds(),nv_pair[4//code goes off screen
 
}
}
bFile.close();
 
 
} // TravZoomImpCmd::doIt
 
 
How many different kinds of whitespace usage and indentation can
you count in this block of code? It's all over the place. I also
love the debugging output that has been left in the code in place
of actual error handling. The identifier naming convention also
appears to be all over the map. Control structures are used
inconsistently in conjunction with indentation, so it is difficult
to get an overview of the control flow by scanning the indentation.
We're using fixed-length raw char arrays for string input and then
we immediately copy the raw string array into a string class.
Because what's better than doing a copy while reading data but to
do another copy immediately after reading the data? To me, this
mess of low-level code looks like it could have been written much
more succinctly using Boost.Spirit to parse the string into
name/value pairs. Since we are manually closing "bFile" at the
bottom of the block, it is also clear that this code is not exception
safe. Again, another opportunity for pulling out a small abstraction
like an RAII save resource handling class for whatever is managed
by bFile. Did I say "bFile"? What is this "b" prefix? Maybe this
code came out of the 1990s when Hungarian Notation was all the rage,
even though MS finally came to their senses and specifically tells
you to avoid it now.
 
Finally, I'll leave you looking at that very last line that tells us
that this ginormous method is named "doIt". Really? You're kidding,
right? Grinding through this input file and processing name/value
pairs is described by "doIt"? I guess I shouldn't expect better of
the author since they can't decide how whitespace should be used
to make control structures clear. Just look at how the spaces
are randomly puked between braces and keywords like else, if and
continue.
 
1:08
Here we see more of the code after the intention-revealingly named
"doIt" method.
 
void TravZoomExpCmd::addTDInfo( SPTimeDepthInformation *info )
{
if (!info ) return;
 
SpTimeOrDepth td = info->timeDepth();
if (td == SpDEPTH)
addNameValuePair("DOMAIN","DEPTH");
else
addNameValuePair("DOMAIN","TIME");
addNameValuePair("DOMAIN_UNITS","TIME");
addNameValuePair("DATUM","TIME");
addNameValuePair("DATUM_UNITS","TIME");
 
 
}
 
Again, I don't understand the use of whitespace here. For some
reason it was considered necessary to make the arguments to this
method stand off with extra whitespace -- even though that is what
the parenthesis already do.
 
In the very first statement we see that someone added a defensive
guard against passing a nullptr into this method. I'd probably
consider passing a nullptr to this method to be a programming
error. Why are you asking me to add time/depth information from
no information? There should probably be an assert here to indicate
that there is a programming error elsewhere -- our preconditions
were not met. Yes, we shouldn't crash and we should program
defensively, but without issuing any sort of warning or performing
an assert, we've simply left client code that calls our code in
violation of our preconditions intact. Good code not only defends
against misuse, but properly informs the programmer of that misuse.
Basically, I'm saying we shouldn't code around programming errors
at runtime. If we were using a design-by-contract sort of language,
we would have violated the contract to pass in a nullptr and the
code wouldn't compile.
 
We can also see that some random whitespace was chucked onto the
end of the conditional expression. There's also a blank line before
the end of the method body. Why? You may have different preferences
for whitespace usage than myself, but good god, at least use one
consistent way of writing the code.
 
Even better, we can see linker warnings in the build output window:
 
warning LNK4049: locally defined symbol daglbc_ imported
 
This isn't a good thing! But of course, since we can't seem to
manage to get any sort of consistent whitespace in our source code,
why should we be bothered with consistently declaring the import/
export relationship of our symbols?
 
<https://msdn.microsoft.com/en-us/library/atww7hec.aspx>
 
1:22
 
We see some more code, now with commit messages as embedded comments!
 
// ------- -------------------- ----------------------------------------
// 14Oct14 Paresh Patel Initial Implementation.
// 04Nov14 Paresh Patel Refactor display of wells
// to XSectionUtil::showWellsForXSetion()
//
//==========================================================================
 
Boy, Shell sure does love BIG BLOCK COMMENTS, don't they? Isn't this
the sort of information that should be in the version control system?
When reading this code today and trying to understand it, why is it
important to know that it was implements in early October, 2014?
 
This is more "comments as a code smell" -- this stuff belongs in the
version control system and not in the source code itself. It is
cluttering up the screen with information we don't need 99.99999% of
the time.
 
void TraverseWindow::slotOpenXSectionFile()
{
QString xSectionFileName = QFileDialog::getOpenFileName(
this,
tr("Open XSection File"),
QDir::currentPath(),
"nDI XSection File (*.xsec)"
);
if( xSectionFileName.isEmpty() ){
SpErr("Open XSection File", SpEC_DEBUG)
<< "xSectionFileName is empty." << ends;
return;
}
 
SpTraverseView* view = currentTraverseView();
if (!view || !view->traverseDescription()) {
SpErr("slotOpenXSectionFile", SpEC_DEBUG)
<< "Invalid view or traverseDescription pointer." << ends;
return;
}
 
So it seems that this is a Qt application, or at least the GUI is
written with Qt. However, we still can't seem to decide if we're
going to be writing an 'if' statement like a function call with
spaces inside the parenthesis:
if( xSectionFileName.isEmpty() ){
of if we're going to write an 'if' statement like a control structure:
if (!view || !view->traverseDescription()) {
 
At least this time the debugging error messages are written to some
sort of debug error message facility instead of just dumping them
randomly to stderr (which may not even exist on some systems; think
Windows or embedded programming).
 
1:28
 
Here we see a little bit of Qt GUI code:
 
QWidget* SaveZoomDG::createWorkArea( QWidget* container )
{
ui = new UI::SaveZoomDG;
ui->setupUi( container );
 
ui->fileSelector->pattern(sp2qstr("*" +_ext));
new SpWidgetValidation( ui->fileSelector, "File dir_write NULL");
 
ui->fileSelector->allowNonExistingFile(true);
ui->fileSelector->dialogTitle("Select Zoom Save File");
return this;
}
 
Oops, we forgot to translate that dialog title. And why is there two
spaces between Zoom and Save in that dialog title? Again, whitespace
seems to be all over the map here -- we can't manage to use whitespace
consistently in a method with 7 lines of code?!? Apparently we're
naming member variables by beginning them with an underscore ("_ext"),
but fortunately our inconsistent use of whitespace around operators
makes it easy to miss that it's a member variable and we might be
referring to a local or global variable named "ext". Who knows, maybe
it is a global or translation unit scope variable after all. We can't
tell from this snippet.
 
1:34
 
Ah, yes, it is indeed referring to a member variable now that we can
see the constructor for a presumably related class:
 
//==========================================================================
// NAME: LoadZoomDG::LoadZoomDG
//--------------------------------------------------------------------------
// PURPOSE:
//
// The class constructor.
//
//==========================================================================
 
LoadZoomDG::LoadZoomDG (const char * name,
const SpString& ext,
bool autoFree) :
_ext(ext),
ui(NULL),
_setAsHome(false),
SpConversationDialog(name, autoFree)
{
setWindowTitle (tr("Load Zoom Coordinates"));
 
} // LoadZoomDG::LoadZoomDG
 
It's a good thing we had that big comment to tell us the name of the
method and that it was a constructor, or I wouldn't be able to make
sense of the code! Wait, wasn't this supposed to have an edit
history on it too? We better dock them a few points on the code
review paperwork!
 
The other code was using indent levels of 4 or 3, but what the
hell, let's use an indent level of 2 for this class, just for fun.
 
While we're at it, let's randomly place whitespace around pointer
and reference indicators when declaring them! It's not like they
are important in the role of the variables.
 
And just for yucks, let's write the initializers in any old order we
feel like it -- the compiler will sort it all out, right?
 
In all fairness, perhaps this was Shell's shittiest code in which they
had the least amount of intellectual property and that's why they were
willing to let Microsoft film it for this video.
 
But come on, guys, if you're going to put your code in front of a camera,
have a little pride and make it code that isn't embarrassingly full of
crap.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
The Computer Graphics Museum <http://computergraphicsmuseum.org>
The Terminals Wiki <http://terminals.classiccmp.org>
Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>
legalize+jeeves@mail.xmission.com (Richard): Jun 20 08:56PM

[Please do not mail me a copy of your followup]
 
Real Troll <real.troll@trolls.com> spake the secret code
 
>On 18/06/2016 15:55, Jerry Stuckle wrote:
>> So now you've answered the question. You are both an idiot and a troll.
 
>Are you sure dickhead?
 
Oh yes, he's a sure dickhead alright. He's always sure of himself and
he's a dickhead.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
The Computer Graphics Museum <http://computergraphicsmuseum.org>
The Terminals Wiki <http://terminals.classiccmp.org>
Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>
Ralf Goertz <me@myprovider.invalid>: Jun 20 11:10AM +0200

Am Sat, 18 Jun 2016 23:32:12 +0200
 
> It depends on the specification.
 
> Most implementations I know use \1, \2 ... for back references in the
> pattern and $1, $2 ... in the replacement string.
 
But that's the problem. Boost also uses $ for back reference. So why
would I need to escape the backslash?
"Öö Tiib" <ootiib@hot.ee>: Jun 20 05:58AM -0700

On Monday, 20 June 2016 12:10:46 UTC+3, Ralf Goertz wrote:
> > pattern and $1, $2 ... in the replacement string.
 
> But that's the problem. Boost also uses $ for back reference. So why
> would I need to escape the backslash?
 
Best is to assume that 'boost::regex_replace' and 'std::regex_replace'
are totally different functions and those having something in common
is purely accidental.
 
Issue about escape symbols is always about details of grammar. Regex
grammars can be seemingly configured with constants (your posted
code goes with defaults). http://www.cplusplus.com/reference/regex/regex_constants/
There can be (I'm speculating now) some sort of difference. For example
that the 'format_default' means slightly different things for 'boost' and
'std'.
 
Such differences are no problem but the endless source of work (and
so bread) for you. ;-)
You received this digest because you're subscribed to updates for this group. You can change your settings on the group membership page.
To unsubscribe from this group and stop receiving emails from it send an email to comp.lang.c+++unsubscribe@googlegroups.com.
Posted by at

No comments:

Post a Comment

Subscribe to: Post Comments (Atom)