New answers tagged

1

I have no knowledge about Unity3d, so here are some general comments. If world.SectorSize is constant throughout the session, you can change some of the properties to readonly fields and calculate them in the constructor: public readonly int SectorX; public readonly int SectorY; public readonly FlowFieldSector Sector; ... public Coordinates(int worldX, int ...


-2

I implemented a thread safe pseudo LRU designed for concurrent workloads. Performance is very close to ConcurrentDictionary, ~10x faster than MemoryCache and hit rate is better than a conventional LRU algorithm. I compared throughput to a classic linked list implementation with a lock to protect the list, and ConcurrentLru performs at least 5x better. Full ...


1

Errors aren't answers This: try { return new DriveInfo(path).DriveType == DriveType.Network; } catch (Exception) { return false; } will harm and not help you. What if a caller passes the integer 3 for path? The network drive status is certainly not True, but you can't say that it's False, either. It's an error, and ...


1

The lambda expression you pass to Where() is where you define the condition, so this is where it makes the most sense to put all the logic for this. .Where(u => strict ? u.Nickname.Equals(login) : u.Nickname.Contains(login)) Or for higher performance you can conditionally pass a different lambda expression: Func<User,bool> checker = strict ? (...


1

The first things that strike me: Where is the documentation? How do I know what the code is intended to do (which may be different from what it does)? Why are there no comments at all in/near the code? As such, the code is not maintainable, neither by you in 6 months or some poor maintenance programmer who gets to fix/update the program. (Always program ...


0

// Both s_weights and s_values will have the same length. private static int[] s_weights; // array holding the weights of the items. private static int[] s_values; // array holding the values of the items. private static Dictionary<(int, int), long> s_memo; // memoization dictionary. // NOTE: I cannot use an array instead of a dictionary here, cause ...


5

Other minor points: Imports Sort your usings, and move them to the inside of your namespace. Also consider using StyleCop, which would suggest this. Underscored privates Why do these private readonly ApplicationDbContext _db; private readonly SqlConnectionConfiguration _configuration; have underscores? Typically that's a Python convention. If you're ...


6

In GetAll, the feedback you've already had seems pretty valid, to be honest; there is really no value in re-throwing an exception you've caught directly; if this is after some kind of processing, then throw; (without the exception instance) is a good way to re-throw the original exception without damaging it, but in your case the entire thing is redundant, ...


1

Instead of storing the actual values in a tuple as the key in a dictionary for memoisation, multiplex them together into a single value and use that as the key. You will need to pick multiplex value that is an order of magnitude higher than the largest "numberOfItemsToConsider" you can expect. Or you could turn them into strings and concat for the ...


1

I may be wrong, but I think you're trying to solve an XY-problem. If I understand the term "DescriptiveStatisticalSet", you want to provide a data set class, that exposes some significant statistical properties and therefore the generic type parameter of the set must be a numeric type of some kind (int, double, ??). To fix that, you try to develop ...


1

The idiomatic TryParse signature is public bool TryParse(string input, out T result). It should never throw; that's what regular Parse is for. Return true if it succeeds and false otherwise, with result being set to the parsed value or default respectively. If you really do want to distinguish between the cases "there exists a converter but the string ...


4

Is this a good enough answer? This isn't meaningfully answerable without understanding both your skill level and the expected skill level you're interviewing for, and any possible modifiers/priorities you've been given to solve this exercise. Because I know people can solve it using Linq with less lines of code Line count is not the main measure of ...


1

As has been said in the comments, it's unclear why you feel you need this behaviour to restrict the types used. There's an interesting discussion around constraining types here, which includes a link to example code. So, there's a couple of questions. Why do you need to restrict it to double / integer types? Is it really that you want to do is restrict it ...


8

Algorithm Correctness Due to a small problem in GetBranchPrecedents this algorithm does not actually work. The case that all parents might already be present in the complete order. E.g. take the graph with the edges (C,A), (D,A), (E,A), (D,B), (E,B). In this case, A has the most prerequisites and will be treated first. This puts all nodes but B into the ...


20

How did you determine the output ' # ' for the case n==1? I'd have expected it to just be '*'. If you're unclear about the specification for code you're writing in an interview, it's best to either ask for clarification or document your assumptions -- it's possible they deliberately handed you an unclear spec and are testing how you handle that. If we take ...


3

Consider not using System.Drawing to manipulate images in ASP.NET: Classes within the System.Drawing namespace are not supported for use within a Windows or ASP.NET service. Attempting to use these classes from within one of these application types may produce unexpected problems, such as diminished service performance and run-time exceptions.1 An ...


7

Welcome to CodeReview! I hope you get some good feedback. Absolute paths string ID = Request.QueryString["id"]; string imageFile = "\\\\server\\c$\\inetpub\\wwwroot\\web-app\\images\\" + ID+ ".jpg"; This has a few issues. First, it's typical for a server to be in a "jail" where it shouldn't have access to the ...


2

Namespaces Have a read through https://stackoverflow.com/questions/125319/should-using-directives-be-inside-or-outside-the-namespace . I agree with StyleCop's default recommendation of moving using within the namespace: namespace RockPaperScissorsLizardSpock { using System; using System.Collections.Generic; using System.Linq; using System....


2

Rewrite the code so all references are "Player", not "Human and/or Computer" Your code will shrink by more than half. Do not use class name in method names In ComputerPlayer ComputerRandomMove would be RandomMove SetPlayerName : SetName (Name may be ok too ) PlayerMove : Move Do not name classes as their inheritance chain HumanPlayer : ...


0

I agree with @GeorgeBarwood answer, full stop. Next note @ Peilonrayz comment ...we don't have enough context If the contex or intent is to emphasize "Increment until reaching the max value", or better: "make this many copies", then use a while loop. int total = 0; while (total <= maxValue) { total += nCopies; }


3

I would say this is quite questionable. Imagine you never saw this code before, and are trying to understand it, would this make it easier to understand or harder? I say harder.


2

There is not much to add to Slicers answer other than underscore in names is rather un-C#-ish, and you should avoid single character names (s) as well. You ask for more efficient ways for the algorithm. One could be the following: public (char value, int count) MaxByFrequency(string input) { var chars = input.ToCharArray(); Array.Sort(chars); char ...


6

As per this answer, it would be wiser to use a tuple in this case. I use lowercase "t"uple as I highly recommend the built-in language support for tuples (char, int) over the explicit Tuple<T1, T2> declaration, etc. Few more points: What should happen in a tie? Which gets returned? If any is acceptable, it's fine as-is. To be more of a ...


3

public int? Value { get; } I really can't think of any situation where I would vant to store a null value in a search tree. But maybe I'm too narrow minded? What should the point be? It is by the way impossible to set the value to null, because it is readonly and the only constructor takes not an int? but an int. So how would one could set a null value? And ...


4

You can tremendously reduce the memory requirements of this code by acknowledging that you can replace the lists you use by single values: private static bool IsBinarySearchTree(Node root, int max, int min) this allows you to successively tighten the bounds on subtrees without storing the values you traversed in a list: public static bool IsBinarySearchTree(...


0

I think there is no need to create a new list for every recursive step. The main logic is whenever the current node is left side of its parent then: its (current node) left child value <= current node value its (current node) right child value >= current node value and less than the value of the current node's parent And the logic is exactly reverse ...


0

After many tries, I have found the solution. And it is working perfectly for me. maybe this solution will help others public static void SanitizeHtmlContent(string htmlContent) { var sanitizer = new HtmlSanitizer(); sanitizer.AllowedAttributes.Remove("src"); sanitizer.RemovingAttribute += (s, e) => ...


0

Extending from my comment on the question, I'd probably go ahead and implement the less than and greater than operators and a handful of interfaces like the underlying double has. I'm also a big proponent of the struct itself being immutable. The result looks something like (I only did Meter for this exercise): [Serializable] public struct Meter : ...


2

The code you have there may accomplish the task you have in mind for it, but it's not useful for a "real world" usecase. That's because usually you'd want to be able to run an algorithm like this multiple times on the same graph, maybe even concurrently. The problem that's posing itself here is that Node mixes two concerns: Representing the graph ...


1

From the separation of concerns perspective it seems fine that you do not want to validate against emptiness. On the other hand, one can argue that why do you treat an empty string as a valid name. My suggestion is to create a ctor with a parameter, where the consumer of the attribute can define how should it behave in case of empty input. public class ...


1

I think other answer had some great points, also I would use fluent validation instead so your model does not need to have dependencies on validation attributes. https://docs.fluentvalidation.net/


1

The only thing that stands out to me is these blocks: var cartVM = new CartVM(); cartVM.MovieTitle = item.MovieTitle; cartVM.HallNo = item.HallNo; cartVM.MovieDateTime = item.MovieDateTime; cartVM.Seats = item.Seats; cartVM.SeatId = item.SeatId; var movieVM = new MovieVM(); movieVM.MovieTitle = ...


4

You could easily turn your method into a one-liner expression-bodied method with a few tricks: use the _ discard variable instead of the explicit parsedDate variable (also, it doesn't need initializing to new DateTime()). use the conditional operator since you're returning one of two ValidationResults. value.ToString() can be passed directly into ...


2

I find using XDocument to be a lot simpler: var fileName = @"C:\Users\name\train\dev\offer\TestParsing.xml"; var document = XDocument.Load(fileName); var offerData = document.Descendants("offer").Descendants("data"); foreach (var d in offerData) { var rule = (string)d.Element("rule"); if(rule.Length < 2) ...


4

If you would define a model like this: public class Data { public string Id { get; set; } public string OCR { get; set; } public string Rule {get; set; } public string Output {get; set; } } then you could easily separate your ETL job's different stages. For example the Extract phase would look like this: Document doc = XDocument.Parse(xml); var ...


2

If you do not need report progress then the LoadIntoBuffer source code + CopyToAsync source code combo could be handy. All you need to prevent the concurrent call of the LoadIntoBuffer method. (You can call it n times it does not matter, because the IsBuffered flag will short-cut the method.) This can be done by using a Monitor or a Semaphore for mutual ...


4

Symbolic Constants It is good that you named these numeric constants, but they are constants so rather than declare them as variables, declare them as constants. static void Main(string[] args) { const int n = 5; //nodes const int m = 100; //size of square const int mm = 999; //maximum cost ...


1

The data structure List<T> is great most of the time. What it is not great at, is this: public bool Contains(T item) { return m_ListContainer.Contains(item); } Which unfortunately is an important operation for a set. There are different ways to set up a set that maintains insertion order that have different trade offs. For example: A hash set (...


3

Wrap ISet<T> in a namespace to avoid conflicts I presume you're already doing this, but there already exists an incompatible ISet<T>; you should wrap your interface with a namespace to avoid a collision. Replace your internal List<T> with a HashSet<T> You noted in the comments that elements should be unique; a trivial way to do this ...


1

One main remark: the paging method is too busy. The number of arguments is a tell-tale. The parts that don't belong there are filtering and sorting. In general, a method returning any paged result, should only receive an IQueryable and the paging instructions page number and page size. Your second method is almost there. After removing the superfluous ...


1

Some quick remarks: There are almost 90 white-space characters before I can see the arguments to GetPaginatedSearchAsync. In such cases, move the parameters to the next line. Consider renaming PaginatedDataResult and ProductSearchResult (is the "Result" part of the name necessary?), and even GetPaginatedSearchAsync (is there a non-async method? ...


7

I don't think exposing the Node class is useful. It should probably be mostly an internal class within your class (or at least package/namespace) Node could be using generics, so Node<T> instead of always having an int At least some of your AddNode-methods could simply be called Add instead. Because they're actually adding a number, a list of numbers, ...


1

In your implementation you have smashed together two separate roles: Product and Creator. Your implementation is equivalent with the following: public class GoldCreditCard : CreditCard { private GoldCreditCard(decimal creditLimit, decimal annualCharge) { CardType = CardType.Gold; CreditLimit = creditLimit; AnnualCharge = ...


2

As others pointed out you are iterating over the source a lot. Once ToList and every loop in the step. Your grouping method is tied to repository and GetAll method. It would be better to pass that into the method instead of having how to access data as part of the grouping. I would also recommend removing the mapping from this method as well and use the ...


1

Couple of pointers. You are mixing data notation and fluent mapping. replace NotMapped attribute with .Ignore fluent mapping. Also I think its cleaner to move each entity into its own mapping config class using IEntityTypeConfiguration<T> Dont mix DTO and entities like that. Best practice is to project your entities to DTOs.


-1

I was think a lot that should I write this answer or not. I don't want to be rude or offensive, but this piece of code is terribly. It has tremendous problems. Let me just highlight a couple of them: Initialization of the JsonSerializationOptions: You have a static variable, which you initialize in your instance level constructor always with the same values....


1

The conclusion misses the point Your code technically does touch on what makes a list readonly, but the example you've used to display that behavior suggests a completely different problematic scenario, i.e. that of mutable objects. This by itself has nothing to do with lists, regardless of whether they're readonly or not. So your example is not good. Not ...


3

Pick your framework You're learning new stuff, so your style will develop as you progress. While the code is still fresh, try to refactor as you go, so that the code moves towards a consistent style. If you want to move towards FluentAssertions, then try to use it for all of your assertions. Rather than this: // Fluent Assertions accountExpected.Should()....


3

I'm not sure if it's in the spirit of this site to suggest an entirely new approach, but here is how I would go about this: Select all of the process-group elements: var processGroups = doc.SelectNodes("/*/process-group"); Create a dictionary of them, indexed by name (this assumes the names are all unique): var processGroupDictionary = ...


2

ctor Neither key.Length nor iv.Length can be smaller than 0 you only need to check if it is equal to 0. But for me as a user of that class it would look strange to receive an ArgumentNullException if I pass e.g a key with Length == 0. I would expect an ArgumentOutOfRangeException. Omitting braces althougth they might be optional can lead to hidden and ...


Top 50 recent answers are included