20
I won't repeat the germane critiques of the other answers, which are good. Rather than concentrating on the line-by-line improvements, I have some suggestions for the general organization of the code.
First off, your general technique here is sound. But I want to describe two techniques you can use to make the code easier to read and understand, and to make ...
8
Some quick remarks:
Please follow the Capitalization Conventions.
Use meaningful names.
strList is bad in various ways: not only does it contain the name of its type (List), it also contains a pointless abbreviation. Why not name this input parameter words, the term you actually use in your Main()?
Same for str or sortedStr.
anagramsTable does not tell ...
6
var sortedStr = string.Concat(str.OrderBy(c => c));
string.Concat(sortedStr.OrderBy(c => c));
I think the second line is redundant as you don't bind its result to anything.
var anagrams = new List<List<string>>();
This is also redundant, because you actually have the lists of anagram items in the dictionary. So instead of moving ...
3
Interesting code! Am about to write something similar for a quite extreme case (decoration elements for procedurally generated terrain) so I could share a few thoughts I had:
That mechanism for prePooledObjects feels a bit too entwined with the pooling system itself. Why not provide a single function like:
PrePoolObjects(GameObject go, int count);
That ...
3
Just looking at this one section:
if (key.ToUpper() == "EXIT" || key == null)
Environment.Exit(1);
else if (key[0] > 71 || key[0] < 65)
{
Console.WriteLine("Invalid key, try again.");
goto Start;
}
Why Environment.Exit(1) and not simply exit(1)?
Given that the program has already exited for the true condition, there is no point in having ...
2
Its good you recognize that goto is a bad pattern, I would go so far I would reject any code that I reviewed that had it. They can be converted to a loop statement pretty easy.
For Example
var key = string.Empty;
//Get key
do
{
Console.Write("Enter key (type 'Exit' to exit): ");
key = Console.ReadLine();
if (key != "EXIT" || key[0] > 'G' ||...
2
What is a service?
There are mupltiple definitions. Probably the most generic one is that it is any object that provides some useful functionality. Sometimes it may be a synonym for a singleton.
What is a factory?
Factory is basically a service that instantiates other objects. A class constructor can be considered a factory in a way that is not polymorhic ...
2
The main validation logic doesn't need "min node" and "max node",
only the values.
Passing the nodes as parameters adds complexity,
because you have to handle null values,
which would not be the case with simple integers.
Being an unnecessary complexity (since nodes are not really needed),
I suggest to eliminate it, for example:
public static bool IsValid(...
2
For the most part I agree with your code. However there are a couple of things:
IsValidBST I think is redundant. Anyone running this method already knows it applies to a BST. I think just IsValid would be better.
Helper should have a better name, I think an IsValid overload would work and it should be private.
It doesn't make much sense to me to make ...
2
My take on this is that the function signature should not lie.
So if it can handle a null passed in, as it can, it should use
Bar? bar = null
Another approach could be to overload the function with another which only takes a single parameter. Something like ...
Task FooAsync(
Bar bar,
CancellationToken ct = default)
{
...
}
Task FooAsync(
...
2
Style
As far as style goes, your code looks clean and readable, and follows the conventions, so good job for that.
You may want to include documentation comments with \\\ four your class and methods. Although the method names are descriptive enough in this rather simple case, it is a good habit to take on.
Tests
It's nice you use a test framework to test ...
1
Some quick remarks WRT style:
Public properties should have at least a { get; } (IMHO; I'm not a fan of public readonly and instead prefer { get; private set; }) and should be PascalCase.
Don't just throw any Exception, throw the correct one, e.g. ArgumentException.
Underscores should only be used to prefix class-wide private properties; certainly not ...
1
Avoid unnecessary variables
The counter variable is unnecessary.
For each level, you need just a List<int> level,
add the elements of the level to this list,
and then add this list to res.
By eliminating this variable, you also eliminate any concerns about the correctness of res[counter].Add(temp.val);.
With the code changed to level.Add(temp.val),
...
1
You might need to create a class handler that would handle the PdfDocument and MemoryStream, which will make things easier to handle and disposed as well. Then, you can just adjust the handler to your application requirements. here is an example :
public class PdfDocumentManager
{
// Use Dictionary to keep both PdfDocument and MemoryStream paired.
/...
1
I don't like classes named …class. In languages including C#, it leads to expressions like new IsValidBSTclass(), not yielding a class.
I'd prefer BSTChecker over BSTValidator for the foreseeable bstValidator.isValid().
1
I would rather use the LINQ Query Syntax for simplicity and readability
var input = new List<string> { "Tom, 1, 3, -1", "Gillie, 2, 3, 1", "Thor, 1000, 1001, 1002" };
var personsWithLoss =
from pl in input
let splitted = pl.Split(",")
let person = splitted[0]
let maxLoss = splitted.Skip(1).OrderByDescending(x=>x).First()
select ...
1
A few things can be noted.
Find the largest value by using Max() which is a LINQ extension.
Don't initialize integers variables with MinValue or MaxValue such as decimal.MinValue. Use a default value of 0 or -1 (which are the common used ones` for better code management).
IsInValidInput is not presented in your post, but I assume it's a method where you ...
1
I wrote this code with a simpler version.
At first I created a class to store parsed information.
public class PersonWithMinLoss
{
public string Name { get; set; }
public decimal MaxLoss { get; set; }
private readonly List<decimal> Amounts;
public PersonWithMinLoss(string currentPersonData)
{
var parts = currentPersonData....
1
A few things I noticed:
Using the range option of the array index(var allAmounts = currentPersonData[1..];) doesn't really help here. All that's needed is to use the appropriate indexes of the original array.
It's quite inefficient to use a try block to test if a string is a valid number. Using the tryParse method is much more efficient.
A hard coded ...
1
Don't have methods like OpenConnection() and CloseConnection(). Instead, use a using block:
using(var conn = new MySqlConnection(connString))
{
// db stuff
}
Same for MySqlDataAdapter and DataSet: both implement IDisposable and thus should be handled properly. Instead of writing lots of code to do this yourself, use the built-in using.
using(var conn = ...
1
Other answers have pointed out the code issues, but you also asked about potential improvements in efficiency. Currently this program runs comparisons in O(n log n) time (per comparison), mainly due to the sorting algorithm. (Any default library sorting algorithm is going to have average case O(n log n) time.) Can we do better? Sure. This problem can be ...
1
(The question caught my attention way down into the weekend (many offices starting the working week this minute): observations about the approach for starters.)
As commented by Pieter Witvoet, determining (all) similar strings does not require full evaluation of the Levenshtein distance
difference in length sets a lower bound on Levenshtein distance
...
1
I would suggest to implement a HeatMapGridPoints class and inside this class, create two separate methods for GetUnexploredPoints and GetOpenPoints which will return list of GridPoint. This way, you can expand and also reuse the HeatMap. Also, it would be more readable than just gathering them into one method.
Example :
public class HeatMapGridPoints
{
...
Only top voted, non community-wiki answers of a minimum length are eligible
