36

Make it simpler, you can use read-only properties: public class User { public string FirstName { get; } public string Surname { get; } public string Email { get; } public User(string firstName, string surname, string email) { FirstName = firstName; Surname = surname; Email = email; } } Now let's consider to ...


33

All your fields are private and final, which is the first step to being immutable. This is good. The Date class is not immutable, so, for example, you have a problem with the method: public Date getDateOfBirth() { return dateOfBirth; } because, someone could do: employee.getDateOfBirth().setYear(1900); and suddenly your employee has aged... a lot. ...


16

In addition to protecting getDateOfBirth as @rolfl already explained, it's equally important to make defensive copies in constructors too: Person(String name, Date dateOfBirth) { this.name = name; this.dateOfBirth = new Date(dateOfBirth.getTime()); } Otherwise, if you do simply this.dateOfBirth = dateOfBirth;, then somebody could write this code: ...


15

This answer isn't about immutability since @rolfl has it covered. You should put spaces between your = in your assignations, it is easier to spot the = so it makes it easier to see the assignation. There is lots of white spaces between your getters from Employee, one should be enough otherwise the code takes alot of space for nothing. You have a parameter ...


13

First of all, this class is not immutable at all. You can modify both the head and the tail at any given node in the list from calling code. Here are some quick fixes to make the list immutable: public class ImmutableList<T> : IEnumerable<T> { private static readonly ImmutableList<T> _Empty = new EmptyList<T>(); private ...


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


11

First of all, very good job. Now I will point out a few things that I think can be improved on. Code Duplication You have copied over all the private fields from Person to the Builder, which is necessary when the fields are declared final. As coderodde pointed out, using a final declaration is not necessary here when the fields are private and there are ...


10

Implement IEquatable<T> Generally speaking, whenever I find myself writing overrides for object.Equals and object.GetHashCode, I also make the type implement IEquatable<T>. It's generally pretty easy to do so by calling the IEquatable<T>.Equals(T) method from the object.Equals(object) override: public override bool Equals(object obj) { ...


10

You should be able to cut the code length and improve performance, by simplifying the algorithm. Here are some of the problems I've noticed in your implementation: You're currently processing all of the type's properties, this is not necessary as you only need few concrete ones. There are few other problems deriving from this one. You're creating the ...


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


9

Indentation The non-standard end-of-block token indentation is off-putting; this is the first time I see VB code that doesn't align the start and end token columns of code blocks: Public Sub DoSomething() 'instructions End Sub As opposed to: Public Sub DoSomething() 'instructions End Sub Seems you go out of your way to fight the IDE to ...


9

Your view isn't immutable, though immutability might be a property of the underlying object. It's a constant view, meaning the underlying object might change, but not from that view. The canonical way to get a constant view is by the way to use a constant reference, unless the view should erase some of the differences between underlying containers, like std::...


8

It depends on what the goal is. Can the list never change after construction? Can it change but only the ColumnWrapper can do so? Or can it change and everyone is allowed to do that? If the list can not change after construction, consider using an ImmutableList (from google's guava). You should then declare the field columnNames and the return value of ...


8

As mentioned in different answers, Date is a mutable structure so you need to copy it every time you want to share it. My advice is not use at all, and use an immutable structure like Joda Data. The type of salary is double, and thats wrong, you should be using BigDecimal for money and not doubles, and make sure you use the right constructor which is the ...


8

I would prefer the following syntax for its compile-time type-checking: Employee John = humanBuilder .WithValue(t => t.Name, "John") .WithValue(t => t.Age, 32) .Build<Employee>(); Enabled by the following version of WithValue: public ImmutableObjectBuilder<T> WithValue<TValue>( Expression<Func<T, TValue>&...


7

This class should instead use an AtomicReference to ensure the state is kept valid. Alternatively, you should incorporate thread-safe handling of the String. Consider: private final AtomicReference<String> stateref = new AtomicReference<String>(); public void setState(final String state) { // only one initializer will succeed (assuming ...


7

General Review Bad Practices First, you import the whole package: import java.io.*; import java.util.*; It is usually better practice to import specific classes. Also, I don't even see the use of any of these packages. Don't import what you don't need: it will affect performance. Formatting Your second class seems to have two-space indents. Java ...


7

When I think of immutable object I think of get only properties backed with readonly fields. public string Bar { get; private set; } That's not a fully immutable instance you still have the private set but removing the set completely will cause problems in your implementation, because your nested Builder class relies on the fact that it can set them. ...


7

You use IList<> where you should use ICollection<>. I've rarely encountered a scenario where IList<> actually needs to be used. The ICollection<> interface has most of the list's methods, but without everything related to indexing, which you don't use anyway. It's not that big of a deal, but I think it's good knowledge. When you ...


6

It would be better to do the faster checks first in this method: protected bool Equals(Matrix other) { return _values.Cast<double>() .SequenceEqual(other._values.Cast<double>()) && M == other.M && N == other.N; } So we have this: protected bool Equals(Matrix other) { return M == other.M && ...


6

WithItems doesn't play nicely with empty sequences. This line will throw an exception, which I consider unexpected behaviour: var xs = ImmutableList<int>.WithItems(new int[0]); If that code seems contrived, consider this example where GetFoos returns an empty sequence: var xs = GetFoos().ToImmutableList(); // throws InvalidOperationException


6

I usually create an inner class, Copier, which is responsible for copying all values from the original object to the new one. And the Copier class also provides setters that are used to override values. This approach is fairly similar the fluent builder API. A note that you have to firstly copy original values in the constructor in order to make sure all ...


6

First, let me get out of the way that I'm not a "Functional Programmer", and I probably won't ever be. I'm also not an "Object-Oriented Programmer", or any other paradigm. I tend to use whatever the language most lends itself to and what others are doing, and what fits the bill. On that note, to the actual review! I'm only going through First Test for now. ...


6

In this implementation, null entries are problematic. From the public interface, it is impossible to tell if the stack has a null entry, or has reached the bottom: in both cases, top() will return Optional.empty(). It seems wrong to silently convert nulls into Optional.empty()s. I would do one of the following: Do not store null entries in the stack. ...


5

This code assumes that all empty stacks are instances of ImmutableEmptyStack, but the public constructor allows code to create a new ImmutableStack that happens to be empty, so it would be better to use temp.IsEmpty() while(!(temp instanceof ImmutableStack.ImmutableEmptyStack)){ It would be better to make the empty values static members, so you'd need to ...


5

I would use unmodifiableList in that case. That way: public class ColumnsWrapper implements Columns { private final List<String> columnNames; public ColumnsWrapper(List<String> columnNames) { this.columnNames = Collections.unmodifiableList(columnNames); } @Override public List<String> getNames() { ...


5

You claim you have a simple immutable class, but you don't. All public methods on Immutable classes should also be final. For example, you claim your class is immutable, but, I can change it with: class MyColumnsWrapper extends ColumnWrapper { private final List<String> mutableColumnNames; public ColumnsWrapper(List<String> ...


5

Comments - in methods are only useful if they tell you something that is not immediately obvious when you read the code. Your code is immediately obvious, and thus the comments inside the method are completely redundant. I have a note a little later about your JavaDoc though.... Exceptions - In many ways the difference between when checked and unchecked ...


5

The Javadoc style headers you have provided for this method are already more than enough. If you are adding comments to your method it should only be either for API documentation, or where the code is sufficiently complex to warrant an explanation that you cannot provide by simplifying or more appropriately naming the variables. You should however make it ...


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