23

I have a general rule: if I see string concatenation with a loop I assume it's not the best way of doing it. Let's look at this: foreach (Location exit in Exits) { description += $"— {exit.Name}"; if (exit != Exits[Exits.Length - 1]) { description += "\r\n"; } } I see this pattern a lot. What you need is Select with string.Join: ...


10

You've already made a decent effort to make your types immutable by setting its properties in a constructor and providing get-only access, not set - like you have with Location.Name for example. This is good practice whenever it is reasonable to do so (because, among other things, this means you can pass objects around without ever worrying that something ...


9

RobH's review covers syntax and style well so I won't go into that. Instead I'd like to give my take on feedback given by Svek and BittermanAndy. Separation Of Concerns I think Svek's commentary about the CreateObjects method is spot on, but I don't think it goes far enough. The need for such a method in the first place hints that the ExploreTheHouseForm ...


7

The other reviews covered many of the main points I'd raise, but there are a handful of others. Constructors in abstract classes should be protected, not public: public Location(string name) => Name = name; You can't create a new instance of the abstract class, so it is for all intents and purposes protected anyway. Make the access modifier match ...


6

In this code block here: ... OutsideWithDoor backYard; Outside garden; public ExploreTheHouseForm() { InitializeComponent(); CreateObjects(); // <--- bleh MoveToLocation(livingRoom); } This is call to the CreateObject() method, is something I don't like to see in code (it could be a personal style issue) but if you are constructing an ...


5

You may want to step back and make sure you know where you want to put the responsibility for compliance, and how you want your users to express their expectations. My initial impression (an outsider's to your problem set, of course) is that you're expecting a good deal from "novice" users -- multiple inheritance, classes with arbitrary fields, and set-like ...


4

public interface IFaker<T> where T : class { Faker<T> GetFaker(); } I don't think you need this interface and the additional call to GetFaker. Instead you can derive your class from the Faker<T> and use the constructor to set everything up: public class DogFaker : Faker<Dog> { public DogFaker() { RuleFor(dog =>...


3

public interface IInfiniteCounter : IEnumerable<(int Value, InfiniteCounterState State)> vs int Current { get; } (int Value, InfiniteCounterState State) Next(); void Reset(); The interface itself looks a lot more like IEnumerator<> than IEnumerable<>, so IMO implementing IEnumerable<> is misleading and dangerous. The ...


3

There no such thing like an interface class (term used in initial version of post). You have interfaces and classes implementing the interfaces. I don't see any List<Document> in your code, but If you have defined the list as List<IDocument>, you must stick to this definition. You cannot assign it a List<Document>. But you can add Document ...


3

This has been raised in many of your reviews, but let me repeat: make member functions that don't modify object contents const. This improves readability and advocates correctness. (One example is get_board(), but there are plenty more). Don't pass expensive objects (like std::string) by value unless you have to. A better idea is to pass them by const-ref (e....


3

Go doesn't guarantee atomicity ... so I built a simple Go interface to do that Okay, but is your implementation atomic? The only atomic operations guaranteed to be atomic in Go are through the sync/atomic package (or things like func (*Cond) Wait under sync). If you need true atomic write, use the atomic package. However, using log is usually sufficient. ...


2

Everything is an Entity! If you would follow the Entity Component System approach, that is. The idea is that all objects are entities, and each Entity has several components. Size, Position, Movable - all this are different aspects of what an Entity possibly could do and are therefore each it's own Component. As for Systems, that's where most of the logic ...


2

When you roll the die (not role), the result is one of: 0 1 2 3 4 5 6 7 8 9 rand() % 10 1 2 3 4 5 6 7 8 9 10 + 1 0 0 1 1 1 2 2 2 3 3 / 3 Rolling a 0 is not fun. Whenever I roll the die, something should happen. When I read the code from top to bottom, I wondered what the magic number 9 had to do with the player. Further down in the code I learned ...


1

Your first variant is unnecessary detailed and hard to read: an IPv4 address consists of exactly 4 digit groups 4 is a small number, maybe it's worth to inline the for loop the for loop treats the case k == 0 specially the += operator for strings allocates a new string each time the code is much longer than the description in the task "join the four numbers ...


1

First off: It's invalid HTML to have a div directly inside an ul element. Any special reason you need the <div class="tagsend"></div>? Modern CSS techniques usually don't need additional elements like that. You shouldn't repeatedly call the getter methods in a loop like that. If they do more work than just returning an array, then it's wasted ...


1

@Peter Taylor is right. It was some strange enumerable/enumerator hybrid so I have turned it into a real IEnumerator<T> of itself because I think is makes it easier to use in various scenarios. If I now want to have an IEnumerable I can use the new AsEnumerable extension and if I want to have a countdown then I can flip the value with new ...


1

Overview The trouble here is ownership. Who owns the tail? In C++ ownership is a very important topic and defines who is responsible for who should destroy an object. The problem with C like pointers is there are no explicit ownership semantics. So as part of the documentation you need to define who the owner (and people read documentation is the problem ...


1

You don't need the type parameter on IOrderable: public interface IOrderable { double Ordering(); } I don't like that you sort the source vector without notifying the client. In this way mysterious things are going on and the method does more than one thing. At least you should have a flag argument that lets the client determine if the source ...


1

You describe what you did but you did not sufficiently describe why you did it. What problem you are trying to solve by your design? I see some major problems in your design. Uno.h does not need inttypes.h. uint8_t is in stdint.h. namespace instead of class would be sufficient. Define these constants as plain int constexpr. Never ever define pins like A0 ...


1

The main problem with your code is that you've been encoding currency denomination data in C#'s type system: one type for each currency. That somehow led to a complicated interface/generics design, all without providing any real benefits. In fact, this design has several drawbacks: Each new currency requires additional code (3 extra classes in total). ...


1

A class is a blueprint or template or set of instructions to build a specific type of object. Every object is built from a class. An instance is a specific object built from a specific class. It is assigned to a reference variable that is used to access all of the instance's properties and methods. Ryan and nate should both be an instance. They have ...


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