• Prime Mover (unregistered)

    I wonder whether someone has inherited this routine, and been told "Add comments to document what it does, but don't change anything."

  • DQ (unregistered)

    To bad I'm not the first to comment

  • Industrial Automation Engineer (unregistered)

    That code looks.... random

  • Cheeseman (unregistered)

    Seems like one "developer" wrote the (uncommented) code and another one had the sad job of fixing some bug within and decided to comment the code after understanding the mess, so that at least his successors would understand "why".

    And all this mess just looks like availableList = Optional.ofNullable(availableList).orElse(new ArrayList<>()); availableList.removeAll(excludeList); Collections.shuffle(availableList); return availableList.stream().limit(numToDraw).collect(Collectors.toList());

  • (nodebb) in reply to Cheeseman

    If the items are distinct and the number of items to remove is significant, I'd make them a set first to make the removal step much faster. After all, the order of the items really isn't very important if you're going to be picking a random subset of them.

    But this is C# and not Java, so the way you write that down is going to be a bit different.

  • my name is missing (unregistered)

    The most horrifying thing about this code is that someone was paid to write it.

  • redshirt (unregistered) in reply to Cheeseman

    I don't think this was a case of "phostumous documenting".

    Those comments lack the displeasure with the way the Original Coder decided to implement this feature, the sarcasm of someone forbidden to change stupid stuff, and the existential dread of someone who realizes that humanity will never stop sprouting junk code like this.

    I might have had to add comments to established projects full of WTFs, yes.

  • OldCoder (unregistered) in reply to my name is missing

    Actually, I wonder if someone was made to write this!

    Imagine a newby with a good knowledge of C# and only a little experience in this field. Then imagine a senior programmer^H^H lead developer, whatever... who says, Here's the spec. Never mind all your fancy lists and functions, I don't care what you thjink you know, I want it written this way. Oh, and by the way, why haven't you finished it already?

  • David Delbecq (unregistered)

    I like how this code will never return if there are too many items in excludedlist compared to number to draw. But that's cool, code will progressively slow down when we are approaching that limit :D. Enough time to fix it because of client complains :)

  • (nodebb)

    Yes, the implementation is a complete WTF... But the initial comment about the number of ViewModels likely is not. Each ViewModel should be used by A (one, single, solitary) View. It is the adapter for the common (often aggregated) Model.

  • Charles (unregistered)

    I see that addedIndex is updated, but never used. And it's probably redundant because the chosen item is marked in availableList[N].IsSelected For bonus points, this might be the source of the information in excludedList, making that redundant also.

    Further fun occurs with duplicate choices. If you have already chosen 0, 1, 2 and 7 out of 10, then you can choose 7 again, but not 0, 1, or 2. This is very unlikely to be correct. The test at the beginning to ensure there are at least as many items as numToDraw suggests that duplicates are not supposed to happen (since otherwise you can choose any number of items from a non-empty list).

    Despite the comments, sorting is probably bad. That's O(NlogN). Use bitsets. Setup in O(N) and choices are O(1) unless you're trying to choose all, or almost all, items from a large range that is almost already chosen except for two widely spaced items.

  • Peter D (unregistered)

    This almost certainly has been through a bunch of iterations. I imagine the first version picked a small percentage (maybe 5 out a list of hundreds) from a single list. But it was found to be very slow when the number of picks approached the size of the list (so they added all the min and max crap). Someone else complained that the list wasn't random enough (add the new seeds). And at some point, someone wanted to add the exclude list as a parameter, maybe because someone wanted to call this function recursively.

  • Annnnnd.... (unregistered)

    We swap it out aaaaannnd down the drain the original code goes, never to be seen again.

  • (nodebb)

    Of course they have to create a new Random object each time, otherwise they could end up reusing someone else's random numbers!

  • Chris (unregistered) in reply to TheCPUWizard

    While you may have many ViewModels, this code probably doesn't belong in any of them. It should be in a separate class, and certainly should not have been copy-pasted into many different places.

  • jochem (unregistered)

    That final comment about the sorted list/hashset is probably by one of the developers who had to maintain this crap. But he probably couldn't change it, as its not tested, and maybe other code depends on the side effects of this method. Very frustrating...We've all been there...

  • Prime Mover (unregistered) in reply to OldCoder

    If that's the case, then for a start this:

    	if (availableList == null || numToDraw > availableList.Count)
	{
		// Can't draw the required number of objects
		return new List<MyObject>();
	}
	
	List<MyObject> result = new List<MyObject>();

    would have been this:

    	List<MyObject> result = new List<MyObject>();

	if (availableList == null || numToDraw > availableList.Count)
	{
		// Can't draw the required number of objects
		return result;
	}

    but as it's not, you can assume the coder was a careless noob.

Leave a comment on “A Sort of Random”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article