48
Seems to be swatting a fly with a Buick to me. The first form seems pretty concise and the variable names are quite descriptive. The second form creates a new object (the array) which will eventually have to be GC'd and introduces a new lambda variable, c which doesn't seem descriptive any more.
48
Impressive. I mean it.
Couple observations:
Your enums...
public enum SudokuProgress { FAILED, NO_PROGRESS, PROGRESS }
Should be:
public enum SudokuProgress { Failed, NoProgress, Progress }
When the first thing you see is this:
public class SudokuBoard
{
public SudokuBoard(SudokuBoard copy)
{
_maxValue = copy._maxValue;
tiles = ...
41
Your loop is close to 200 lines, that's waaaay too long. That's long for a full class, let alone one method, let alone part of one method.
Why do you use for (int i = 1; i < count - 1; i++) when you don't do anything with i? Why not use foreach(var line in lines)? Oh wait, I see, you've called your collection line -- a bad name, since this doesn't tell ...
38
I created an extension method for DataTable to convert them into a List<T>
public static class Helper
{
/// <summary>
/// Converts a DataTable to a list with generic objects
/// </summary>
/// <typeparam name="T">Generic object</typeparam>
/// <param name="table">DataTable</param>
/// <...
29
Fancy is not the word for it. This is insane. Ever heard of the KISS principle? Keep-it-simple-stupid. Unless of course you are intentionally looking for ways to make your code look obfuscated, and your executable file bloated.
24
Overall, I like this. It is concise, and neat, and all. My only beef is the nested ternaries.
In general, ternary operator precedence is complicated by the lack of blocks.... The need for the parenthesis on the modulo precedence makes the conditions clear ( (n % 15 == 0) ) and the actual indenting you have, makes the logic a little clearer, but precedence ...
24
There is a simple solution that does not require a dictionary:
int[] A = { 9, 3, 9, 3, 9, 7, 9};
int result = 0;
foreach (var i in A)
result ^= i;
Console.WriteLine(result); // 7
The trick here is, that "a xor a" gives 0. So all number will be erased except the single one.
Or in short:
var result = new [] { 9, 3, 9, 3, 9, 7, 9}.Aggregate(0, (a, b) =&...
23
Can't read all this code on my phone even though it looks pretty well structured to me! Good job!
I saw this. Isn't the exception message contradicting the if clause?
if (value < CLEARED)
throw new ArgumentOutOfRangeException("SudokuTile Value cannot be zero or smaller. Was " + value);
CLEARED is set to 0, and the if checks for 'less than 0' so ...
22
As a Java developer, the first is much easier to read. It could be confusing too: why the author use LINQ instead of a simple ||? It reminds me the law of the instrument: "if all you have is a hammer, everything looks like a nail".
Anyway, maybe it's worth creating a well-named local variable for the condition which could help the readers:
boolean needName ...
22
It is possible to solve the problem without sorting the array (which is expensive). The input array is actually a simple arithmetic sequence 1, 2, 3, 4 ... N+1, the numbers are just in a jumbled up order and one of them is missing.
The sum of the sequence is easy to calculate as n/2(1+n) (here n = N + 1). The sum of the input array will be the same as this, ...
22
Use SelectMany. As in, allPersons.SelectMany(s => s);. This flattens the sequence for you.
answered Jan 10 at 13:24
23fc9a62-56de-47fb-97b4-737890
17.7k5 gold badges69 silver badges161 bronze badges
21
Something like this should work:
public static string ToString(this Dictionary<string,string> source, string keyValueSeparator, string sequenceSeparator)
{
if (source == null)
throw new ArgumentException("Parameter source can not be null.");
var pairs = source.Select(x => string.Format("{0}{1}{2}", x.Key, keyValueSeparator, x.Value));
...
20
This is amazing. Especially for person who is not using C# every day for years.
My main concerns are
too many thing that are public that should be internal, sometimes internal members can be turned to private. Use the most restrictive access level that makes sense for a particular member.
error-prone method AddRow of SudokuBoard. I'd prefer single string ...
19
I know next to zilch about C# so I won't be much help with this review, but I can say that it looks well thought out and implements some cool features. And like retailcoder I'm still working on my version (hoping to minimize the brute-force portion) using Ruby.
If C# allows enums to implement methods, I would move CombineSolvedState into SudokuProgress. ...
19
I'm not a member of the “goto is evil and must not be used under any circumstances” camp. Especially in performance-critical low-level routines, it can be useful from time to time. However, most of the time, the temptation to use goto actually stems from missing a better opportunity to structure the code. In your example, I'd simply re-structure it like ...
18
I think the reason it's done this way is to make GroupBy() consistent with the rest of LINQ: everything is as lazy as possible, so that following queries can be efficient.
And there already is a method that does almost the same thing as your GroupToDictionary(), it's called ToLookup().
18
There are several ways to achieve what you're after and it depends on whether you want the results drip fed to you as they're available or whether you're happy to have them all in one bang.
The way you've implemented your method gives it all in one bang - which is fine.
A shorter implementation could be
public static async Task<IEnumerable<T>> ...
18
I'd prefer to use the String.Join method instead of the Aggregate:
public static string FizzBuzz(int start, int end)
{
return String.Join(" ", Enumerable.Range(start, end - start + 1).Select(FizzOrBuzz));
}
This should eliminate multiple string concatenations.
EDIT. Removed the superfluous generic type parameters.
18
You could use goto, but you can also just define a function, as the label returns a value anyways. For example, your label FAILED:, would become the following function.
public IQueryable<T> Failed()
{
if (defaultSort != null)
{
query = query.OrderBy(defaultSort);
}
return query;
}
You shouldn't ever use goto for flow control, ...
17
You can write your Box method without foreach loops or yield return:
public static IEnumerable<Tuple<int, int>> Box(int sizeX, int sizeY)
{
return
from x in Enumerable.Range(0, sizeX)
from y in Enumerable.Range(0, sizeY)
select Tuple.Create(x,y);
}
or equivalently:
public static IEnumerable<Tuple<int, int>&...
17
Instead of using linq lookups for the lists you should map your data into dictionaries, which are MUCH faster to access specific instance of your data, memorywise you might take a hit but the mapping is just onetime.
var countries = new List<Country>();
var countriesByCode = countries.ToDictionary(c => c.Country_Code, c => c);
Country cntry;
if ...
16
A starts-with should be easy enough to store in a pre-sorted list, ideally using a case-insensitive sort comparer rather than applying conversions each sort. Then: use binary search to find the first match, and keep moving forwards until it no longer matches. Should be pretty efficient.
If this needs to be edited in a shared multi-threaded context, be sure ...
16
Marc has the best answer but a trivial step in the right direciton would be to drop all the expensive ToUpper calls. Just replace .Where(s => s.ToUpper().StartsWith(term.ToUpper())) with .Where(s => s.StartsWith(term, StringComparison.InvariantCultureIgnoreCase)
16
Using ToString() to compare expressions for equality might work in simple cases, but:
It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => ...
16
Why not use Regex.Match instead of using those confusing joins/splits? Unless you are purposely trying to code golf this, doing it using regular expression is much easier to follow logically.
After taking into account delegates used with Regex.Replace (as explained in svick's comment), here is a functionally superior solution to the previous answer I posted....
16
I would say yes, there is value in removing the foreach loop, because your code will more appropriately mirror what you're expecting to happen versus what can happen. It's unclear to someone looking at this code for the first time that you're really doing a lookup based on one value as opposed to a join.
I would go a step further than storing the reference ...
15
Your code is nice, relatively simple and a good attemp at LINQ - I've written very similar code before. However I have the following critiques:
A list is created unnecessarily, eagerly loading the data. LINQ loves being deferred and streamed when possible - what happens if we never execute the code? What happens if we decide that we only need the first ...
15
Methods Set and Get Properties through their Get and Set methods, so it isn't a really good idea to perform operations on properties inside their get and set methods.
you should use the object methods to do this.
public string DisplayErrors
{
get
{
var errorString = "";
if (HasErrors)
{
errorString += Issues....
15
I think that most things are said, I recapitulate and add some of my own stuff:
As Jesse said, I'd use Any instead of Where + Count. It should have a better performance, I think.
Properties are for me: HasNoIssues, HasWarnings and HasErrors
In my Opinion the rest should be methods
Issues.Where(x => x.Name == "Warnings").First() is the same as Issues....
15
Q&A
Is the code readable?
ExactlyOne states very clearly what the method is supposed to do.
source is null seems odd to me (does that even compile?). I'd prefer source == null. (Edit from comments: a topic about is null vs == null)
IEnumerator<TSource> enumerator = source.GetEnumerator() can be written as var enumerator = source.GetEnumerator()....
Only top voted, non community-wiki answers of a minimum length are eligible
