Win a copy of Murach's Java Programming this week in the Beginning Java forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

designing code based on SOLID principals  RSS feed

 
Punit Jain
Ranch Hand
Posts: 1030
2
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10929
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Marshal
Posts: 54882
155
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
    Ranch Hand
    Posts: 1030
    2
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thank you so much for all your replies. Following is how I have implemented so far:

    Vehicle.java


    MemoryPersistence.java



    VehicleService.java



    IVehicleService.Java



    VehicleResource.Java




    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 1030
    2
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thank you for your response and sorry to make it confusing.

    Following is a basic test case I have written:



    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 1030
    2
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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



    VehicleService.java



    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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):

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Here are two more principles that are even more fundamental than SOLID: 1. Choose good names, and 2. Be consistent.

    Consider what I wrote:

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 1030
    2
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thank you so much.

    How about this :



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



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

    Again, Thanks a lot.
     
    Junilu Lacar
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Punit Jain wrote:How about this :

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I have refactored most of my code. Following is refactored, MemoryPersistent Class :

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Here's another way comments can be smelly:

    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:

     
    Junilu Lacar
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:

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

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Given everything I have told you so far, I'd like to challenge you to find the smells in this test that you wrote:

    Identify the lines of code (yes, there are multiple) that smell and explain why they smell.
     
    Junilu Lacar
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:

    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:

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    @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
    Ranch Hand
    Posts: 1030
    2
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 1030
    2
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:

    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
    Sheriff
    Posts: 10929
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.
     
    It is sorta covered in the JavaRanch Style Guide.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!