designing code based on SOLID principals

[Punit Jain]

Hi Everyone,

I have a task which I need to implement with SOLID Design principals. Following is the problem statement:

There is a mechanic droid, which performance the maintenance and repairing of vehicles. It services each vehicle, and then track them manually by giving information about the vehicle to the head maintenance engineer, who then notes it down by hand. My task is to create a program which help mechanic droid to track each vehicle that has been serviced and store the information until it can be uploaded to a central repository. I need to use in memory persistence to store the information.

Following is how I am thinking to design it:

1.) VehicleService interface [which will have the definition of create, update and delete methods]
2.) Vehicle class this would be a basic pojo class which will have vehicle information (i.e. model, year, class etc.)

Questions:
1.) Would it be a good idea to use factory pattern here?
2.) Am I violating the SOLID principals anywhere?
3.) Should every vehicle has it's own individual class, or I should simply instantiate vehicle class every time.
4.) Just to confirm, using collection is in-memory persistence, right?
5.) Any other suggestions?

Thank you in advance.

[Damon McNeill]

Without seeing more of a design, I'm sure your not yet violating any SOLID principles here. Sounds like a legitimate approach, to me. I like the idea of having an immutable "Vehicle" class. Your "VehicleService" can be another immutable class that represents an instance of a service performed to a vehicle. As services are performed, you create and buffer the VehicleServices in memory, then once all services are performed, you iterate over all performed services, writing them to some file or database?

[Damon McNeill]

Also, computer memory is never persistent.. I've never heard of "memory persistence".. it's like an oxymoron.

We usually call that buffering.

[Junilu Lacar]

Punit Jain wrote:I have a task which I need to implement with SOLID Design principals.

That's kind of like saying "I want to build an engine with thermodynamic principles." or "I need to create a ledger and implement it with Generally Accepted Accounting Principles." None of these make sense.

Principles are not implements/tools that you make things with. Rather, they are general guidelines that help you make informed decisions on how to best proceed in doing certain things. By nature, principles are general, so there are many different kinds of situations to which they can be applied. The challenge is to understand and be able to discern when and which particular principle or set of principles apply to any given situation you are faced with.

[Damon McNeill]

Just to preemptively avoid counter attacks, I mean that computer RAM is not persistent. ROM is a kind of "memory", but different in that is is immutable.

[Junilu Lacar]

Damon McNeill wrote:Just to preemptively avoid counter attacks, I mean that computer RAM is not persistent. ROM is a kind of "memory", but different in that is is immutable.

"Counter attacks" are not tolerated well around here. We try to keep discussions civil and rational and we try to answer in the spirit of learning and teaching, not trying to show who's better.

Of course, the in-memory persistence that OP is probably referring to is, as you called it, a way to buffer or temporarily store information until such time that it can be written out to more a permanent or longer-lasting medium. A collection that remains in memory for a while can certainly qualify as an in-memory persistence store. If OP wants to go one step further, he can use any of a number of lightweight databases for Java that can be embedded in the program. Most of those are in-memory databases.

[Junilu Lacar]

Punit Jain wrote:
Following is how I am thinking to design it:

1.) VehicleService interface [which will have the definition of create, update and delete methods]
2.) Vehicle class this would be a basic pojo class which will have vehicle information (i.e. model, year, class etc.)

This is kind of a bottom-up approach or maybe more accurately, starting from closer to the middle/bottom rather than from the top approach. There's nothing wrong with it per se but you can easily get lost and confused with lots of details that you start surrounding and overburdening yourself with prematurely.

Try to balance this out with a high-level contextual definition first. Describe in an abstract, high-level way the behaviors that you want your mechanic droid to have. Once you have an idea of what those general behaviors are, then you can start drilling down to more details and experiment with different design options. Without this context to guide you, you're liable to lose sight of the forest for all the trees you will start trying to cut down.

[Damon McNeill]

Agreed. As I see it, there's only a need for a Vehicle and VehicleService class, and a List<VehicleService>.

In what context are these objects created? There needs to be some top-level design. A "controller", if you will.

[Damon McNeill]

You might also need a Droid class, with some method with a signature like: VehicleService service(Vehicle v)

Maybe the Droid needs a reference to the List<VehicleService>, in order to append it to the list?

There's really not enough context given to suggest more...

[Junilu Lacar]

The way I read the requirements, the program is meant to be loaded into this hypothetical droid to help it perform its tasks, one of which seems to be to manage information about the vehicles it has worked on. That's why OP was asking about in-memory persistence, so the droid can be "offline" while it does its work, then later it can connect and synch up its updates to a central location where vehicle service records are kept.

[Junilu Lacar]

Damon McNeill wrote:You might also need a Droid class, with some method with a signature like: VehicleService service(Vehicle v)

Maybe the Droid needs a reference to the List<VehicleService>, in order to append it to the list?

There's really not enough context given to suggest more...

You seem to have the same kind of tendencies OP is showing: wanting to jump right into design details without the benefit of a high-level context.  At least you also seem to realize that you first need to know more about the context before you can proceed further. I don't like to jump right into the middle of a bunch of details, but then again, I'm a Taurus so that makes me a little bit more cautious and circumspect than most people when it comes to these kind of things.

As I was saying before, it's helpful if you first define some general behaviors of this droid as they relate to its doing vehicle maintenance and repair work. This will give you more context from which to decide what kind of program classes/objects you'll need to create.

[Junilu Lacar]

Punit Jain wrote:
2.) Am I violating the SOLID principals anywhere?

In my experience, the only way you can really tell if you've violated any design principle(s) is when you're looking at actual code and trying to work with it. Violations of design principles makes it difficult to work with code. It's harder to understand, test, debug, extend. Code that violates one or more design principles tends to be overly complex. One important design principle is KISS or Keep It Simple, Silly—I'm using "Silly" here but the more common interpretation is not as nice.

A violation of a design principle may not be immediately apparent either. Often you'll just sense that there's something wrong with the code but you can't quite put your finger on it. This is where an awareness of and ability to discern various code and design smells is useful. Design smells usually lead you to the realization of what design principle has been violated.

So, if you really want to figure out if you are violating any design principles, first learn about code and design smells, then learn how to relate those smells to design principles that are most likely to be violated when those smells are present.

[Damon McNeill]

then again, I'm a Taurus so that makes me a little bit more cautious and circumspect than most people when it comes to these kind of things.  

I'm an Aries, so I tend to jump into  things quickly... lol

[Junilu Lacar]

Damon McNeill wrote:
I'm an Aries, so I tend to jump into  things quickly... lol

Yes, I saw that the other night. Hope you feel a little better, now that you've had a chance to blow off some steam...

[Campbell Ritchie]

Damon McNeill wrote:. . . computer RAM is not persistent. ROM is a kind of "memory", . . .

I think we may have an issue about language. Many people here didn't learn English at their mother's knees, and many people don't realise that jargon is supposed to be very precise. I think the original intent was to persist the state of the object somewhere, but using the phrase memory persistence isn't quite 100% accurate. There are several ways I can think of to persist an object:-

1: Serialise it to a .ser file or similar

2: Write it to an XML file as a JavaBean

3: Write it to an ordinary text file with an application‑specific format

4: Persist it in a database

There are doubtless more. I think that is what OP meant.

[Punit Jain]

Thank you so much for all your replies. Following is how I have implemented so far:

Vehicle.java

package vehicle_maintenance.model;

public class Vehicle {
	private int id;
	private int year;
	private String make;
	private String model;
	private String Class;

	public Vehicle() {
	}

	public Vehicle(int id, int year, String make, String model, String Class) {
		this.id = id;
		this.year = year;
		this.make = make;
		this.model = model;
		this.Class = Class;
	}

	public int getId() {
		return id;
	}

	public void setId(int id) {
		this.id = id;
	}

	public int getYear() {
		return year;
	}

	public void setYear(int year) {
		this.year = year;
	}

	public String getMake() {
		return make;
	}

	public void setMake(String make) {
		this.make = make;
	}

	public String getModel() {
		return model;
	}

	public void setModel(String model) {
		this.model = model;
	}

	public String getCategory() {
		return Class;
	}

	public void setCategory(String category) {
		this.Class = category;
	}
}

MemoryPersistence.java

package vehicle_maintenance.persistence;

import java.util.HashMap;
import java.util.Map;

import vehicle_maintenance.model.Vehicle;

// yet to make it thread safe
public class MemoryPersistence {
	public static Map<Integer, Vehicle> vehicles = new HashMap<>();
	
	public static Map<Integer, Vehicle> getVehicles(){
		return vehicles;
	}
}

VehicleService.java

package vehicle_maintenance.service;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import vehicle_maintenance.model.Vehicle;
import vehicle_maintenance.persistence.MemoryPersistence;

public class VehicleService implements IVehicleService{

	private Map<Integer, Vehicle> vehicles = MemoryPersistence.getVehicles();
	
	public VehicleService(){
		vehicles.put(1, new Vehicle(1, 2012, "Honda", "H1", "M-Class"));
		vehicles.put(2, new Vehicle(2, 2013, "BMW", "B1", "X-Class"));
	}

	/**
	 * Returns the list of all the vehicles
	 *  
	 * @return
	 */
	public List<Vehicle> getAllVehicles() {
		return new ArrayList<Vehicle>(vehicles.values());
	}

	/**
	 * Return Vehicle based on Id
	 * 
	 * @param id
	 * @return
	 */
	public Vehicle getVehicle(int id) {
		return vehicles.get(id);
	}

	/**
	 * To add/create a vehicle to the list
	 *  
	 * @param vehicle
	 * @return
	 */
	public Vehicle createVehicle(Vehicle vehicle) {
		vehicle.setId(vehicles.size() + 1);
		vehicles.put(vehicle.getId(), vehicle);
		return vehicle;
	}

	/**
	 * Update the vehicle.
	 * 
	 * @param vehicle
	 * @return
	 */
	public Vehicle updateVehicle(Vehicle vehicle) {
		if (vehicle.getId() <= 0) {
			return null;
		}
		vehicles.put(vehicle.getId(), vehicle);
		return vehicle;
	}

	/**
	 * Removes the vehicle from the memory/database. 
	 * 
	 * @param id
	 * @return
	 */
	public Vehicle removeVehicle(int id) {
		return vehicles.remove(id);
	}
}

IVehicleService.Java

package vehicle_maintenance.service;

import java.util.List;

import vehicle_maintenance.model.Vehicle;

public interface IVehicleService {
	public abstract List<Vehicle> getAllVehicles();
	public abstract Vehicle getVehicle(int id);
	public abstract Vehicle createVehicle (Vehicle vehicle);
	public abstract Vehicle updateVehicle (Vehicle vehicle);
	public abstract Vehicle removeVehicle (int id);
}

VehicleResource.Java

package vehicle_maintenance.resource;

import java.util.List;

import javax.ws.rs.Consumes;
import javax.ws.rs.GET;
import javax.ws.rs.POST;
import javax.ws.rs.PUT;
import javax.ws.rs.Path;
import javax.ws.rs.PathParam;
import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;

import vehicle_maintenance.model.Vehicle;
import vehicle_maintenance.service.VehicleService;

@Path("vehicles")
public class VehicleResource {
	
	VehicleService vehicleService = new VehicleService();

	/**
	 * get the list of vehicles serviced URL :
	 * http://localhost:8080/vehicle_maintenance/webapi/messages
	 * 
	 * @return
	 */

	@GET
	@Produces(MediaType.APPLICATION_JSON)
	public List<Vehicle> getVehicles() {
		return vehicleService.getAllVehicles();
	}

	/**
	 * get the message by making a rest call URL :
	 * http://localhost:8080/vehicle_maintenance/webapi/messages/{message id}
	 * 
	 * @param messageId
	 * @return
	 */

	@GET
	@Path("/{vehicleId}")
	@Produces(MediaType.APPLICATION_JSON)
	public Vehicle getVehicle(@PathParam("vehicleId") int vehicleId) {
		Vehicle vehicle = vehicleService.getVehicle(vehicleId);
		return vehicle;
	}

	/**
	 * add message by making a rest call URL :
	 * http://localhost:8080/vehicle_maintenance/webapi/messages [pass the json
	 * in the body]
	 * 
	 * @param message
	 * @return
	 */

	@POST
	@Consumes(MediaType.APPLICATION_JSON)
	@Produces(MediaType.APPLICATION_JSON)
	public Vehicle addMessage(Vehicle vehicle) {
		return vehicleService.createVehicle(vehicle);
	}

	
	// INCOMPLETE
	
	@PUT
	@Path("/{vehicleId}")
	@Consumes(MediaType.APPLICATION_JSON)
	@Produces(MediaType.APPLICATION_JSON)
	public Vehicle updateMessage(Vehicle vehicle) {
		return vehicleService.updateVehicle(vehicle);
	}
}

Any suggestion/comments? Appreciate if anyone can do a quick code review?

Note 1: By in-memory, I meant collection.
Note 2: VehicleResource class is still incomplete and I am still working on it.

Thanks in advance!

[Junilu Lacar]

Nothing significant jumps out at me after scanning through that code. However, it's like looking at parts of a car laying on a shop floor. That's the wheel... that's the exhaust... that's the engine...  These classes are kind of just sitting there. I still don't have a context that allows me to see how these classes really interact with each other and do something that will benefit this android and its owner.  A set of unit tests that set up scenarios like what happens when an android is working in offline mode and he services 20 vehicles. What would we expect to find in the android's memory? How do these Vehicle, VehicleResource, and VehicleService types help the android do its job? What happens when the android connects and goes online again? How does it synch its offline data back to the central database?  These are things that the code you have there still doesn't show.

You're still in programmer/tinkerer mode. You're still focused on implementation details. Back up and focus on how you can write something that would be useful, not just technically correct.

[Punit Jain]

Thank you for your response and sorry to make it confusing.

Following is a basic test case I have written:

public class VehicleServiceTest {

	private static Map<Integer, Vehicle> vehicles = MemoryPersistence.getVehicles();
	VehicleService service = new VehicleService();

	@BeforeClass
	public static void initialize() {
		vehicles.put(1, new Vehicle(1, 1960, "T-65B", "X-wing", "Starfighter"));
		vehicles.put(2, new Vehicle(2, 1965, "T-65C-A2", "X-wing", "Starfighter"));
		vehicles.put(3, new Vehicle(3, 1990, "T-70", "X-wing", "Starfighter"));
		vehicles.put(4, new Vehicle(4, 1991, "T-85", "X-wing", "Starfighter"));
	}

	@Test
	public void testUpdateVehicle() {
		Vehicle vehicle = vehicles.get(1);
		vehicle.setYear(2006);
		service.updateVehicle(vehicle);
		assertEquals(2006, vehicles.get(1).getYear());
	}
	
	@Test
	public void testServicedVehicle() {
		Vehicle vehicle = new Vehicle(5, 1998, "T-99", "Z-wing", "starship");
		service.createVehicle(vehicle);
		assertEquals(5, vehicles.size());
	}

	@AfterClass
	public static void cleanUp() {
		vehicles.clear();
	}
}

So basically, the android, services the vehicles and keep them in memory for tracking until they get loaded to a central repository. My task, for now, is to keep the vehicles in memory only, so I am not much concerned about storing them into some kind of non-volatile memory. However, I believe the in memory map can simply be stored in database or file system.

The purpose of VehicleResource is to simply provide a way handle vehicle data over HTTP via rest calls.

A set of unit tests that set up scenarios like what happens when an android is working in offline mode and he services 20 vehicles. What would we expect to find in the android's memory?

Android memory/hashmap should have 20 vehicles.

Note: I am taking an assumption that so far nothing is stored in non-volatile memory. Hence I have not added any check to verify that whether this data is already stored in non-volatile.

Again thanks for all your inputs. helped me to think in more deeper way.
 

[Junilu Lacar]

Now we're getting somewhere. The tests show your point of view as a programmer and designer and it's still very procedural. This is Java; think objects and object-orientation.

Here's what I mean: Why are you calling getVehicles() on line 3? The test class name is VehicleServiceTest and the tests are testUpdateVehicle() and testServicedVehicle(). The code in the tests don't tell a coherent story though.  I see you call the service.updateVehicle() method on line 18 but don't see how that's even connected to the setup you're doing and the assertion you're making.  If anything, you could delete line 18 and lines 16 & 17 would still make me expect line 19 to be good.  The test doesn't make sense to me and it seems pointless.  

Part of the problem is that you've broken the encapsulation of the MemoryPersistence class on line 3.  You've taken a data structure that the MemoryPersistence class should be hiding and managing internally and now you're doing things to it that no client should be doing. Test code should be an example of what clients of your class under test would do. Breaking the MemoryPersistence class' encapsulation is certainly not a good example of what client code should do.

[Junilu Lacar]

This is where focusing on the detailed design before you have a clear picture of the high-level context really gets you in trouble. Your design decisions are mostly based on attributes of the objects that you think are needed in this system.  Object-orientation is mainly about behaviors and relationships/collaborations between different program entities.  Data as represented by object attributes/fields are just enablers: they parameters that objects are bound to adhere to as they perform actions. Like if a Vehicle had only half a tank full of gas, it can only run as many miles as that much fuel will allow, given the vehicle's efficiency.  The amount of fuel and the vehicle's efficiency are the parameters that affect it's ability to go distances.  When you focus on attributes instead of behaviors, you're turning the whole point of object-orientation on its head and that's not a good thing.

Here's a telltale sign that you focused more on attributes that behaviors: Your Vehicle class has no interesting methods. It's all getters and setters.  Your MemoryPersistence class is the same way. It's just a field and a getter for that field. In fact, there's not even instance fields or methods in it. The two members you have there are static and therefore belong to the class, not a specific instance.

I won't even start with the VehicleResource and the IVehicleService interface but there's not a whole of nice things about those either.

Here's what I'd suggest to you as your next steps. Think about how this mechanic android does its work. What would the android need to know about each Vehicle that it works on? What kind of information does it need to keep about each Vehicle that it works on? How often does the Android synch its cached data with the central application? What updates does the android need to get from the central application to continue doing more work once it has uploaded and synched its latest batch of Vehicle Servicing information?

Draw out the story and concentrate on behaviors and relationships/collaborations.  The fields/attributes will follow these, not the other way around.

[Punit Jain]

Thank you for your response.

Line 3 was a silly mistake , I have updated my test cases and updateVehicle API in my service class as following:  

VehicleServiceTest.java

public class VehicleServiceTest {

	static VehicleService service = new VehicleService();

	@BeforeClass
	public static void initialize() {
		Vehicle vehicle1 = new Vehicle(1, 1960, "T-65B", "X-wing", "Starfighter");
		Vehicle vehicle2 = new Vehicle(2, 1965, "T-65C-A2", "X-wing", "Starfighter");
		Vehicle vehicle3 = new Vehicle(3, 1990, "T-70", "X-wing", "Starfighter");
		Vehicle vehicle4 = new Vehicle(4, 1991, "T-85", "X-wing", "Starfighter");
		service.createVehicle(vehicle1);
		service.createVehicle(vehicle2);
		service.createVehicle(vehicle3);
		service.createVehicle(vehicle4);
	}
	
	@Test
	public void testUpdateVehicle() {
		Vehicle vehicle1 = new Vehicle(1, 2006, "T-65B", "X-wing", "Starfighter");
		service.updateVehicle(vehicle1);
		assertEquals(2006, service.getAllVehicles().get(0).getYear());
	}
	
	@Test
	public void testGetAllVehicleForMake() {
		List<Vehicle> vehhicle1 = service.getAllVehiclesForMake("T-65B");
		assertEquals(1, vehhicle1.size());
	}
	
	@Test
	public void testGetAllVehicleForModel() {
		List<Vehicle> vehhicle1 = service.getAllVehiclesForModel("X-wing");
		assertEquals(4, vehhicle1.size());
	}
	
	@Test
	public void testGetAllVehicleForClass() {
		List<Vehicle> vehhicle1 = service.getAllVehiclesForClass("Starfighter");
		assertEquals(4, vehhicle1.size());
	}
	
	@Test
	public void testUpdateVehicleNegative() {
		Vehicle vehicle1 = new Vehicle(10, 2006, "T-65B", "X-wing", "Starfighter");
		service.updateVehicle(vehicle1);
		for(Vehicle vv : service.getAllVehicles()){
			assertFalse("", vv.getId() == vehicle1.getId());
		}
	}
	
	@Test
	public void testGetVehicle() {
		Vehicle vehicle1 = service.getVehicle(1);
		assertEquals("T-65B", vehicle1.getMake());
	}
	
	@Test
	public void testServicedVehicle() {
		Vehicle vehicle = new Vehicle(5, 1998, "T-99", "Z-wing", "starship");
		service.createVehicle(vehicle);
		assertEquals(5, service.getAllVehicles().size());
	}

VehicleService.java

public Vehicle updateVehicle(Vehicle vehicle) {
		if (VehicleUtils.isEmptyOrNull(vehicle) || vehicle.getId() <= 0) {
			return null;
		}
		Set<Integer> keys = vehicles.keySet();
		for (int key : keys) {
			if (key == vehicle.getId()) {
				vehicles.put(vehicle.getId(), vehicle);
			}
		}
		return vehicle;
	}

Moreover, the reason I don't have anything other than getter and setter in my Vehicle class is because I have given with only those fields and supposed to use only them.  

Also, changed the MemoryPersistence and removed the static fields to make a separate copy available for every instance.

[Junilu Lacar]

Sorry, that doesn't look much better to me.  Remember how I said that what you posted as your production code looks like a bunch of car parts just sitting there on the shop floor? To me, your tests look like you're just walking around, pointing to a part and saying "This is the engine!" and "This is the exhaust!"

Your tests still haven't shown how these things you've defined are going to come together to make your mechanic android actually do some useful work.

I'll post an example of what I meant when I said "Draw out the story and concentrate on behaviors and relationships/collaborations."

[Junilu Lacar]

Here's an example of a "story" that sets up a high-level context for your design:

STORY: Mechanic Droid gets a list of Vehicle Service Orders to work on.

A mechanic droid will work offline most of the time. At the start of its work shift, the droid will connect to the main Vehicle Service system (VSS) and receive a number of Service Orders (SO) to work on. Each SO will have all the information the droid needs to find the vehicle to be serviced and perform all the tasks need to bring the vehicle to its standard operating capacity (SOC). Each SO will also have an Estimated Time to Complete (ETC), which is predetermined and is the average time it takes to complete the type of servicing involved. The droid must account for this time and only accept as many SOs as it can reasonably complete within a six-hour window (two-thirds of a droid's full work shift).  The difference of two hours is to allow for unforeseen complications in the servicing. Based on historical records, there is a 10% probability that the actual work needed to service a vehicle will take more time than the ETC.

So, given this context, here's part of the Test class that I'd probably end up writing to implement this story (I'm using some JUnit 5 features in this example):

@DisplayName("Droid gets list of Service Orders to work on")
public class DroidGetsServiceOrdersTest {

   Droid mechanic = new MechanicDroid();

   @Nested
   @DisplayName("At start of shift")
   class AtStartOfShift {
        
        @Test
        @DisplayName("droid has no service orders") 
        void noServiceOrders() {
             assertFalse(mechanic.hasOutstandingServiceOrders());
        }
 
        @Test
        @DisplayName("droid has maximum available capacity")
        void hasMaximumAvailableCapacity() {
              assertThat(mechanic.availableCapacity(), is(ONE_HUNDRED_PERCENT));
        }

        // ... more tests for when droid hasn't synched up with VSS yet
   }

   @Nested
   @DisplayName("After receiving one Service Order") 
   class ReceivesOneServiceOrder {

        // Fixtures for this nested test
        private ServiceOrder serviceOrder;
       
        // setup code that initializes the SO and simulates droid receiving the SO from the VSS
        ...
       
        @Test
        @DisplayName("has one outstanding Service Order")
        void trackServiceOrdersReceived() {
             assertThat(mechanic.outstandingOrdersCount(), is(1));
        }

        @Test
        @DisplayName("has reduced available capacity based on the Estimated Time to Complete")
        void knowsItsOwnLimitations() {
             assertThat(mechanic.availableCapacity(), is(lessThan(ONE_HUNDRED_PERCENT));
             // or
             // assertThat(..., is(expectedCapacity()));
        }
   
        // ... more tests to describe what to expect after receiving on Service Order

        // convenience methods for this nested test

        private int expectedCapacity() {
             return 100 - mechanic.capacityUsedFor(serviceOrder);
        }

   }

   // ... more nested test classes to check different states of synching up with SOs

}

Do you see how very little implementation detail I'm showing in these tests? Do you see what I mean by "telling a story"?

[Junilu Lacar]

Read through that hypothetical test class I wrote. Can you pick out the parts of the Droid API that I'd want to start implementing? Does the API not suggest the type of information that you'd need to have in order to give the droid the kind of behavior and capabilities that the test code implies? This is what I meant when I said that the fields/attributes will follow from the behaviors/capabilities of the objects that you define.

In here and all throughout this example, you'll see decisions that were guided by SOLID. This is what I meant when I said at the top of this thread that principles are not implements or tools that you make things with. They are guidelines that help you make informed decisions on how to best proceed in doing certain things.

[Junilu Lacar]

Here are two more principles that are even more fundamental than SOLID: 1. Choose good names, and 2. Be consistent.

Consider what I wrote:
   @Test    @DisplayName("has one outstanding Service Order")    void trackServiceOrdersReceived() {         assertThat(mechanic.outstandingOrdersCount(), is(1));    }
Most programmers will read that and just move past it without even pausing a second to consider whether or not this is a consistent test. Does it tell a consistent story? Are all the names consistent? On closer examination, this test is not consistent at all.

The DisplayName says one thing and the assertion on line 38 is pretty consistent with what is stated. However, the test method name is not consistent. It conveys a related idea but the nuance is pronounced enough to warrant some refactoring to get everything in this method aligned with the idea of tracking the number of outstanding service orders a droid is working on.

One of the hardest problems in programming is finding good names. In fact, some will say that finding good names is THE single most difficult thing about programming. I'll leave it to you as an exercise to find a better name for that test method.

[Junilu Lacar]

There's more refactoring I'd do to make the test code more consistent.

If you read the DisplayName annotations and string up the sentences they form as you drill down to detailed test methods, you'd see sentences like this:

"At start of shift droid has no service orders"
"At start of shift droid has maximum available capacity"

"After receiving one Service Order has one outstanding Service Order"
"After receiving one Service Order has reduced available capacity based on the Estimated Time to Complete"

These last two sentences are not very consistent. This is the nice thing about this new feature in JUnit 5; when used judiciously, it can help you find smells in your test code.

I'd refactor those @DisplayName tags so that I'd have the tests say it like this instead:

"After receiving one Service Order droid has one outstanding work item"
"After receiving one Service Order droid has reduced available capacity based on the SO's Estimated Time to Complete (ETC)"

[Junilu Lacar]

One last thing to consider: Do you see how none of the classes you initially came up with are even referenced in that test? This demonstrates how badly you can get misled by focusing on fields and attributes and not having a high-level context to guide your design decisions.

[Junilu Lacar]

Now, let's go back to a requirement you mentioned in your original post, where you said the droid would upload information about the maintenance service it performs on vehicles. What would be the story there?

I'd imagine it would be something like this.

STORY: Droid synchs service record updates to Vehicle Service System

At the end of its work shift, a mechanic droid will connect to the VSS and upload its completed Service Orders. The VSS will then take the completed SOs and update each vehicle's service history.

Do you think you can think of some candidate tests to write for this story?

[Punit Jain]

Thank you so much.

How about this :

public class VehicleServiceTest {
	
	private static int ONE_HUNDRED_PERCENT = 10; // 10 - means 100% available.
	IVehicleService service = new VehicleService();
	 
	   @Nested
	   @DisplayName("At start of shift")
	   class AtStartOfShift {
	         
	        @Test
	        @DisplayName("droid has no service orders") 
	        void noServiceOrders() {
	             assertTrue(service.getAllVehicles().isEmpty());
	        }
	  
	        @Test
	        @DisplayName("droid has maximum available capacity")
	        void hasMaximumAvailableCapacity() {
	              assertThat(service.getAvailableCapacity(), is(ONE_HUNDRED_PERCENT));
	        }
	   }
	 
	   @Nested
	   @DisplayName("After receiving one Service Order") 
	   class ReceivesOneServiceOrder {
	 
	        @Test
	        @DisplayName("After receiving one service order droid has one outstanding Service Order")
			void trackServiceOrdersReceived() {
	        	createVehicles(1);
				assertThat(service.getOutstandingOrdersCount(), is(1));
			}
	
	        @Test
	        @DisplayName("After receiving one service order droid has reduced available capacity based on the Estimated Time to Complete")
	        void knowsItsOwnLimitations() {
	        	createVehicles(1);
	        	assertThat(service.getAvailableCapacity(), is(9));
	        }
	   }
	   
	   @Nested
	   @DisplayName("After receiving more than 10 Service Order") 
	   class ReceivesMoreThanTenServiceOrder {
	 
	        @Test
	        @DisplayName("After receiving more than 10 service orders droid has more than one outstanding Service Order")
			void trackServiceOrdersReceived() {
	        	createVehicles(13);
				assertTrue(service.getOutstandingOrdersCount() > 1);
			}
	
	        @Test
	        @DisplayName("After receiving more than 10 Service Order droid has reduced available capacity based on the Estimated Time to Complete")
	        void knowsItsOwnLimitations() {
	        	createVehicles(13);
	        	assertTrue(service.getAvailableCapacity() > (ONE_HUNDRED_PERCENT-3));
	        }
	   }
	   
	   private void createVehicles(int vehiclesToCreate){
		   for(int vehicles = 1; vehicles <= vehiclesToCreate; vehicles++){
			   Vehicle vehicle = new Vehicle(vehicles, 1960+vehicles, "T-65B", "X-wing", "Starfighter");
			   service.addVehicle(vehicle);
		   }
	   }
}

I have refactored most of my code. Following is refactored, MemoryPersistent Class :

public class MemoryPersistence {

	// Assuming the droid's maximum limit for one batch is 10 vehicles and after that 
	// it should process those 10 to non-volatile memory.
	private static int MAX = 10;
	
	public Map<Integer, Vehicle> vehicles = new HashMap<>();

	/**
	 * Adds vehicles to map. 
	 * The maximum limit to add vehicles for a particular instance is 10. Droid should keep 
	 * updating the vehicles to non-volatile memory.
	 * 
	 * @param key
	 * @param value
	 * @return
	 */
	public void put(Integer key, Vehicle value) {
		if (vehicles.size() >= MAX && !vehicles.containsKey(key)) {
			addVehiclesToNonVolatileMemory(new ArrayList<Vehicle>(vehicles.values()));
			vehicles.clear();
		} else {
			vehicles.put(key, value);
		}
	}

	/**
	 * Return in-memory vehicles
	 * 
	 * @return
	 */
	public Map<Integer, Vehicle> getVehicles() {
		return vehicles;
	}
	
	/**
	 * There should be code to add services vehicles to non-volatile memory.
	 * This code currently sets a flag to those vehicles, which has been serviced.
	 * 
	 * @param vehicles
	 */
	public void addVehiclesToNonVolatileMemory(List<Vehicle> vehicles){
		for(Vehicle vehicle: vehicles){
			vehicle.setUpdatedToNonVolatile(true);
			//TODO: There should be code to add Serviced Vehicles to Non-Volatile Memory.
		}
	}
}

Also added few API's in my service class to return the outstanding and available numbers.

Again, Thanks a lot.

[Junilu Lacar]

Punit Jain wrote:How about this :

public class VehicleServiceTest {
	
	private static int ONE_HUNDRED_PERCENT = 10; // 10 - means 100% available.
	IVehicleService service = new VehicleService();
	 
	   @Nested
	   @DisplayName("At start of shift")
	   class AtStartOfShift {
	         
	        @Test
	        @DisplayName("droid has no service orders") 
	        void noServiceOrders() {
	             assertTrue(service.getAllVehicles().isEmpty());
	        }
	  
	        @Test
	        @DisplayName("droid has maximum available capacity")
	        void hasMaximumAvailableCapacity() {
	              assertThat(service.getAvailableCapacity(), is(ONE_HUNDRED_PERCENT));
	        }
	   }

Not quite. You are still hung up on your initial "design." Throw all that stuff away. Wipe it from memory because it will only lead you one way: The Wrong Way.

The code I quoted above only has a few lines of code that are different from what I gave as an example. Yet, you have changed the story tremendously and not for the better. It's like you took the script for the next episode of Star Wars and re-introduced Jar Jar Binks—who most people hate—and added in an appearance by the Power Rangers. I'll let you find your own adjective for something like that.

On line 3, the constant is named ONE_HUNDRED_PERCENT but you assign a value of 10. Furthermore, you try to explain yourself with a redundant comment, overlooking the fact that most people will be wondering WHY you assigned the value 10 instead of 100 or even just 1.0 and double instead of int? When you have to put a comment like that, it's your subconscious telling you that people will probably not understand why you're doing something. Adding a comment is an ineffective attempt to mitigate a mistake you subconsciously already know you're making. It may work for you but the comment doesn't work for other people and certainly doesn't work well for the test or the production program code.

Besides, the ONE_HUNDRED_PERCENT constant was something I purposely put in there to hopefully show later on that it needed to be refactored. Unfortunately, the smell of the constant was a little subtle and you followed the lead, probably thinking it was a good thing, and actually made it even worse and in need of even more refactoring.

The changes you made on line 13 and 19 pretty much made the tests inconsistent and frankly, nonsensical. Neither of those display names match what the body of the tests are doing and you've made what was explicit more implicit. You've basically taken the dots and the lines that connected them and erased the lines and some of the dots.

Even if you make local fixes to the modifications you made to the code, continuing to focus on the VehicleService is still wrong, in my opinion, and shows that you still don't have a clear vision of the big picture.

Starting with something like a VehicleService makes no sense to me in the context of the problem you were given. The problem revolves around a Mechanic Droid. Yes, the droid does vehicle servicing but neither the vehicles nor the servicing done on them is the main character, the droid is.. It's like you were given the general outline of the Star Wars saga and you started detailing out what an X-Wing fighter is and what its navigation and control parameters are. Luke, Leah, Han, Darth, Obi Wan, those are all the main characters; that's where you should focus first to fill out the script. Likewise, the droid is the main character of this story so start with the droid: what are its behaviors and responsibilities and what will it need to collaborate with to exhibit its behaviors and carry out its responsibilities.

[Junilu Lacar]

I have refactored most of my code. Following is refactored, MemoryPersistent Class :

public class MemoryPersistence {

	// Assuming the droid's maximum limit for one batch is 10 vehicles and after that 
	// it should process those 10 to non-volatile memory.
	private static int MAX = 10;
	
        ...
	
	/**
	 * There should be code to add services vehicles to non-volatile memory.
	 * This code currently sets a flag to those vehicles, which has been serviced.
	 * 
	 * @param vehicles
	 */
	public void addVehiclesToNonVolatileMemory(List<Vehicle> vehicles){
		for(Vehicle vehicle: vehicles){
			vehicle.setUpdatedToNonVolatile(true);
			//TODO: There should be code to add Serviced Vehicles to Non-Volatile Memory.
		}
	}
}

See, it didn't take long for the smell of the ONE_HUNDRED_PERCENT constant you created in the test code to spread and infect your production code. Worse yet, if I'm interpreting and diagnosing your thought process correctly, you've masked the fact that MAX here and ONE_HUNDRED_PERCENT in the test code are actually related or exactly the same idea.

I'm guessing this is the reason you felt you needed to add the comment in the test code, because you had somehow created a connection between the abstract idea of "maximum capacity" as expressed in the test code and the implementation detail in the production code that would determine that capacity, which is the size of the collection that will hold Vehicle information.

Here's another thing: the addVehiclesToNonVolatileMemory() again smacks of the programmer/tinkerer's mindset and a strong focus on implementation detail.  Again, think about the client code instead. Outside client code cares absolutely nothing about whether the persistence is happening in volatile or non-volatile memory. That is an implementation detail. It should not be reflected in the name of a method that is for public consumption.

[Junilu Lacar]

Here's another way comments can be smelly:
   /**     * There should be code to add services vehicles to non-volatile memory.     * This code currently sets a flag to those vehicles, which has been serviced.     *     * @param vehicles     */    public void addVehiclesToNonVolatileMemory(List<Vehicle> vehicles){        for(Vehicle vehicle: vehicles){            vehicle.setUpdatedToNonVolatile(true);            //TODO: There should be code to add Serviced Vehicles to Non-Volatile Memory.        }    }
Once again, you exposed implementation details in something that's for public consumption: The JavaDocs. Don't make those kind of comments in JavaDocs. If you're going to leave yourself or other developers a note about implementation, do it like this:
   /**     * ...     *     * @param vehicles     */    public void addVehiclesToNonVolatileMemory(List<Vehicle> vehicles){     // *** Developer Note ***     // @TODO There should be code to add services vehicles to non-volatile memory.           // Implementation Note:     // This code currently sets a flag to those vehicles, which has been serviced.        for(Vehicle vehicle: vehicles){            vehicle.setUpdatedToNonVolatile(true);            //TODO: There should be code to add Serviced Vehicles to Non-Volatile Memory.        }    }

[Junilu Lacar]

If you spend enough time reading through the standard library JavaDocs, you'll see a few cases where it's appropriate to comment about implementation-specific details in JavaDocs. One is when you're explaining the choice of an algorithm. Another is when you want to highlight some kind of implementation detail that can potentially affect performance of code that uses your class/method, such as when you're programming for concurrency. There may be other reasons, but these two are the most common ones I've seen. Those JavaDocs may look like this:

/**
 * Sorts the given list of BlahBlah objects.
 *
 * <strong>Implementation Note:<strong> this implementation uses the insertion sort algorithm because
 * the list of BlahBlah objects is not expected to reach sizes where a different algorithm
 * would give better performance. This may change in future versions of the application 
 * if we find that performance is becoming an issue.
 * ...
 */

In Java 8, you can use the new @implNote tag:

/**
 * Sorts the given list of BlahBlah objects.
 *
 * @implNote this implementation uses the insertion sort algorithm because
 * the list of BlahBlah objects is not expected to reach sizes where a different algorithm
 * would give better performance. This may change in future versions of the application 
 * if we find that performance is becoming an issue.
 * ...
 */

When you're still trying to come up with a workable design, you should not even be thinking about these kinds of comments yet.

[Junilu Lacar]

   /**     * There should be code to add services vehicles to non-volatile memory.     * This code currently sets a flag to those vehicles, which has been serviced.     *     * @param vehicles     */    public void addVehiclesToNonVolatileMemory(List<Vehicle> vehicles){        for(Vehicle vehicle: vehicles){            vehicle.setUpdatedToNonVolatile(true);            //TODO: There should be code to add Serviced Vehicles to Non-Volatile Memory.        }    }
There's also another more disturbing smell the comments in this code is giving off. They reveal a lack of focus and consistency in purpose.  The method name says that you're just adding things to the persistence store. Yet, the comment says the code is also setting a flag on the vehicles. Why would you mix the concern of setting a flag on the vehicles in a method that should only be worried about the task of adding to the persistence store? You're violating SRP right there, the first SOLID principle.

[Junilu Lacar]

Given everything I have told you so far, I'd like to challenge you to find the smells in this test that you wrote:
      @Nested       @DisplayName("After receiving more than 10 Service Order")       class ReceivesMoreThanTenServiceOrder {                  @Test            @DisplayName("After receiving more than 10 service orders droid has more than one outstanding Service Order")            void trackServiceOrdersReceived() {             createVehicles(13);                assertTrue(service.getOutstandingOrdersCount() > 1);            }                @Test            @DisplayName("After receiving more than 10 Service Order droid has reduced available capacity based on the Estimated Time to Complete")            void knowsItsOwnLimitations() {             createVehicles(13);             assertTrue(service.getAvailableCapacity() > (ONE_HUNDRED_PERCENT-3));            }       }
Identify the lines of code (yes, there are multiple) that smell and explain why they smell.

[Junilu Lacar]

And just to show that I have no delusions of being a perfect designer myself, I'll do the same exercise with some of the code from the example I gave:
  @Nested   @DisplayName("After receiving one Service Order")   class ReceivesOneServiceOrder {        // Fixtures for this nested test        private ServiceOrder serviceOrder;                // setup code that initializes the SO and simulates droid receiving the SO from the VSS        ...                @Test        @DisplayName("droid has one outstanding Service Order")        void trackServiceOrdersReceived() {             assertThat(mechanic.outstandingOrdersCount(), is(1));        }
There is still a smell of inconsistency and lack of clarity/explicitness here.

The nested test is supposed to be for a scenario where the droid has just received one Service Order. The test says "track service orders received" yet the body of the test says nothing about "tracking orders received". Instead, the body of the test refers to "how many outstanding orders" there are. While the connection between "tracking services orders" and "how many outstanding orders" can be inferred, relying on the reader to be able to make that inference is a tenuous proposition at best. At worst, the reader won't make the connection at all.  The text in the display name reflects this problem of the implicit dot and line that connects the actions of "receiving a service order", "tracking service orders", and the state of "having more outstanding service orders."

A clearer and more explicit test might express the ideas like this:
  @Nested   @DisplayName("After receiving one Service Order, ")   class AfterReceivingOneServiceOrder {        // Fixtures for this nested test        ServiceOrder serviceOrder;        int initialOutstandingSOs;        @BeforeEach        void initializeWithOneServiceOrder() {           initialOutstandingSOs = mechanic.outstandingOrdersCount();           // create a SO and simulate droid receiving it from central source           serviceOrder = ...           mechanic.accept(serviceOrder);        }                @Test        @DisplayName("the droid will have one more outstanding SO.")        void droidCountsItAsOutstanding() {             assertThat(mechanic.outstandingOrdersCount(), is(initialOutstandingSOs + 1));        }
Before refactoring:
- Combined display name is "After receiving one Service Order droid has one outstanding Service Order"
- The display name, test method name, and test code do not convey a consistent idea.

After refactoring:
- Combined display name is "After receiving one Service Order, the droid will have one more outstanding SO."
- The display name, test method name, and test code convey a more consistent idea.
- Removed the hardcoded value 1 and made the expectation more generic and relative to the initial state that
was capture in the @BeforeEach setup method.

I also made the nested class name more consistent with its Display Name.

[Junilu Lacar]

@OP: You may be wondering why I never use a reference to a Vehicle or VehicleService as you have tried to do in your code.  I don't feel there's a need to know about an entire Vehicle and therefore I don't feel like there's a need to interact with a service that will help me obtain references to Vehicle objects. All the droid is doing is service and maintenance work and I feel all it needs to track related to that work are vehicle parts that were serviced, resources that were used (oil, electricity, special equipment like anti-gravity lift devices and plasma torches, etc.), and how much time and effort went into the servicing. That's why I keep referencing Service Order instead of a VehicleService or Vehicle. If there's anything that has to do with Vehicle, I could imagine a Service Order having a field of type VehicleTracking to find the location of the Vehicle and some of its basic information (like Vehicle Type) so that the droid can find appropriate parts and servicing procedure manuals.

This is why I said that focusing on all those classes and their fields/attributes in isolation was a way to get yourself trapped in the thick of the trees and lose sight of the forest.

[Punit Jain]

Junilu Lacar wrote:

The changes you made on line 13 and 19 pretty much made the tests inconsistent and frankly, nonsensical. Neither of those display names match what the body of the tests are doing and you've made what was explicit more implicit. You've basically taken the dots and the lines that connected them and erased the lines and some of the dots.

But if the memory is empty, which means there are on service orders droid has. That's what I am checking at line 13.

Junilu Lacar wrote:
Starting with something like a VehicleService makes no sense to me in the context of the problem you were given. The problem revolves around a Mechanic Droid. Yes, the droid does vehicle servicing but neither the vehicles nor the servicing done on them is the main character, the droid is.. It's like you were given the general outline of the Star Wars saga and you started detailing out what an X-Wing fighter is and what its navigation and control parameters are. Luke, Leah, Han, Darth, Obi Wan, those are all the main characters; that's where you should focus first to fill out the script. Likewise, the droid is the main character of this story so start with the droid: what are its behaviors and responsibilities and what will it need to collaborate with to exhibit its behaviors and carry out its responsibilities.

Agreed that my starting point should be focusing and writing the droid class. However, In the problem statement I am given to assume that there is already a droid which performs the task of maintaining and servicing vehicles. Nothing other than that.  

[Punit Jain]

Junilu Lacar wrote:Given everything I have told you so far, I'd like to challenge you to find the smells in this test that you wrote:
      @Nested       @DisplayName("After receiving more than 10 Service Order")       class ReceivesMoreThanTenServiceOrder {                  @Test            @DisplayName("After receiving more than 10 service orders droid has more than one outstanding Service Order")            void trackServiceOrdersReceived() {             createVehicles(13);                assertTrue(service.getOutstandingOrdersCount() > 1);            }                @Test            @DisplayName("After receiving more than 10 Service Order droid has reduced available capacity based on the Estimated Time to Complete")            void knowsItsOwnLimitations() {             createVehicles(13);             assertTrue(service.getAvailableCapacity() > (ONE_HUNDRED_PERCENT-3));            }       }
Identify the lines of code (yes, there are multiple) that smell and explain why they smell.

1.) At line 47, the display name says about outstanding order but the method name is order received.
2.) At line 57, instead of using greater than I could have check for the exact number.
3.) Again the method name and display names are inconsistent at line 54 and 55.

[Junilu Lacar]

Punit Jain wrote:
1.) At line 47, the display name says about outstanding order but the method name is order received.
2.) At line 57, instead of using greater than I could have check for the exact number.
3.) Again the method name and display names are inconsistent at line 54 and 55.

This is only scratching the surface. On #2 above, your suggested mitigation would make the smell worse.

Try harder and look at the example of what I did to highlight the smells in my own tests.