2012/7/31 Timon Gehr <timon.g...@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?
