27

An argument against table-style alignment in code Except when the editor/IDE helps maintain alignment with little work from the programmer, and all those who work with the code have that same facility, horizontal, table-like alignment in code is more trouble than it's worth, for these reasons: Search/replace operations on the entire code-base (as when ...


24

None of your QUEUE_* functions validate their input arguments before using them. NULL pointers will be a problem, particularly for the pointer-to-pointer arguments. C's memory-allocation functions return a void*, which implicitly converts to any other pointer type. This means that you don't need to typecast the result of calloc. Doing so can actually ...


24

The reason why there is no async API for a dictionary is, that all operations on a dictionary are so fast, that there is no need for asynchronicity. For concurrent scenarios there is the thread safe variant - the ConcurrentDictionary. Adding an async API to these dictionaries has absolutely zero value. Rather it increases complexity and reduces performance....


23

This is some very nice-looking code, and it looks like it is working. My criticism falls into the following topics: Discussions about style, naming, tools used etc. I assume you have consciously settled on a certain style, but some aspects strike me as so unusual that I would like to talk about them. I too, like to overengineer. But there are some parts of ...


22

The fact that this was an interview question changes how I look at the code. The interviewer was almost certainly looking for one answer: "union-find data structure" or "disjoint sets data structure". You were being judged by whether you said those magic words within the first few seconds, could come up with something similar on your own, could come up ...


22

IOutput class module (Interface) Looking at how the interface is being used: this.Parent.OutStream.PrintLine output Where output is clearly a String, which makes sense. But the interface's signature doesn't reflect that, and is confusing: Public Sub PrintLine(Optional ByVal object As Variant) Why is the parameter optional? and why is it a Variant? ......


21

The Regex expression [^0-9.] matches anything other than 0 to 9 and .. If you remvoe the . from the expression, It should only match everything you want. But, this can be furthermore simplify to \D which matches anything but numbers. So the whole thing could be written in one line Regex.Replace(value, @"\D", "") I don't think it is worth to create a ...


21

I suppose you don't indent the printf debug lines to make them easier to spot. That's a sensible idea (although not one I've seen before). However, I treat Code Review answers as a "just before checkin" thing, so I'd suggest removing these by this point. You pre-declare your variables. Don't do this; write them as close to point-of-use as reasonable. Your ...


20

if (moduleName == null) { moduleName = componentType == vbext_ComponentType.vbext_ct_StdModule ? "Module1" : componentType == vbext_ComponentType.vbext_ct_ClassModule ? "Class1" : componentType == vbext_ComponentType.vbext_ct_MSForm ? "Form1" : "Document1"; } Sorry, that I have ...


18

This is very nicely written, pleasure to read. Most of my comments will concern usability and readability. The NONE operator typedef enum { AND = '&', OR = '|', XOR = '^', NONE = 0 } operator; NONE kinda sticks out there: it's not an operator. It made me wonder how it's used. It turns out, if the operator is NONE, then the calculator ...


18

It's easier to do it with LINQ which is in this case virtually a one-liner. var text = "DEFD"; var result = text .AsQueryable() // this isn't necessary as noted by @CodesInChaos .GroupBy(c => c.ToString(), c => c, StringComparer.OrdinalIgnoreCase) .Where(g => g.Count() == 1) .Select(g => g.Key) .First(); // this should ...


17

Is there a common Java library that serves a similar purpose, or am I reinventing-the-wheel? As far as I know the JRE doesn't offer this out of the box. Are the semantics for null keys and zero counts reasonable? Would it be typical to be counting nulls? I guess it's nice that it's possible, even though it opens the possibility for exceptions. ...


17

The idea of abstracting DateTime.Now and DateTime.UtcNow for testability is definitely a good one. There are some improvements I'd suggest: Naming FooProvider is a name I usually pick when I absolutely can't think of a better name for something that provides Foo. In this case, a much more natural name than DateTimeProvider would be Clock. I also don't ...


17

Welcome to code review and thanks for sharing your code. When writing UnitTests keep in mind that they are documentation. Test code has less restrictions for identifier names. This means that method names are usually longer and you should not "shorten" your unit test methods. The test name should describe the expected behavior in detail @Test public void ...


16

Almost all of this code can (should?) be moved directly into the MockFactory. [TestInitialize] public void Initialize() { _ide = MockFactory.CreateVbeMock(); _ide.SetupProperty(m => m.ActiveCodePane); _ide.SetupProperty(m => m.ActiveVBProject); _ide.SetupGet(m => m....


15

First : the comments about aesthetic. You can use List/set/dict comprehension more than you do : self.values = {1: True,2:True,3:True,4:True,5:True,6:True,7:True,8:True,9:True} becomes : self.values = {n:True for n in range(10)} or self.values = dict.fromkeys(range(10), True) out = [] for k,d in self.values.items(): if d==True: ...


15

A minor remark: your test is named testCreateNotExistingEmail. The @Test annotation already indicates that this is a test so it doesn't have to be repeated in the methodname itself. Adding to that: I like to structure my tests in the format [UnitOfWorkName]_[ScenarioUnderTest]_[ExpectedBehaviour]. It is hard to tell what exactly your test does when I look ...


15

You implementation makes assumptions about how Builder.add() is called. If they are not explicitly documented, they are errors in the code. Divisors not added in increasing order: @Test public void BuilderAdd_DifferentDivosorOrder_SameOutput() { FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 5).build(); FizzBuzz buzzFizz = ...


15

There is already a good answer that describes how you could improve your tests, however, I would like to offer another solution. I had a question that was similar to this on an interview (well, I solved it using a similar method to the one I gave here). When the interviewer asked this question, I think they wanted you to find a solution that is not only \$\...


15

Here are a few of the high- and low-level ideas: too many comments. Remember, comments tend to age and outdate, they need to be maintained as the code changes. And, if they are over-used, they hurt readability, not improve it. See more at Coding Without Comments convert some of the comments preceding the functions and methods to proper documentation strings ...


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()....


14

You are doing it wrong. Why are you using WCF if you are only using it as an old timer ASMX service without using the extensibility points? First things first - clean up [ServiceContract] public interface IWorkingService { [OperationContract] Collection<Ergebnis> UpdateEntity(int entityId); } The service interface is the same as yours, but: ...


14

I scanned through your code and also stepped through it with a debugger, line-by-line. The more I looked, the more things I found that I wanted to point out. Let's start by looking at some unnecessary comments: // BASE PRICE private static String BASE_PRICE = "$1299.99"; // Number of People private static String NUM_OF_PEOPLE = "3 people"; (Why aren't ...


14

When you're in an interview, few interviewers expect you to come up with the perfect solution. Mainly, they're looking for an idea of how you think and how you approach a problem. Especially as a junior programmer. Approaching the problem from a test driven perspective first might seem the logical way, but what about just writing the code to solve the ...


14

The single sign of over thinking was your use of a business primitive (PhoneNumberManager). The problem called for you to start with a string and end with a string so no need for an extra type in the middle. It looks like it also caused you to miss the obvious string extension approach. Getting the Regex wrong is normally ok in interviews since almost ...


14

Your main is a classic arrow anti-pattern. You can use guard clauses to make the code flat and easier to understand. Bare excepts are normally not a good idea. Why would you want to print the usage if the code works fine but the user passes a keyboard interrupt? You may want to use except Exception:. I would prefer if mask and unmask were wrapped up in a ...


13

There are a few things I see in here that I would suggest could be different. I don't like that the logic for determining whether a cell is a sink is on both Rainfall and Cell. In fact, both classes have the method called is_sink... My preference would be to move the logic on to the Cell, which already knows how to calculate it's neighbours.... and then ...


13

Naming Interfaces are usually named starting with a capital I in .NET. ScientificFormatter Here you have public string Format(BigInteger number) and private string FormatNumberString(string numberString) where the second would be easier to read if you would use method overloading. Method overloading is just that you can have a method with the same name ...


13

JUnit 4 has a Parameterized test runner that does most of the work for you. For your tests, it would look like: import maharishi.DigitIncreasing; import static org.junit.Assert.assertEquals; import java.util.Arrays; import java.util.Collection; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org....


13

So first, as CAD97 rightly points out, if you simply define your protocol as identically matching the methods from UserDefaults you intend to use, then making UserDefaults conform to the protocol is much simpler: extension UserDefaults: UserDefaultsProtocol {} And there ya go, done. Having to write less code is nice. What's even nicer here is that we're ...


Only top voted, non community-wiki answers of a minimum length are eligible