www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - sorting failed error

reply maarten van damme <maartenvd1994 gmail.com> writes:
For fun I started implementing a program that uses genetics
algorithm's to solve the travelling salesman problem. I want to sort
an array of chromosome structures according to their fitness. I now
get the error:

core.exception.AssertError C:\D\dmd2\windows\bin\..\..\src\phobos\std\algorithm.
d(6714): Failed to sort range of type chromosome[]. Actual result is: [chromosom
e([city(20, 0), city(25, 25), city(10, 65), city(50, 50), city(75, 30), city(0,
10)]), chromosome([city(10, 65), city(50, 50), city(25, 25), city(75, 30), city(
...

I have no idea what is wrong with my code and the error is not very informative.
Does anyone have any idea as to what the problem could be?

(full code is at
https://dl.dropbox.com/u/15024434/travelling_salesman.d , If it's not
something obvious then I'm going to try to reduce it to something as
small as possible)

Maarten
Jul 30 2012
next sibling parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 12:36:21 UTC, maarten van damme wrote:

 For fun I started implementing a program that uses genetics
 algorithm's to solve the travelling salesman problem. I want to 
 sort
 an array of chromosome structures according to their fitness. I 
 now
 get the error:

 core.exception.AssertError C:\D\dmd2\windows\bin\..\..\src\phobos\std\algorithm.
 d(6714): Failed to sort range of type chromosome[]. Actual 
 result is: [chromosom
 e([city(20, 0), city(25, 25), city(10, 65), city(50, 50), 
 city(75, 30), city(0,
 10)]), chromosome([city(10, 65), city(50, 50), city(25, 25), 
 city(75, 30), city(
 ...

 I have no idea what is wrong with my code and the error is not 
 very informative.
 Does anyone have any idea as to what the problem could be?

 (full code is at
 https://dl.dropbox.com/u/15024434/travelling_salesman.d , If 
 it's not
 something obvious then I'm going to try to reduce it to 
 something as
 small as possible)

 Maarten


I couldn't have access to your code, but my *guess* would be that 
the provided "less", or in your probable case, your "opCmp", is 
not transitive.

Eg, if you have 3 cities A, B and C, then if A<B and B<C, then 
you MUST have A<C.

Could you post here your implementation of opCmp?

If you are trying to implement traveling salesman, then I don't 
think "sort" is the algorithm should be using.
Jul 30 2012
parent reply maarten van damme <maartenvd1994 gmail.com> writes:
My cmp is :

bool fitnessCompare(chromosome first,chromosome second){
		return fitness(first)>fitness(second);
}

float fitness(chromosome victim){
	city[] cities=victim.dna;
	
	//we need to start from home and return to home
	cities=city(0,0) ~ cities;
	cities~=city(0,0);
	
	float travelled=0f;
	for(int x=0;x<cities.length-1;x++)
		travelled+=distance(cities[x],cities[x+1]);

	return 1/travelled;
}



I've posted my code too on pastebin : http://pastebin.com/7927Hpv2
Jul 30 2012
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 13:27:58 UTC, maarten van damme wrote:

 My cmp is :

 bool fitnessCompare(chromosome first,chromosome second){
 		return fitness(first)>fitness(second);
 }

 float fitness(chromosome victim){
 	city[] cities=victim.dna;
 	
 	//we need to start from home and return to home
 	cities=city(0,0) ~ cities;
 	cities~=city(0,0);
 	
 	float travelled=0f;
 	for(int x=0;x<cities.length-1;x++)
 		travelled+=distance(cities[x],cities[x+1]);

 	return 1/travelled;
 }



 I've posted my code too on pastebin : 
 http://pastebin.com/7927Hpv2


That seems like a reasonable cmp function. But on the expensive 
side, but that is irrelevant.

I can't access any internet share sites from where I am, so I 
can't dig in much further than that. The assertion you are 
hitting is one that is called after the sort is done, to make 
sure the elements are indeed sorted. In your case, after the 
sort, the elements are not sorted :/

The way I see it, there could be 1 of 2 issues:
*A bad cmp function.
*Data that mutates during sort.

Your looks OK, and I doubt you are mutating. I only see floating 
point gotchas:
If a chromosome travels nothing, then his fitness is 1/0 = NaN.

NaN then compares false with everything, making it un-transitive, 
and potentially breaking your cmp. COuld you try the algo with 
"return 1/(1+travelled)".

That, or because of the non-deterministic nature of floating 
point calculation, I'd create an array containing all the 
fitnesses, and sort using that. This should avoid any 
"re-evaluation error".
Jul 30 2012
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 07/30/2012 03:52 PM, monarch_dodra wrote:

 ...
 Your looks OK, and I doubt you are mutating. I only see floating point
 gotchas:
 If a chromosome travels nothing, then his fitness is 1/0 = NaN.


1/0 evaluates to Inf


 NaN then compares false with everything, making it un-transitive, and
 potentially breaking your cmp. COuld you try the algo with "return
 1/(1+travelled)".

 That, or because of the non-deterministic nature  of floating point
 calculation,


Floating-point computation is deterministic.


 I'd create an array containing all the fitnesses, and sort
 using that. This should avoid any "re-evaluation error".
Jul 30 2012
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 22:23:10 UTC, Timon Gehr wrote:

 On 07/30/2012 03:52 PM, monarch_dodra wrote:

 ...
 NaN then compares false with everything, making it 
 un-transitive, and
 potentially breaking your cmp. COuld you try the algo with 
 "return
 1/(1+travelled)".

 That, or because of the non-deterministic nature  of floating 
 point
 calculation,


 Floating-point computation is deterministic.


Is this specific to D? Because it isn't in C++:

http://www.parashift.com/c++-faq-lite/floating-point-arith2.html
Jul 30 2012
next sibling parent reply maarten van damme <maartenvd1994 gmail.com> writes:
I now tried to bring it to the next level, using concurrency to bread
a couple of populations and make the best solutions migrate between
them.
But, to use concurrency in D, one has to continually cast between
immutable, cast immutable away, ....
Not only that, casting away from immutable (or too, the error message
is not informative at all) you get error's along the line of "opEquals
doesn't work with immutable arguments"...

Why is it that cumbersome? It seems like D took a good idea and ruined
it with all that const-stuff.

I have the feeling that I've completely missed the whole idea on how
concurrency in D should work. Is it normal that you have to either
mess with immutable or with the broken shared?
Am I implementing everything wrong?
Jul 31 2012
parent "Minas" <minas_mina1990 hotmail.co.uk> writes:
On Tuesday, 31 July 2012 at 09:41:31 UTC, maarten van damme wrote:

 I now tried to bring it to the next level, using concurrency to 
 bread
 a couple of populations and make the best solutions migrate 
 between
 them.
 But, to use concurrency in D, one has to continually cast 
 between
 immutable, cast immutable away, ....
 Not only that, casting away from immutable (or too, the error 
 message
 is not informative at all) you get error's along the line of 
 "opEquals
 doesn't work with immutable arguments"...

 Why is it that cumbersome? It seems like D took a good idea and 
 ruined
 it with all that const-stuff.

 I have the feeling that I've completely missed the whole idea 
 on how
 concurrency in D should work. Is it normal that you have to 
 either
 mess with immutable or with the broken shared?
 Am I implementing everything wrong?


Use std.parallelism. It has everything that (I think) you need, 
e.g. support for parallel for, tasks and more.
Jul 31 2012
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, July 31, 2012 11:41:19 maarten van damme wrote:

 I now tried to bring it to the next level, using concurrency to bread
 a couple of populations and make the best solutions migrate between
 them.
 But, to use concurrency in D, one has to continually cast between
 immutable, cast immutable away, ....
 Not only that, casting away from immutable (or too, the error message
 is not informative at all) you get error's along the line of "opEquals
 doesn't work with immutable arguments"...
 
 Why is it that cumbersome? It seems like D took a good idea and ruined
 it with all that const-stuff.
 
 I have the feeling that I've completely missed the whole idea on how
 concurrency in D should work. Is it normal that you have to either
 mess with immutable or with the broken shared?
 Am I implementing everything wrong?


To pass something across threads, it either needs to be a value type, or 
shared, or immutable. Unfortunately, shared doesn't work std.concurrency right 
now though, so you're stuck using immutable. What we really need is a way to 
indicate that ownership is being passed from one thread to another, but that 
can't really be done with the current type system. However, there are probably 
tweaks that can be made to make it more pleasant to deal with.

As for "messing it up with const", immutability (and therefore const) is a 
core part of D's threading model. By making everything thread-local by 
default, you then need a way to share instances across threads, and that 
requires shared (and immutable is implicitly shared). And with immutable, that 
can be done much more efficiently, because then the compiler doesn't have to 
worry about multiple threads screwing with the variable.

So, the situation isn't perfect and still needs some work, but the basic idea 
is solid and does work, even if it deosn't yet work quite as well as we'd 
like.

- Jonathan M Davis
Jul 31 2012
prev sibling next sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
maarten van damme:


 I have no idea what is wrong with my code and the error is not 
 very informative.


As first step I suggest to add a space after commas, before 
brackets, around operators, etc. The improved readability helps 
spot the bugs.

Adding Contracts and other asserts sometimes helps. Adding some 
unit tests sometimes helps.

Bye,
bearophile
Jul 30 2012
parent reply maarten van damme <maartenvd1994 gmail.com> writes:
it is guaranteed to never be 0 because it always contains all cities
so it will always travel something unless there are no cities or when
all cities are in the same place (in which case the task to solve is
senseless).

I'll create an array containing all finesses and try sorting using that.

And bearophile, you're right. it was supposed to be a very quick
program just for fun to see how powerful genetics can be. (read about
them yesterday.Never looked at a complete implementation and I'm
trying to see how far I'd get with some simple rules. My cross over
function isn't really that good...)

I really dislike contracts, they kind off blow your function up while
adding not that much. I'll add some unit tests.
Jul 30 2012
parent reply "bearophile" <bearophileHUGS lycos.com> writes:
maarten van damme:


 I really dislike contracts, they kind off blow your function up 
 while adding not that much. I'll add some unit tests.


They make your programs more robust. Contracts are used to spot 
bugs in your code and not to validate data coming from the 
"outside world".

As most other tools there are few ways to use them well, and many 
ways to use them badly. I suggest you to read about the meaning 
of Contract programming, so maybe you will learn to appreciate 
them.

Bye,
bearophile
Jul 30 2012
parent reply maarten van damme <maartenvd1994 gmail.com> writes:
2012/7/30 bearophile <bearophileHUGS lycos.com>:

 maarten van damme:



 I really dislike contracts, they kind off blow your function up while
 adding not that much. I'll add some unit tests.



 They make your programs more robust. Contracts are used to spot bugs in your
 code and not to validate data coming from the "outside world".

 As most other tools there are few ways to use them well, and many ways to
 use them badly. I suggest you to read about the meaning of Contract
 programming, so maybe you will learn to appreciate them.

 Bye,
 bearophile


I certainly will. Andrei's chapter didn't convince me but maybe
reading something more specialized to contract programming
specifically will convince me :)

About my pass-by-value problem, I am not used to working with structs,
I usually work with classes. That's why I completely forgot about
post-blit constructors... Still my sorting problem isn't sorted out.
Jul 30 2012
parent reply "bearophile" <bearophileHUGS lycos.com> writes:
maarten van damme:


 Still my sorting problem isn't sorted out.


Another possible cause: in D if you sort structs that contain 
dynamic arrays, the actual contents of the arrays is ignored. 
This is a HUGE bug in DMD.

Bye,
bearophile
Jul 30 2012
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 18:50:24 UTC, bearophile wrote:

 maarten van damme:


 Still my sorting problem isn't sorted out.


 Another possible cause: in D if you sort structs that contain 
 dynamic arrays, the actual contents of the arrays is ignored. 
 This is a HUGE bug in DMD.

 Bye,
 bearophile


Could you share the bug report? This works fine for me... :

----
import std.stdio;
import std.range;
import std.algorithm;
struct S
{
     int opCmp(S b)
     {
       if(v[0] < b.v[0]) return -1;
       if(v[0] > b.v[0]) return  1;
       return 0;
     }
     int[] v;
}

void main()
{
   S[3] ss;
   ss[0].v = [1, 2].array();
   ss[1].v = [5, 6].array();
   ss[2].v = [3, 4].array();
   writeln(ss[]);
   ss[].sort();
   writeln(ss[]);
}
----

Or did you mean something else?
Jul 30 2012
parent reply "bearophile" <bearophileHUGS lycos.com> writes:
monarch_dodra:


 struct S
 {
     int opCmp(S b)
     {
       if(v[0] < b.v[0]) return -1;
       if(v[0] > b.v[0]) return  1;
       return 0;
     }
     int[] v;
 }
 ...
 Or did you mean something else?


I meant without a definend opCmp. So this is not your problem.

Bye,
bearophile
Jul 30 2012
next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 19:17:30 UTC, bearophile wrote:

 monarch_dodra:


 struct S
 {
    int opCmp(S b)
    {
      if(v[0] < b.v[0]) return -1;
      if(v[0] > b.v[0]) return  1;
      return 0;
    }
    int[] v;
 }
 ...
 Or did you mean something else?


 I meant without a definend opCmp. So this is not your problem.

 Bye,
 bearophile


I don't get it? If you don't define an opCmp, then you can't 
compare the elements. At that point, sort won't "fail", it just 
won't compile...?
Jul 30 2012
prev sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Monday, July 30, 2012 21:17:29 bearophile wrote:

 I meant without a definend opCmp. So this is not your problem.


I'd actually argue that structs without opCmp shouldn't have it implicitly 
defined. It's just begging for bugs otherwise.

- Jonathan M Davis
Jul 30 2012
parent reply "bearophile" <bearophileHUGS lycos.com> writes:
Jonathan M Davis:


 I'd actually argue that structs without opCmp shouldn't have it 
 implicitly defined. It's just begging for bugs otherwise.


For reference, this is the issue we are talking about:
http://d.puremagic.com/issues/show_bug.cgi?id=3789

The current situation is totally not acceptable. Even doing what 
I think you are suggesting, that is generating a compile-time 
error, is better than the current situation, that leads to silent 
bugs.

Bye,
bearophile
Jul 30 2012
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 31 July 2012 at 00:21:59 UTC, bearophile wrote:

 Jonathan M Davis:


 I'd actually argue that structs without opCmp shouldn't have 
 it implicitly defined. It's just begging for bugs otherwise.


 For reference, this is the issue we are talking about:
 http://d.puremagic.com/issues/show_bug.cgi?id=3789

 The current situation is totally not acceptable. Even doing 
 what I think you are suggesting, that is generating a 
 compile-time error, is better than the current situation, that 
 leads to silent bugs.

 Bye,
 bearophile


Oh, thanks. Because we were talking about sorting, and you spoke 
of "comparison", I didn't get you meant comparison in the 
opEquals way (as opposed to equivalence).

Thanks for taking the time to look for the bug for me.
Jul 30 2012
prev sibling next sibling parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 12:36:21 UTC, maarten van damme wrote:

 For fun I started implementing a program that uses genetics
 algorithm's to solve the travelling salesman problem. I want to 
 sort
 an array of chromosome structures according to their fitness. I 
 now
 get the error:

 core.exception.AssertError C:\D\dmd2\windows\bin\..\..\src\phobos\std\algorithm.
 d(6714): Failed to sort range of type chromosome[]. Actual 
 result is: [chromosom
 e([city(20, 0), city(25, 25), city(10, 65), city(50, 50), 
 city(75, 30), city(0,
 10)]), chromosome([city(10, 65), city(50, 50), city(25, 25), 
 city(75, 30), city(
 ...

 I have no idea what is wrong with my code and the error is not 
 very informative.
 Does anyone have any idea as to what the problem could be?

 (full code is at
 https://dl.dropbox.com/u/15024434/travelling_salesman.d , If 
 it's not
 something obvious then I'm going to try to reduce it to 
 something as
 small as possible)

 Maarten


Just saw your code. Intriguing.

I'll have to stick with my gut feeling that sorting values by 
calculating a floating point value on the fly is not optimal.

BTW: This:
workers=array(sort!(fitnessCompare)(workers));
is equivalent to:
workers.sort!fitnessCompare();
The point of sort's return value is that it is typed as sorted, 
but you don't need to array->reassign.


Now, if instead of defining fitnessCompare, you use 
"schwartzSort", a sort made to sort things depending on the 
result of a function, calling the function one (AND ONLY ONCE) 
for each value:

Replace
workers.sort!fitnessCompare();
with
workers.schwartzSort!(fitness,"a > b")();

Then the sort works correctly. And schwartzSort ALSO validates 
the resulting sort.

Sorry I can't *certify* what the problem is, but at least you 
have a workaround ;)
Jul 30 2012
parent reply maarten van damme <maartenvd1994 gmail.com> writes:
Great, with that workaround everything works correctly now.

I can finally start playing around with my salesman's dna :p

There is one little problem left, the comparing problem. I can't
really define an opcmp because a city isn't smaller or bigger than
another city, it's simply another city. I tried defining opEquals but
it still refuses to work (could be a problem on my end though).

To clear things out: Comparing an array of structs with inside a
dynamic array fails unless you add an opCmp (?or opEquals?) function
to that struct.

I thought comparing failed because the references to those dynamic
arrays were different and that he was comparing references instead of
values.
Jul 30 2012
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 20:01:59 UTC, maarten van damme wrote:

 Great, with that workaround everything works correctly now.

 I can finally start playing around with my salesman's dna :p

 There is one little problem left, the comparing problem. I can't
 really define an opcmp because a city isn't smaller or bigger 
 than
 another city, it's simply another city. I tried defining 
 opEquals but
 it still refuses to work (could be a problem on my end though).


Why do you want to sort the cities?

you need to define an opCmp to do that (not opEquals).

While you *can* deterministically sort the cities using an 
arbitrary norm (eg Manhattan distance), the end result will be 
mostly an artificial ordering.
http://en.wikipedia.org/wiki/Manhattan_distance
Jul 30 2012
prev sibling next sibling parent reply Ellery Newcomer <ellery-newcomer utulsa.edu> writes:
On 07/30/2012 05:36 AM, maarten van damme wrote:

 I have no idea what is wrong with my code and the error is not very
informative.
 Does anyone have any idea as to what the problem could be?


Congratulations, it looks like you've hit a compiler bug.

http://d.puremagic.com/issues/show_bug.cgi?id=8476

quickest fix: change return type of fitness to real.
Jul 30 2012
parent maarten van damme <maartenvd1994 gmail.com> writes:
monarch_dodra, I'm not trying to order cities, I'm trying to filter
out duplicate's in my dna chromosomes and "==" isn't working on
structs that encapsulates dynamic arrays.

And I can compare elements without defining opCmp. I've written a
function that calculates the distance travelled when going to the
cities according to that dna and sort using that.

It looks like you didn't really get how or what I'm trying to do (I
don't blame you, I'm terrible at explaining things). You should really
look at the code, most things are more obvious there.

Ellery,
When I change it so it uses double
Jul 30 2012
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 07/30/2012 02:36 PM, maarten van damme wrote:

 For fun I started implementing a program that uses genetics
 algorithm's to solve the travelling salesman problem. I want to sort
 an array of chromosome structures according to their fitness. I now
 get the error:

 core.exception.AssertError C:\D\dmd2\windows\bin\..\..\src\phobos\std\algorithm.
 d(6714): Failed to sort range of type chromosome[]. Actual result is:
[chromosom
 e([city(20, 0), city(25, 25), city(10, 65), city(50, 50), city(75, 30), city(0,
 10)]), chromosome([city(10, 65), city(50, 50), city(25, 25), city(75, 30),
city(
 ...

 I have no idea what is wrong with my code and the error is not very
informative.
 Does anyone have any idea as to what the problem could be?

 (full code is at
 https://dl.dropbox.com/u/15024434/travelling_salesman.d , If it's not
 something obvious then I'm going to try to reduce it to something as
 small as possible)

 Maarten


I'd claim it is a 32 bit specific compiler bug, but it is likely that
someone will differ.



reduced test case:

float foo(){
   auto f = 3f;
   return 1/f;
}
void main(){ assert(foo() == foo()); }

The problem is presumably that one of the operands is truncated to
float while the other remains at register precision.



workaround:

bool fitnessCompare(chromosome first,chromosome second){
   auto a = fitness(first), b = fitness(second);
   return a>b;
}

(the reason schwartzSort works is because it caches the fitnesses,
which is better anyway.)




I realize that the code is just for fun, but I have some comments:

- don't .dup stuff just in order to index directly. it never has
any effect other than performance degradation. (this could be a
compiler warning)

- crossOver and mutate mutate their parameters in-place. I assume this
is by mistake. reproduce effectively inserts every element a couple
times because of this -- this is why the bug manifests reliably.
(const, immutable annotations?)

- make use of $

int from=uniform(0,victim.dna.length);
int to=uniform(0,victim.dna.length);
swap(victim.dna[from],victim.dna[to]);

=>

swap(victim.dna[uniform(0,$)],
      victim.dna[uniform(0,$)]);

- sort is in-place

workers=array(sort!(fitnessCompare)(workers));

=>

sort!fitnessCompare(workers)

- mark local functions static if they do not need to access the
enclosing context.

- use size_t for array iteration variables (if necessary)

for(int x=0;x<cities.length-1;x++)
     travelled+=distance(cities[x],cities[x+1]);

=>

for(size_t x=1;x<cities.length;x++)
     travelled+=distance(cities[x-1],cities[x]);

I also removed the subtraction from the array length. This would be
correct in this case because cities.length>=2 is guaranteed, but it is
an error prone pattern.

- another way to do it is to drop the loop completely and to use
ranges instead:

return zip(cities,cities.drop(1))
       .map!(a=>distance(a.expand))
       .reduce!"a+b";


The benefit is that this is now obviously correct.


You might also want to consider not building up the cities array
and computing the terms at the boundary explicitly instead:

return distance(city(0,0), victim.dna[0]) +
        distance(victim.dna[$-1], city(0,0)) +
        zip(victim.dna, victim.dna.drop(1))
       .map!(a=>distance(a.expand))
       .reduce!"a+b";

The goal is to craft the shortest code possible that is still
efficient and easy to follow.

- type deduction?


further comments whose application does not lead to immediate benefit:

- in contracts are better specified in their dedicated section to push
the requirements onto the caller.

- consider for(;;) as a means for indefinite looping.

- the convention is upper case for type names
Jul 30 2012
parent reply maarten van damme <maartenvd1994 gmail.com> writes:
2012/7/31 Timon Gehr <timon.gehr gmx.ch>:

 I realize that the code is just for fun, but I have some comments:

 - don't .dup stuff just in order to index directly. it never has
 any effect other than performance degradation. (this could be a
 compiler warning)

 - crossOver and mutate mutate their parameters in-place. I assume this
 is by mistake. reproduce effectively inserts every element a couple
 times because of this -- this is why the bug manifests reliably.
 (const, immutable annotations?)

 - make use of $

 int from=uniform(0,victim.dna.length);
 int to=uniform(0,victim.dna.length);
 swap(victim.dna[from],victim.dna[to]);

 =>

 swap(victim.dna[uniform(0,$)],
      victim.dna[uniform(0,$)]);

 - sort is in-place

 workers=array(sort!(fitnessCompare)(workers));

 =>

 sort!fitnessCompare(workers)

 - mark local functions static if they do not need to access the
 enclosing context.

 - use size_t for array iteration variables (if necessary)


 for(int x=0;x<cities.length-1;x++)
     travelled+=distance(cities[x],cities[x+1]);

 =>

 for(size_t x=1;x<cities.length;x++)
     travelled+=distance(cities[x-1],cities[x]);

 I also removed the subtraction from the array length. This would be
 correct in this case because cities.length>=2 is guaranteed, but it is
 an error prone pattern.

 - another way to do it is to drop the loop completely and to use
 ranges instead:

 return zip(cities,cities.drop(1))
       .map!(a=>distance(a.expand))
       .reduce!"a+b";


 The benefit is that this is now obviously correct.


 You might also want to consider not building up the cities array
 and computing the terms at the boundary explicitly instead:

 return distance(city(0,0), victim.dna[0]) +
        distance(victim.dna[$-1], city(0,0)) +
        zip(victim.dna, victim.dna.drop(1))
       .map!(a=>distance(a.expand))
       .reduce!"a+b";

 The goal is to craft the shortest code possible that is still
 efficient and easy to follow.

 - type deduction?


 further comments whose application does not lead to immediate benefit:

 - in contracts are better specified in their dedicated section to push
 the requirements onto the caller.

 - consider for(;;) as a means for indefinite looping.

 - the convention is upper case for type names


Thank you very much for this criticism, I'm relatively new to
programming and these mistakes/points are the kind off things you
can't learn from reading a book or two.

I have one little question about one of the last points though
why use for(;;)?
As far as I can tell this simply reduces readability from while(true).
Is there any reason for this?
Jul 30 2012
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 07/31/2012 12:30 AM, maarten van damme wrote:

 2012/7/31 Timon Gehr<timon.gehr gmx.ch>:

 ...
 further comments whose application does not lead to immediate benefit:

 - in contracts are better specified in their dedicated section to push
 the requirements onto the caller.

 - consider for(;;) as a means for indefinite looping.

 - the convention is upper case for type names


 Thank you very much for this criticism, I'm relatively new to
 programming and these mistakes/points are the kind off things you
 can't learn from reading a book or two.


I'm glad to help.


 I have one little question about one of the last points though
 why use for(;;)?


Well, as stated, there is no benefit.


 As far as I can tell this simply reduces readability from while(true).


That is entirely a matter of preference.


 Is there any reason for this?


I like it more because it says "loop".
while(true) says: "loop as long as 'true' still holds", which is silly.
Jul 30 2012
next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 7/31/12, Timon Gehr <timon.gehr gmx.ch> wrote:

 I like it more because it says "loop".


I'd love to have "loop { }". But keyword bloat yada yada. :)
Jul 30 2012
prev sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 30 July 2012 at 22:58:28 UTC, Timon Gehr wrote:

 On 07/31/2012 12:30 AM, maarten van damme wrote:

 2012/7/31 Timon Gehr<timon.gehr gmx.ch>:

 ...
 further comments whose application does not lead to immediate 
 benefit:

 - in contracts are better specified in their dedicated 
 section to push
 the requirements onto the caller.

 - consider for(;;) as a means for indefinite looping.

 - the convention is upper case for type names


 Thank you very much for this criticism, I'm relatively new to
 programming and these mistakes/points are the kind off things 
 you
 can't learn from reading a book or two.


 I'm glad to help.


 I have one little question about one of the last points though
 why use for(;;)?


 Well, as stated, there is no benefit.


 As far as I can tell this simply reduces readability from 
 while(true).


 That is entirely a matter of preference.


 Is there any reason for this?


 I like it more because it says "loop".
 while(true) says: "loop as long as 'true' still holds", which 
 is silly.


I've usually see the "for(;;)" syntax to loop infinity: This 
reads as: Just keep looping, with no condition.
Jul 30 2012