13

Create a strategy to find the mapped implementation of the interface provided: IDictionary<Type, Type> mappings; //Populated with all interface to implementation types private DalServiceBaseClient CreateHttpClient<T>(string baseAddress) { var httpClient = new HttpClient { BaseAddress = new Uri(baseAddress) }; var ...


11

Two things strike me: There's no need to have a separate factory class when you could put the functionality into the enum, unless you expect to have other factory implementations As you say, if the classes are stateless, there's no need to create a new instance on each call. (I've only just spotted your factory map class, which effectively does something ...


11

Programming / detail review Fan(Fan *peer__ = NULL); (and some more occurrences) Identifiers that contain a double underscore are reserved for the compiler/libary ("C++ implementation"). The NULL macro can be thought of as deprecated. Use the nullptr keyword instead. It is more typesafe. static Fan * create(Fan *peer = NULL); Since those pointers ...


11

The number, letter, and strRep fields should be final and must not be static. By making them static, you've made all of your RegistrationNumber objects very weird, and quite the opposite of immutable. Constructing one RegistrationNumber changes every previously generated RegistrationNumber! getInstance() is a bit odd. Conventionally, the name getInstance(...


10

I personally would not rely on using magic strings. Magic strings are where you have taken something like a class/method/variable name and written it within a string, which is then used to identify the appropriate class/method/variable This makes it hard to refactor later on if you change a class name, it is too easy to miss instances etc. Furthermore, ...


10

In 30 min I could only create a draft. It's crazy what they requrie in an interview. It usually takes hours to come up with a resonable design. Anyways this design has one very bad blocker that prevents it from being extendable. It's the CarManufactures switch. You have a fixed number of those types and you cannot add new ones without modifying the switch ...


9

Let's look at the requirements, an immutable unique Registration number which consists of a letter and 4 digits, constucted by a factory method. There are 4 parts here: immuatable letter/number combination unique factory. Immutable This is easy to solve, but harder to describe. A Java class that is final, has only private and final fields, is Immutable. ...


8

To quote an ancient truth, "To every thing there is a season, and a time to every purpose." There is a time to fancy-pants-code, and there is a time to cowboy-code. Experience is what has to guide you in determining which method is better suited for a given task. There are many things that can impact that decision, for instance: How long will it take How ...


8

I can't recommend these string constants at all. We should be using an enum. Even if your enum is still string backed (which it doesn't appear that that'd be necessary), it's still going to be better overall to use an enum. As a start, the enum, means that users can't simply pass any random value in. The values they can select from are more clearly ...


8

Use Standards Use standard notations and representations whenever possible: see Algebraic Notation for example. Avoid Global Variables Board.CHESS_BOARD is a global variable. Global variables are bad. So instead of a global variable, pass the board as an argument to the method. It is very possible that a program may need to consider multiple chessboard ...


7

Start from the simplest possible way to solve the problem, and add complexity if there is a reason. Reasons could be a specific need from the specifications, or something that your experience tells you will very likely be needed at some point. In this specific case, there would be a point in isolating the implementation for a printer inside a class by ...


7

That is a Factory pattern, but it may not be the most efficient setup. If you had a finite set of controls, this wouldn't necessarily be bad, but here are some potential improvements. Generics They are your friend. Having the type enum is redundant and makes this class violate SRP (Single Responsibility Principle) because you will need to modify the ...


7

The simplest thing to do is probably to use FactoryGirl's sequence: FactoryGirl.define do factory :company do sequence(:name) { |n| "#{Faker::Company.name} #{n}" } end end sequence basically gives you an auto-incrementing integer, so you can avoid uniqueness issues. Sure, it'll generate some slightly odd company names, but for testing that shouldn'...


7

1) Which of the moves are actually needed here? Since there are only two moves, let's have a look at them: std::unique_ptr<Car> getCar(CarName sa) { if (d_carMap.find(sa) == d_carMap.end()) { d_carMap[sa] = std::move(createCar(sa)); // (1) } return std::move(d_carMap[sa]); // (2) } Only the latter is necessary. In (...


6

In general your code looks fine, though there are some points to improve: Memory management Instead of using raw pointers and new delete, you should use a smart pointer to express transfer of ownership explicitly: template<typename O> void register_type(const std::string & name) { types[name] = [](){ return std::unique_ptr<O>(new O); };...


6

A few thoughts: As an academic exercise, I generally like what you have done. Your code is clear, concise, understandable, largely documented (though doc blocks are inexplicably missing in some cases), and applies a reasonable design/inheritance model. I would encourage you in the future to always strive to model your objects, interfaces, etc. in terms ...


6

Not sure if that's going to improve the maintainability index, but You're misimplementing Inversion of Control. IoC is when your dependency (namely, IReportHandler) is given to you from outside the class. It isn't the case here. It defeats the whole purpose of having an abstract dependency instead of a concrete one. Read up on Inversion of Control and ...


6

Sure. You implemented the basic factory pattern. But factories don't get useful to you start using Abstract Factories. SO you can swap out out factory for another factory. Side Note: When presenting code it's easier to review of the header comes before the source. And best if the base class comes first. Prevents a lot of scrolling backwards and forwards. ...


6

I think your names could be simplified. For instance, the ShapeType enum could be: public enum ShapeType { Square, Circle, Triangle } Because ShapeType.SquareShape sounds redundant, whereas ShapeType.Square still clearly indicates that the square is a shape type. The ShowArea method is not needed. It would most likely be used to display console writes, ...


6

Thank you for releasing your code! You have some clever solutions and I would like to add few more points to the previous answers. Naming Piece The class Pawn confuses me, not because it is to complicated, but because of its name. Pawn is used to validate a movement and to do a move of a piece on the board, while the name suggests that it is the piece on ...


5

No, you can't use a factory to dispose your objects, your code is creating and immediately disposing the SPSite as soon as it steps out of your method. public override SPSite GetSite(string url = null) { return new SPSite(string.IsNullOrEmpty(url) ? Url : url)) } Have the disposing be handled by the calling code. You can't handle the disposing from ...


5

Rather than doing it that way (separate methods for separate configurations) you should pass in the "configuration" to a single method and let that one handle it. class AcmeFactory { public static function create($config) { $crud = new Crud; $productTypes = new ProductTypes($crud); $productImages = new ...


5

If you are on Java 8, you can use a cool new feature of ConcurrentMap: computeIfAbsent. It lets you defer the actual construction of the Connection until it becomes necessary and makes your code more concise: private static ConcurrentMap<ConnectionSettings, Connector> connectorCache = new ConcurrentHashMap<ConnectionSettings, Connector>(); ...


5

This line isn't needed in Python 3: # -*- coding: utf-8 -*- so you should probably just remove it. UTF-8 is assumed. Your docstring is indented - why? It's not technically wrong, but it seem like a really arbitrary choice. I'm not sure how to say this, but none of your sentences are capitalized. If this were a professional code-base, I would absolutely ...


5

i18n This is just me, but I've always found the _ and _N aspect of gettext a little hard to read and somewhat too tightly coupled. I've always preferred something like this: i18n.py import os.path import gettext def _translations(domain): locales_dir = os.path.abspath(os.path.dirname(__file__)) translation = gettext.translation(domain, ...


5

You could rewrite getShape as follows: public static Shape getShape(String shapeType) { switch (shapeType.trim().toUpperCase()) { case "CIRCLE": return new Circle(); case "RECTANGLE": return new Rectangle(); case "TRIANGLE": return new Triangle(); default: return null; ...


5

The list of features is a very flexible solution that follows the "composition over inheritance" rule and allows to add features that are not even known yet. However, the downside of that approach is, that different groups of features exist where items from one group are not compatible to other ones of the same group (e.g. it makes no sense to add FuelJet ...


5

Some observations: You shouldn't be creating the factory inside the class that uses it. That defeats its purpose! Instead, you should pass the factory to that class's constructor. You shouldn't be using a factoryId parameter to control creation of the factory. The thing using the factory shouldn't need to know what kind of factory to create - it makes it ...


5

I like your code. It is easy to understand and pretty straight-forward. Regarding your question of the measuring units: yes, every variable should have them since you are not operating exclusively on SI units. You have calories (instead of joule) and years and minutes (instead of seconds). I would just change some of the smaller details, from top to bottom:...


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