Trying out this LINQ quasi-SQL syntax
-
I started with this code:
foreach (i in Enumerable.Range(x, ship.size)) { if (blocked[i, y]) { return false; } } return true
Because I like functional style when it comes to loops, I've rewritten it as this:
return !Enumerable.Range(x, ship.size).Select(i => blocked[i, y]).Aggregate((a, b) => a || b);
I thought I might try out this special keyword-mashup syntax C# offers. I came up with this:
return !(from i in Enumerable.Range(x, ship.size) select blocked[i, y]).Aggregate((a, b) => a || b);
But I'm not quite satisfied, because there's still this ugly call to Aggregate. I went googling how to replace it with pretty magic keywords, only to learn that it's impossible. But I thought I'd make use of
where
keyword. So I tried this instead:return !(from i in Enumerable.Range(x, ship.size) where blocked[i, y] select i).Any();
But it still itches my eye because of the totally unnecessary select statement.
Is there a way to "fix" this "problem"? I just want to return
false
if any element istrue
. I don't really care how, as long as it's nice and short.
-
Maybe I'm an idiot, but why can't you do:
return !(Enumerable.Range(x, ship.size).Any(i => i.blocked[i, y]))
I don't know where y comes from though.
I also don't understand the need for the
aggregate
or theselect
, frankly. So I might be way off-base here.
-
+1 for
Any()
-
Maybe I'm an idiot, but why can't you do:
Because I learned LINQ five minutes ago, and my total accumulated experience with C# is like nine hours throughout years, six of which were just messing around with Windows Forms editor.Anyway, thanks!
BTW - redundant parentheses are redundant.
-
My personal style guys says that if you're negating a thing with a period somewhere in it, you use a paren for clarity.
Ok:
!blob; !blob(monsterExample);
Not ok:
!blob.MonsterExample(foo);
Ok:
!(blob.MonsterExample(foo));
Anyway, it's your code, do what you want. No need to chide me.
-
keyword-mashup syntax
It's confusing for no good reason. Drop it and just chain functions.
And if you did that, I think you'd see much more easily that there's no need for a Select call (which takes each element and transforms it) if you're just going to call Any() (which counts the elements) right after. Basic LINQ is much easier to digest when you think in terms of applying transforms on collections, not running SQL-style queries.
-
Because I like functional style when it comes to loops, I've rewritten it as this:
This whole topic reads to me like an excellent argument why one should stay away from functional style when it comes to loops.
I wouldn't normally have said that in here, but since your actual question has already been solved, I couldn't resist putting in my 2¢.
-
This whole topic reads to me like an excellent argument why one should stay away from functional style when it comes to loops.
Ideally, you avoid having to explicitly loop over things in the first place with functional style, instead just saying that you apply an operation to each member of a collection or filter a collection by some predicate.
-
I know what functional style is. I'm really just saying this is the Hitler of examples or something. I guess it's good for @Gaska learning LINQ or whatever, but it's shit for readability compared to what he started with.
-
I think you just don't like functional programming.
-
To be frank, both styles look like shit here, because you're trying to iterate over
blocked
, but for whatever reason, you're looping on enumerable.range instead. The way to make this make sense using functional style would bereturn blocked.Take(ship.size).Skip(x).All(isBlocked => !isBlocked[y]);
Although presumably you can't iterate over the rows of blocked like that, hence why you did it this upside-down way in the first place. In that case, a regular
for(var i=x; i<ship.size; i++)
would still have been clearer than that ridiculous foreach.
Filed under: carrying on personal vendettas in coding help
-
To be frank, both styles look like shit here, because you're trying to iterate over blocked, but for whatever reason, you're looping on enumerable.range instead.
That's because I want only the elements of blocked at specific indexes.```
return blocked.Take(ship.size).Skip(x).All(isBlocked => !isBlocked[y]);1. You messed up the order of Take and Skip. 2. This approach requires iterating from the beginning of row every time, just to ignore first x elements. While it makes sense for streaming (or arbitrary) iterators, it's stupid to do over an array. 3. You obviously omitted the part where you tell if you're iterating over the rows or columns of the 2-dimensional array. It's understandable, since C# has no such functionality (it doesn't even implement LINQ interface for multi-arrays to start with!) - nevertheless, I bet that if it had, it would make your cute line less understandable, or at least less pretty. 4. If it was me, I would use LINQ to select both row and column, not just one of them and then get the other via array syntax. Be consistent. 5. Unnecessarily long variable name in lambda expression is unnecessarily long. @Buddy <a href="/t/via-quote/50848/11">said</a>:<blockquote>Although presumably you can't iterate over the rows of blocked like that, hence why you did it this upside-down way in the first place. In that case, a regular for(var i=x; i<ship.size; i++) would still have been clearer than that ridiculous foreach.</blockquote> What's ridiculous in that `foreach` that isn't ridiculous in this `for`, except it's less portable if you ever wanted to compile my .cs file with GCC? And except this stupid "Enumerable." in front - but if you know C# well, you should have already learned to not read this word.
-
I like functional style, but I agree with @boomzilla. Too much terseness can hurt readability.
-
1. You messed up the order of Take and Skip.
Oh I see, range takes a start and count, not a start and end. Well guess what: I don't give a shit. My way makes it obvious exactly how many elements are being taken from where, and if that makes spotting bugs in my code easier that's exactly the thing point.- This approach requires iterating from the beginning of row every time, just to ignore first x elements. While it makes sense for streaming (or arbitrary) iterators, it's stupid to do over an array.
No it doesn't.
- You obviously omitted the part where you tell if you're iterating over the rows or columns of the 2-dimensional array. It's understandable, since C# has no such functionality (it doesn't even implement LINQ interface for multi-arrays to start with!) - nevertheless, I bet that if it had, it would make your cute line less understandable, or at least less pretty.
Because if you can't iterate over your object, you shouldn't be using linq on it, like I said in the first place.
- If it was me, I would use LINQ to select both row and column, not just one of them and then get the other via array syntax. Be consistent.
You aren't doing a consistent thing. You're iterating the rows, and pulling out an arbitrary column. Using the correct syntax for pulling an arbitrary column out of an array is the clearest way to express that.
- Unnecessarily long variable name in lambda expression is unnecessarily long.
It's nice there, because that way you can read the line from left to right and get a good overview of what's going on. If I knew more about what your code does i could have picked an even better name for it, and for ‘y’, so that we wouldn't be sitting here wondering wtf we're even looking at in the first place.
What's ridiculous in that foreach
What's ridiculous is that you're creating an object to produce indexes for you for absolutely no reason, when there's already an idiomatic way to do that.Again: if the object you are iterating over isn't the the target of the iteration function, you really need to rethink your approach.
-
Oh I see, range takes a start and count, not a start and end. Well guess what: I don't give a shit.
You don't give a shit that your code is no-op whenever ship size is less than its X coordinate, and if it's not, it does the wrong thing anyway except for X=0? OK.My way makes it obvious exactly how many elements are being taken from where
Mine too. You can argue that C#'s Range should take start and end, not start and count, but that's beside the point. Both our approaches are pretty equivalent here, except I'm using indexes and you're using fields.No it doesn't.
I don't know the internals of LINQ and .NET JIT, so I can't say for sure, but I'm not exactly convinced the Skip() would be optimized out.Because if you can't iterate over your object, you shouldn't be using linq on it, like I said in the first place.
Depends on what you mean by iterate. And what you refer to by "your object".You aren't doing a consistent thing. You're iterating the rows, and pulling out an arbitrary column.
I'm iterating over range of integers and accessing cells in array by providing both indices. That's consistent to me.Using the correct syntax for pulling an arbitrary column out of an array is the clearest way to express that.
Alas, C# doesn't have that.It's nice there, because that way you can read the line from left to right and get a good overview of what's going on.
Thanks to the "blocked" at the very beginning, not thanks to the duplicated "isBlocked" at the end. If anything, your line makes me think that you're checking if blocked is blocked.If I knew more about what your code does i could have picked an even better name for it
Likei
. Because the scopes of lambdas are so small that any non-single-letter identifier is just code bloat. And you don't need to know context to know that.and for ‘y’, so that we wouldn't be sitting here wondering wtf we're even looking at in the first place.
I'll provide more context at the end of this post.What's ridiculous is that you're creating an object to produce indexes for you for absolutely no reason, when there's already an idiomatic way to do that.
2007 called. They want to let you know of all the new idioms for iterating over stuff introduced in C# 3.0.Again: if the object you are iterating over isn't the the target of the iteration function, you really need to rethink your approach.
Somehow, this doesn't apply when the iteration is done with old-style for loop, apparently...
OK, some context about what I'm doing. I'm making a game of battleships. The code in OP is from ship placement function - I need to know if the squares the ship is being placed at are already occupied. I have a 2D array of bools that keeps track of fields that are already occupied (called
blocked
). Ship is defined with coordinates of top left corner (x
andy
in code), direction (horizontal or vertical), and length (size
variable; width of ship is always one field, so I don't need to store it). When ship is horizontal, I check all occupied y for given x, and if it's vertical, it's the other way around.
-
Oh, and thanks for letting me know of .All() function! Will make my code teeny tiny bit cleaner (no negating of whole expression, just the result of lambda).
-
I think you just don't like functional programming.
Not when it's worse than the equivalent in an imperative idiom.
Figuring stuff out like you did is fun to write. Just not as fun to read and deal with later. That partly depends on the subtleties involved with the actual problem being solved.
-
Figuring stuff out like you did is fun to write. Just not as fun to read and deal with later.
Shitty code is shitty regardless of paradigm. Functional style at least has the property of not randomly changing random variables everywhere...
-
-
I had a feeling that you're suggesting that functional programming is more shitty on average, or has higher chance of turning out shitty, compared to imperative.
-
I think it often tends to get extremely terse, relatively speaking. Stuff like LINQ also seems to be very dense with stuff going on. Here, I'll compress your original to make it (mostly) a one liner:
foreach (i in Enumerable.Range(x, ship.size)){ if (blocked[i, y]) { return false; } } return true;
Now, your preferred solution:
return !(Enumerable.Range(x, ship.size).Any(i => i.blocked[i, y]))
That's actually a lot better than what you originally posted. To me, the original loop is still clearer, because the intent of it really jumps out just by reading it. To understand the LINQ version, it seems like there's more jumping around, looking at the different bits to put it all together.
I could easily imagine circumstances where the styles were reversed as far as that goes.
-
Now, your preferred solution:
Update:return Enumerable.Range(x, ship.size).All(i => !blocked[i, y]);
-
To me, the original loop is still clearer, because the intent of it really jumps out just by reading it.
Well, I despise theloop { if(cond) return false; } return true;
pattern. It feels so much like a missed opportunity in this age. Kinda like making tiny class with single one-liner method instead of lambda (eg. as comparator in sort function).Also, I can imagine someone taking the imperative version and merging the "check if blocked" loop with "mark as blocked" loop that's several lines later in the code.
-
Well, I despise the loop { if(cond) return false; } return true; pattern. It feels so much like a missed opportunity in this age.
I kind of agree with you, but in a...it's the worst thing, except for all the alternatives...sort of way.
Kinda like making tiny class with single one-liner method instead of lambda (eg. as comparator in sort function).
I go back and forth on this sort of thing. On the one hand, yeah, it seems a little wasteful, but on the other hand, it also seems clearer to give something an actual name and stuff, instead of some random anonymous thing sitting around in your code. I am not at all consistent in this respect.
-
I like functional style, but I agree with @boomzilla. Too much terseness can hurt readability.
Tell that those programmers that code as if they were charged a buck per key stroke.
Anyway, I'm trying to stick to a style that enables me to maintain my own code six months later after having worked on half a dozen other projects with different languages and different style conventions. I wish to be able to kick the ass of my younger self often enough without that.
-
I go back and forth on this sort of thing. On the one hand, yeah, it seems a little wasteful, but on the other hand, it also seems clearer to give something an actual name and stuff, instead of some random anonymous thing sitting around in your code.
If lambda is instantly obvious in what it does, it doesn't need to be named. And if it's not obvious, it shouldn't be lambda.
-
is instantly obvious
Ah, the scariest words that a math teacher can utter..."It is therefore obvious that..." Obviousness is like beauty. It's entirely in the eye of the beholder.
I'm not sure I necessarily agree with your criterion for legitimate lambda-hood. But like I said, I'm not at all consistent about this, so I guess that's obvious,.
-
Ah, the scariest words that a math teacher can utter..."It is therefore obvious that..." Obviousness is like beauty. It's entirely in the eye of the beholder.
As mathematicians in Germany say, "Wie man leicht sieht" ("As can easily be seen") is defined to mean the opposite.
-
-
To me, in the LINQ version, it's really not clear what you're iterating over. What does
Range(x,ship.size)
represent? Clearer might be:return Enumerable.Range(0, ship.size).All(i => !blocked[x+i, y]);
Although this version is slightly more code, it is now clear what we are iterating over - the length of the ship.
-
-
You just need more extension methods:
public static IEnumerable<A> Column<A>(this A[][] matrix, int col) { return matrix.Select(row => row[col]); } public static IEnumerable<A> Slice<A>(this IEnumerable<A> e, int from, int length) { return e.Skip(from).Take(length); } public static bool AnyTrue(this IEnumerable<bool> e) { return e.Any(x => x); }
Then you can write:
blocked.Column(y).Slice(x, ship.size).AnyTrue()
Extension methods!
The issue isn't LINQ or functional vs imperative, it's about expressing code in a way that relates to the problem. Are any (cells?) blocked in column
y
starting fromx
, running as long as thesize
ofship
?
-
Ah, the scariest words that a math teacher can utter..."It is therefore obvious that..."
What's fun is when they work hard for an extended period of time to prove that something actually is obvious.
-
You just need more extension methods
Thanks but no thanks. These methods are too short, the usage count too small, and the original code too simple to be worth writing out.Not to mention you're using the wrong array type.
-
What's fun is when they work hard for an extended period of time to prove that something actually is obvious.
They know that everything is obvious in hindsight, so they give the students enough hindsight to make it obvious. It can take up to three school hours, though.
-
@PleegWat said:
What does Range(x,ship.size) represent?
RTFMThat was a rhetorical question. It does not represent anything very clear - it's a range of numbers that has some relation to the ship and some relation to the grid.
On the other hand,
Range(0, ship.size)
represents the range of positions the ship is occupying (in any direction) which I then map onto the grid in a second step.
In an alternative approach, you could write aship.positions
list which returns a list of (x,y) coordinates the ship occupies (again when moving in a certain direction).When working with something like LINQ, it is important to be able to know at all stages, in as simple terms as possible what your variables/values/streams represent.
-
It does not represent anything very clear - it's a range of numbers that has some relation to the ship and some relation to the grid.
Domain knowledge helps. If you know that Range takes the first value and number of consecutive values, then you look at the code and know that the range encompassesship.size
numbers fromx
onwards. If you know thatx
is the coordinate of the ship on the grid, you can easily deduce that the range is coordinates of all individual cells on the grid occupied by the ship.On the other hand, Range(0, ship.size) represents the range of positions the ship is occupying (in any direction) which I then map onto the grid in a second step.
One could argue that someone could find your mapping as bizzare as you find my range, for exactly the same reason. If someone knows what all the variables mean, both approaches are just as clean, and if they don't, both are confusing.
-
Not to mention you're using the wrong array type.
No, you're using the wrong array type!
-
I'm using multidimensional array. You're using array of arrays. Since I need multidimensional array, my type is more correct than yours. Also, while your code is short and nice in one dimension, there's no easy way to do the same in the other - and I need both.
-
I'm using multidimensional array. You're using array of arrays.
Seems the only sensible way is to use a dictionary of coordinate pairs. And to give each ship a list of the coordinate pairs it occupies. Or another representation those can be deducted from.
On a second thought no - that would be too complicated for others.
-
Seems the only sensible way is to use a dictionary of coordinate pairs. And to give each ship a list of the coordinate pairs it occupies. Or another representation those can be deducted from.
A multidimensional (i.e., 2D) array is actually a sane model of a Battleships game board.
-