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: ...


21

The problem, however, is that this means when creating my classes, I need to ferry around this TPos generic construct to places where it doesn't make sense to define it. I'm left creating new Player<Vector3> or Player<Int2> instances all over the place when really I just want to be creating a Player. Yes, specifying Player<Vector3> each ...


21

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? ......


14

It may appear to some as code smell, but it's a practice in use in the .net framework defined as 'marker interfaces'. IReadOnlySessionState is one of these and as per the documentation: Specifies that the target HTTP handler requires only read access to session-state values. This is a marker interface and has no methods. So, to answer your question, ...


14

Your interfaces are inconvenient, and you are falling in to a trap of convenience that actually leads to inconvenience..... let me explain... C# makes it easy to create the getter/setter methods for interfaces/classes. There are three places I can see where your use (abuse) of this convenience leads to poor usability of your interfaces: public interface ...


14

Two parts to this review: the 1-liner for the newInstance() the general mechanism of the method newInstance() This code will work, but only for a subset of Collections. For example, there are many Collection implementations that do not have an accessible default constructor. What if the input collection is: Collection<String> input = Arrays.asList(...


13

Your example accesses the cat through a variable of type Cat and the dog through Dog. This would work, even if both classes did not implement a common interface. Inheritance is not involved. In order to really demonstrate the usefulness of polymorphism I suggest the following code: interface IAnimal { string Name { get; set; } string Cry(); } ...


13

You said that you packed all the suggestions from the previous question, but there are still some pieces of advice that you did not integrate into your code. Here is how you can still improve your it: math.h legacy In your code, you are using the constant M_PI. While it will probably work on most of the compilers I know, this macro isn't defined anywhere ...


12

Disclaimer I will mostly not critique what your class is doing, but mainly suggest how to make your interface as natural as possible (do as the ints do) by operator overloading best practices in terms of return type and const-correctness. I will mostly stick to C++98/03 and only sparingly suggest C++11 features. You could read the revamped GotW series for ...


12

Having worked on a similar system, I came up with a very similar design. So either we're both doing something right, or we're both doing it wrong :) Here are some things I would change, but obviously different requirements call for different decisions and may not all be applicable here. Remove CodeInspection class I don't think you gain much from having a ...


12

This code is, depending on your point of view, either over-engineered or just wrong. The whole point of separating the interface from the implementation would be to make the interface independent of the implementation. Yet, your interface mentions EngineerImpl.View, which defeats the whole purpose of that separation. The fix, if you consider the code to ...


11

An empty interface is a code smell. In C# you can use attributes to mark a class instead of making it implement an empty interface. As I suggested in my comment, you can use reflection to build your Map instead of receiving it as input, and failing on discrepancy. With the power of attributes, you can add functionality by marking which properties you want ...


10

Extracting an interface essentially boils down to taking the signatures for all public members, making them part of the interface you're extracting, and then making the type implement the interface you've extracted. Tools like ReSharper work amazingly well for this (the partial class is handled, no sweat - in fact, the interface only needs to be specified ...


10

This answer will reflect the naming convention at my work. Naming Convention Prefixing I to all your interface seems superfluous, since we use a simple name like ContractService. The implementation class would have Impl suffixed to the name of the service like : ContractServiceImpl. Normally you will use DI to inject the implementation where you need it, ...


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

Very nice implementation; I always like seeing your code here. I really only have five very minor opinions on this implementation: I'm not sold on expr as an abbreviation for expression. I'd recommend expression in its various incarnations. 1a. For that matter, t in the Create method should probably be called composedExpressions. The class constants ...


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


8

static final int WORK = 1; static final int LOOKING_FOR_NEW_JOB = 2; static final int BUSY = 3; static final int FREE = 4; static final int SLEEP = 5; A few things specific to this First, using int is a bad idea. Allow me to demonstrate engineer.sleep(LOOKING_FOR_NEW_JOB); I don't think engineer.getSpouse() is going to buy the notion that napping counts ...


7

OK, Hello again. So here are my comments: You should not have an array of boats in class Rental, but just a reference to a single boat. That boat should be given in the constructor, and a getter and a setter should be added: The fields should not be static Here's a new version of the Rental class: class Rental { private String name, position; ...


7

(Since you're still a student, I might go over the top with some explanations.) Code Style Always format your code. Always. (CTRL+SHIFT+F in eclipse and CTRL+ALT+L in intellij). There's also the possibility to activate the formatting for save actions. Use the correct indentions and use empty lines (but not three empty lines). Naming things The naming of ...


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

Overall Design first. Name and semantic of your class I have this code that converts audio to different file formats. Not exactly, your code is a client to an Encoder, that converts audio to different file formats. In design, it is very important to be certain about the reason and semantic of an object. So first, the name AudioEncoderDecoder is bad. It ...


6

Is this interface complete? Does it attempt to do too much or too little? In my opinion, interfaces should be as small as possible, providing only methods that are really required to be called without knowing (or casting to) the actual subclass. I don't know how you use it, but my first impression is: your interface contains too many methods... Are there ...


6

There is one performance issue that can be addressed quite easily, and that is when we're even bothering to respond to the notification. This if statement can be completely eliminated: if ([notification.name isEqualToString:State] && [self.stateDictionary count]!=0) The left half of the && can be eliminated by registering only for a ...


6

Things will be greatly simplified if ClassMapping didn't map a class to a class, but a class to an instance. public final class ModelConverterFactory { private enum ClassMapping { DEFAULT(Object.class, new AbstractConverter()), RIGHT(Right.class, new RightConverter()); private Class<?> modelClass; private ModelConverter&...


6

There is a philosophy issue with this problem. The whole point of the Collection interface is to act as a high level interface for Collections. It throws away the implementation specific detail by design. Trying to get it back will usually only introduce the exact problems that the Collection interface exists to solve. Therefore you cannot reliably complete ...


6

You could consider using an generic abstract base class. [ComVisible(false)] public abstract class HostApplicationBase<TApplication> : IHostApplication { protected readonly TApplication _application; protected HostApplicationBase(string applicationName) { _application = (TApplication)Marshal.GetActiveObject(applicationName + "....


6

I really don't like this enum and "type" member. public enum DataMessageType { Login, } public interface IDataMessage { ILogger Logger { get; } DataMessageType Type { get; } void WriteNetworkMessage(NetOutgoingMessage message); void ReadNetworkMessage(NetIncomingMessage message); } The idea behind polymorphism is that so long as the ...


6

am I doing it right or wrong? You are mostly right. This is a very common pattern but your naming is a little bit different to standard implementations. I should note that in this particular case, this is completely overkill but I'm assuming that this is a learning exercise for you and not something you are planning on using. public interface ...


6

Since I am using DI to instantiate classes I have to disappoint you... there is no DI in your code. The only service you are using is instantiated inside the class using it: private ContactService _contactService = new ContactService(); DI would be if you passed it to ContactBusinessImpl via constructor. public string Token { get; set; } public void ...


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