October 20, 2018
On Friday, 19 October 2018 at 03:53:12 UTC, Walter Bright wrote:
> On 10/15/2018 2:23 PM, Walter Bright wrote:
>> I'm giving a presentation at:
>> 
>> http://nwcpp.org/
>> 
>> See you there!
>
> Had a nice crowd there last night. Apparently lots of people were interested in this topic!
>
> Video: https://www.youtube.com/watch?v=lbp6vwdnE0k&feature=youtu.be
>
> Slides: http://nwcpp.org/talks/2018/code_smells.pdf

https://www.youtube.com/watch?v=lbp6vwdnE0k#t=42m01s

Or it's a pain in the ass to try to get more than one pull request merged in due to lack of overseers. It's hard enough trying to get one pull request merged in, but trying to split one pull request into 4 and then try to micro manage it so that they are merged in order as the changes depend on each other (eg spelling error for a function). That becomes a nightmare when you don't have merging permission for a repo, especially for something like DMD where it's hard enough to get even a simple fix through the door.
October 21, 2018
luckoverthere <luckoverthere@gmail.cm> wrote:
> On Friday, 19 October 2018 at 03:53:12 UTC, Walter Bright wrote:
>> On 10/15/2018 2:23 PM, Walter Bright wrote:
>>> I'm giving a presentation at:
>>> 
>>> http://nwcpp.org/
>>> 
>>> See you there!
>> 
>> Had a nice crowd there last night. Apparently lots of people were interested in this topic!
>> 
>> Video: https://www.youtube.com/watch?v=lbp6vwdnE0k&feature=youtu.be
>> 
>> Slides: http://nwcpp.org/talks/2018/code_smells.pdf
> 
> https://www.youtube.com/watch?v=lbp6vwdnE0k#t=42m01s
> 
> Or it's a pain in the ass to try to get more than one pull request merged in due to lack of overseers. It's hard enough trying to get one pull request merged in, but trying to split one pull request into 4 and then try to micro manage it so that they are merged in order as the changes depend on each other (eg spelling error for a function). That becomes a nightmare when you don't have merging permission for a repo, especially for something like DMD where it's hard enough to get even a simple fix through the door.

I unfortunately ran into this with my make-ddoc-support-markdown PR. The original PR was considered too big, and I was asked to split it into multiple PRs against a feature branch, which would then be merged into master. I was fine with this, and started the process. It was a bit slow, but seemed like it would work. Oh, what a fool I was!

When my second PR to the feature branch was approved, it couldn’t be merged
because it caused unrelated errors in the tests. My best theory is that the
feature branch had drifted too far away from master, and since dmd,
druntime and phobos have dependencies on each other it was somehow causing
the test servers to fail.

There’s a bit more to the story, but it boils down to the fact that since I don’t have permissions to update the feature branch, I’m basically dead in the water. So the work is all done and ready, but I have no path forward to get it in. And it’s a bit disheartening to be foiled by the machines :)
October 21, 2018
On 10/21/2018 1:29 PM, David Gileadi wrote:
> There’s a bit more to the story, but it boils down to the fact that since I
> don’t have permissions to update the feature branch, I’m basically dead in
> the water. So the work is all done and ready, but I have no path forward to
> get it in. And it’s a bit disheartening to be foiled by the machines :)

Just PR them against master.

October 22, 2018
On Friday, 19 October 2018 at 03:53:12 UTC, Walter Bright wrote:
> On 10/15/2018 2:23 PM, Walter Bright wrote:
>> I'm giving a presentation at:
>> 
>> http://nwcpp.org/
>> 
>> See you there!
>
> Had a nice crowd there last night. Apparently lots of people were interested in this topic!
>
> Video: https://www.youtube.com/watch?v=lbp6vwdnE0k&feature=youtu.be
>
> Slides: http://nwcpp.org/talks/2018/code_smells.pdf

https://www.reddit.com/r/programming/comments/9qbhw2/nwcpp_walter_bright_talks_about_the_code_smells/
October 30, 2018
On Mon, Oct 22, 2018 at 07:27:51AM +0000, Basile B. via Digitalmars-d-announce wrote:
> On Friday, 19 October 2018 at 03:53:12 UTC, Walter Bright wrote:
> > On 10/15/2018 2:23 PM, Walter Bright wrote:
> > > I'm giving a presentation at:
> > > 
> > > http://nwcpp.org/
> > > 
> > > See you there!
> > 
> > Had a nice crowd there last night. Apparently lots of people were interested in this topic!
> > 
> > Video: https://www.youtube.com/watch?v=lbp6vwdnE0k&feature=youtu.be
> > 
> > Slides: http://nwcpp.org/talks/2018/code_smells.pdf
> 
> https://www.reddit.com/r/programming/comments/9qbhw2/nwcpp_walter_bright_talks_about_the_code_smells/

Haha, I liked how Walter caught the audience's attention by using #include <windows.h> as the first example of a code smell. :-D

Honestly, though, IMO this is also a code smell:

	import std.stdio;

That is, if it appears at the top of every source file.  Walter sortof said the same thing already, but basically, this usually also implies writeln's (or printf's) all over code that probably don't really have anything to do with I/O.  Which in itself is already bad enough -- code that isn't responsible for I/O shouldn't be doing I/O -- but it also binds the code to writing to stdout, so if later on you wish to write to a logfile instead, you're up the creek without a paddle.

I would go even further, though.  All too often I've seen (and wrote)
code that does this:

	import std.stdio;

	auto myFunc(Args...)(Args args) {
		... // do lots of stuff
		auto f = File(myFilename, "w");
		f.writefln(... lots of stuff... );
		...
	}

Then I decide that the code is too complex for me to have any confidence that it is correct, and so I need to write a unittest for it.  But ... since it writes to an external file, and possibly decides on its own output filename, the unittest has to worry about how to test it without leaving detritus lying around in the filesystem.

My current go-to solution is to abstract away the filesystem itself, e.g.:

	// N.B.: *no* import of std.stdio!
	auto myFunc(File = std.stdio.File, Args...)(Args args) {
		... // do lots of stuff
		auto f = File(myFilename, "w");
		f.writefln(... lots of stuff... );
		...
	}

	unittest
	{
		struct FilesystemProxy {
			string[string] contents;
			string curFile;
			this(string filename) { curFile = filename; }
			writefln(Args...)(string fmt, Args args) {
				contents[curFile] ~= format(fmt, args);
			}
		}
		FilesystemProxy fs;
		...
		auto ret = myFunc!FilesystemProxy(blah, blah, blah);

		assert(fs.contents["file1"] == expectedOutput);
		...
	}

This lets me inject a proxy object in place of std.stdio.File, so that the unittest can check code logic without touching the real filesystem. But by default, when real user code calls the function, it writes to the real filesystem.

You may think this is excessive, but recently I've had a success story with this style of coding: I have some D code cross-compiled to run on Android, and it was crashing at runtime in a very hard-to-debug way. In order to narrow things down, I had to write unittests to make sure the code actually did what I thought it did. But I couldn't just write it straight, because it assumes a filesystem structure that doesn't exist on my host PC, and running unittests on Android was out of the question since it would just run into the same bug and crash without yielding useful information.

Abstracting away the filesystem allowed me to compile and test that bit of code on the host PC separately from the other code, and without requiring that I make a copy of the Android filesystem structure on the PC just so I could test the code.  This enabled me to locate the bug and write a unittest that prevents future regression, AND this test can be run locally before being deployed to Android.

Now of course, Android has debugging tools and such that I could have used instead -- but imagine if this code were deployed on a bare-metal embedded system with little or no debugging support.  Being able to test the code logic without deploying the code on the real target hardware is a big plus.  This style of coding makes your code compilable without any platform-specific libraries that may not even run on your host PC.  If compiling for the target machine requires libxyz, and libxyz simply doesn't run on a PC, then you're out of luck. But once you abstract away that dependency, you can compile your business logic without libxyz (by substituting proxy objects like above) and run tests locally until you're sure the logic is sound, before deploying it to the actual hardware.

//

As for encapsulating globals in a struct, I don't like it.  It's better than literally littering globals everywhere, making it next to impossible to figure out what the code is doing -- it's like action at a distance, something physicists are uncomfortable with because it's hard to reason about (well, almost impossible when applied to code).  At the very least, it makes globals greppable, and makes any access plainly obvious. But it still suffers from all the drawbacks of globals:

- It's very hard to tell where the global is initialized -- or more
  importantly, *when* it's initialized.  Not initializing it before it's
  needed is a hard-to-trace bug.

- It exhibits action-at-a-distance: an arbitrary piece of code can
  affect an arbitrary other piece of code. Very hard to reason about the
  code as a whole.

- It's still a side-channel, the same problem discussed a few slides
  prior.  It's still a global, just in a better-looking dress.  It's
  papering over the problem instead of solving it.

- It makes the code hard to test, or just plain non-testable. (Writing a
  unittest that has to setup global variables beforehand is a bad, bad,
  bad thing. If such a test can even be written in the first place...
  usually such code is untestable except in a whole-program way, because
  its pieces are too tightly bound to each other. And we all know what
  happens to code quality when it's not tested enough.)

I would take it one step further: now that all the globals are collected in one place, PASS IT AS A FUNCTION ARGUMENT to code that needs it. That way, you know exactly who it gets passed to, and you know code that you didn't pass it to would not have a side-channel to its members.  But since it's cumbersome to pass it around everywhere (if it's truly global), I'd just move functions that depend on it into the struct as member functions, that way the globals get passed implicitly.

Doing this will naturally lead to the next step: a good programmer will inevitably realize how ugly the struct is, because it's a haphazard collection of random stuff thrown together, and will inevitably feel the urge to split up the struct into logically self-contained pieces, EACH WITH ITS OWN MEMBERS unrelated to the other pieces. In other words, the code will "want" to be in different modules. This logically leads to proper modularization of the original, unfocused, global-dependent code.

//

Now, on the slide about no I/O in low-level functions, I don't think passing a delegate as a stand-in for I/O is a good idea. Especially with the stated example:

	int sum(void delegate(char*) put, int i, int j) {
		if (i > 100)
			put("i is out of range");
		return i+j;
	}

The problem is that this code shouldn't be doing I/O at all, indirectly or otherwise. Writing out a string via a delegate is not in the charter of this function.  Again, it's just papering over the problem rather than solving it.  Doing so uglifies the API, and makes it needlessly cumbersome to use.  Plus, it invites lazy coders to write:

	auto s = sum((s) {} /* ignore errors */, x, y);

just to get rid of annoying error messages, thus leading to code that hides errors rather than deal with them.  Having seen too many cases of this in real-life code, this kind of API always makes me cringe.

Instead, I'd say just throw a darned Exception already.  It's what exceptions are meant for: an out-of-band bailout when something went wrong, that the immediate caller either has to handle, or let somebody further up the call stack handle. Usually, the latter is a much better idea, because the lower-level code may not have the necessary context to make the right decision about this.


T

-- 
Leather is waterproof.  Ever see a cow with an umbrella?
October 31, 2018
On Tuesday, 30 October 2018 at 22:22:33 UTC, H. S. Teoh wrote:

> Haha, I liked how Walter caught the audience's attention by using #include <windows.h> as the first example of a code smell. :-D
>
> Honestly, though, IMO this is also a code smell:
>
> 	import std.stdio;
>
> That is, if it appears at the top of every source file.

IMO, if it appears anywhere at all :)
Pop quiz. Try to answer without compiling, what do you think this *should* print, and what *does* it print?

```
void main() {
    static struct S {
        int value;
        this(this) {
            ++value;
        }
    }

    writefln("%s", S.init);
}
```

In an imaginary ideal world, my answer to the first question would've been "S(0)"... The language has `auto ref` and `shared`, so it has all the capacity necessary to achieve that. Alas, there's a "but" in there somewhere...

> As for encapsulating globals in a struct, I don't like it...

That's something one has to rigorously audit at all times. Grouping of globals is something that just *must* be done, but first and foremost having in mind access locality. The last thing you want to do is sacrifice cache just for that one occasional read or write (*cough* errno *cough*...)
October 30, 2018
On 10/30/2018 5:50 PM, Stanislav Blinov wrote:
>> As for encapsulating globals in a struct, I don't like it...
> That's something one has to rigorously audit at all times. Grouping of globals is something that just *must* be done, but first and foremost having in mind access locality. The last thing you want to do is sacrifice cache just for that one occasional read or write (*cough* errno *cough*...)

Grouping them together is the first step towards getting control of them. A lot of my older code has random globals scattered throughout. Fixing that is a slow process:

  https://github.com/dlang/dmd/pull/8892

That's just for one.
October 31, 2018
On Friday, 19 October 2018 at 03:53:12 UTC, Walter Bright wrote:
>
> Had a nice crowd there last night. Apparently lots of people were interested in this topic!
>
> Video: https://www.youtube.com/watch?v=lbp6vwdnE0k&feature=youtu.be


Interesting talk. Thanks for the link.

I did find it confusing however, that you discuss leaky abstractions, and putting your public interface at the beginning of your code (and all the other crap below it)... but then, in D, once your write your abstraction, say a class, with it's public interface, all the code below it can do whatever it likes to that class, making it a leaky abstraction.

That's sure sound like code smell to me.

i.e. A class (perhaps one of the most important abstractions in programming) within a module, is *always* a leaky abstraction (within the module), because of the way the code further down can just ignore the interface. In fact, there is no way at all to ensure code below the class uses that interface.

So I can't help but see contradictions everywhere, in D.

October 31, 2018
On Wednesday, 31 October 2018 at 05:00:12 UTC, myCodeDontSmell wrote:
> I did find it confusing however, that you discuss leaky abstractions, and putting your public interface at the beginning of your code (and all the other crap below it)... but then, in D, once your write your abstraction, say a class, with it's public interface, all the code below it can do whatever it likes to that class, making it a leaky abstraction.
>
> That's sure sound like code smell to me.
>
> i.e. A class (perhaps one of the most important abstractions in programming) within a module, is *always* a leaky abstraction (within the module), because of the way the code further down can just ignore the interface. In fact, there is no way at all to ensure code below the class uses that interface.
>
> So I can't help but see contradictions everywhere, in D.

That is by design, because in D the unit of abstraction is the module, not the class.
Running into such problems is a sign that your module is too large, and should become a package.
October 31, 2018
On Wednesday, 31 October 2018 at 05:00:12 UTC, myCodeDontSmell wrote:
> in D, once your write your abstraction, say a class, with it's public interface, all the code below it can do whatever it likes to that class, making it a leaky abstraction.

I think there might be some confusion between "leaky abstraction" and "insufficient encapsulation".

Here's the Wikipedia definition of leaky abstractions:

"In software development, a leaky abstraction is an abstraction that *requires* knowledge of an underlying complexity* to be able to know how to use it. This is an issue, as the whole point of an abstraction is to be able to abstract away these details from the user. "

Why would imply that: as long as the user of your class isn't *required* to know about the underlying implementation specifics, this isn't a "leaky abstraction".

My understanding is that:

"Leaky abstractions" are about the interface design, and how one component is meant to be used. Those are unrelated to the programming language (e.g translating the code to another language doesn't make the leaky abstraction disappear).

For example, a shared directory can be a leaky abstraction, if the network is unstable (because then, the client code, which only sees file descriptors, now has to deal with disappearing files).

"Encapsulation" is about implementation hiding and access control ("public/private"), and requires programming language support (e.g most dynamic languages don't have it).