MP REST client review

[lauracowen]

I didn't have time to hands-on test it so these comments are from running the app and then reading and re-reading the guide (rather than from building it myself from the instructions). So there may be things I've missed but hopefully this is a start (let me know if it's not clear what I mean in any of my comments - I'm doing this quite quickly...) :)

• The guide seems to be a nice length. Possibly a few too many sub-headings in the navigation at the side (making it look more complex than it is but I didn't have time to work out whether it was possible to simplify the structure). It's not critical anyway as it's not long.
  Simplified by removing sub-headings
  What you'll learn section:

• How is this app different from the way it’s implemented in the REST intro guide? And why would you do it this way as it seems to get the same result? I think this needs to be clearer in the intro.
  JS: Did you mean to say the MicroProfile Intro guide, we might want to update all other guides using this rest client method soon tho, so there won't be any difference
  LC: No, I meant this one: https://openliberty.io/guides/rest-intro.html because the microservices talk to each other and the app works - why would you implement this application (the inventory--service scenario) in this way rather than that way? what's the benefits of one vs the other?
  JS: Talked to Laura, confirmed that she refers to the CDI/MP-intro guide

• Why two ways of implementing the RESTful client?
  Added more explanation in the intro, please check
  LC: Looks much clearer, thanks. Is the list of two bullets related to that? It's nice and clearly laid out but it seems to duplicate stuff from the middle of the previous paragraph. Can the bullet points be moved up and everything edited down slightly to avoid redundancy?

  • “In addition, you will build the implementation of the RESTful client with two approaches: Contexts and Dependency Injection (CDI) or RestClientBuilder.” <- It needs to explain what the two approaches mean, why there are two, and when you’d use one or the other (or what the purposes are). I think you touched on this later in the guide but I'm not absolutely clear what the reason for them both is.
  • I can’t work out from the “Instantiating the RESTful client” section what’s happening. Is the user creating two things in the same application? Or are they alternatives? In the introductory section of the guide, it says “or” but I’m thinking now it means “and”.

• “Copy all the following code and replace the contents of the src/main/java/io/openliberty/guides/inventory/client/SystemClient.java file:” (and other instances of the same thing) <- no. That’s not the style. The user creates the file and pastes in the whole text. Don't have them pasting over existing files or editing files (unless there's no other way round it). See https://qa-guides.mybluemix.net/guides/rest-intro.html for example of the style and use it consistently for each class file.
  Updated the style
  Instantiating section:

• “Inject the client called defaultRestClient to the InventoryManager class using CDI. ” <- I’m not sure “using CDI” is meaningful enough. You’re not ‘using CDI’, you’re using annotations to inject the dependencies, I think? So maybe “Inject the client called defaultRestClient to the InventoryManager class using annotations. ” (if that’s technically correct?) Avoid hiding meaning behind names of technologies if you can.
  Updated

• "Injecting the client with CDI and Config" <⁃ Similarly, I don’t think it’s meaningful enough to talk about “MicroProfile Config” - very few people know what that is yet. The guide needs to talk in terms of what it does instead of its name.
  Avoided talking about Config directly, but explained that users will use configuration files

• "Then, add two more annotations to the..." <- Don’t have them adding additional annotations in later in the guide. Just have them create the whole class in the first place and (if necessary) say that errors are currently displaying because something hasn’t been created yet. The user hasn’t needed to compile and run the app before this instantiating section so it’s fine to have compilation errors. I know it's not exactly the way a developer would develop the app for themselves but they're here to learn about an app that already exists and you just need to explain what the important parts do so they can play around with the code and maybe adapt it for their own purposes.
  Updated

Building and running section:

• Does the ‘getting the internal IP address’ command work on all OSes? It looks quite Unix-y.
  Removed how to get IP address, asked the reader to get their IP by their own way
  You're done section:

• Is there anything that you could suggest the user experiment with changing to see what happens? eg a property that has a different effect? Or change the value of something? You don't need to tell them what will happen or how to fix it etc, just suggest they make that change. The training wheels are off now...they need to do something for themself and see what happens. This is something we should be doing for all the guides but see what you can come up with.
  Added something to suggest the user to try in the end of the testing section

Things @sishida will probably say too but I'll mention because I wrote them down as I went:
The following issues are all fixed

• “Note that the service will automatically build and generate an implementation” <- “Note that” is redundant. Just say “The service automatically builds…”. And make it present tense whenever you’re talking about how the technology works. Only use future tense when you’re talking about the person (and only if it’s genuinely in the future).

• Only highlight in monospace names of things that actually exist. eg there’s no such thing as “the system client”. Don’t highlight ‘system’. Or, if you need to be clear what you're talking about (which seems like a good approach) just use its proper name SystemClient. If you're talking descriptively about the SystemClient's purpose, it's fine to say "the system client" but avoid using two different ways after that to refer to the same thing - while it's nice in a novel, it's confusing in technical writing. (“Now you need to create a RESTful client interface that represents the system service, call it the system client. ”)

• “In the system client, you add a response ” <- Again (and in several other places throughout), the highlighting isn’t necessary as there’s no client called ‘system’. I think you can just say “In the SystemClient class” or something each time you refer to it? See what works best.

• Is it necessary to say “Read the next section to see more details.” Isn’t that what they’ll do anyway? Especially as the very next line uses the terms you've just used.

• General comment (not this guide specifically, I think) - the H3 headings look a bit lost - like they should be more bold or something? Has this changed? Or is it just me?
  Simplified by removing sub-headings

[JunyiSun]

• Fixed all mentions of system client to SystemClient in PR #15.
• More update in PR #16 #17 .

[JunyiSun]

"How is this app different from the way it’s implemented in the REST intro guide? And why would you do it this way as it seems to get the same result? I think this needs to be clearer in the intro."

I am not sure if we will update all the previous guides to use this mp-rest-client method, so there might not be any difference in the future? @yeekangc @evelinec Any opinions?

[lauracowen]

I guess a related question is, even if we updated the other guides in future, at that point would there be a need for both the REST intro guide and this one (for example)? I think @NottyCode mentioned there might be some duplication with an older guide but I can't remember which one.

[lauracowen]

I haven't re-read the whole thing but your responses to each look good. I think someone with more experience of writing clients (but not having reviewed this already) could do with reading through it from start to finish to check it with a fresh eye but I think it looks pretty good. Thank you!

[JunyiSun]

Thank you Laura!

[yeekangc]

In terms of how does this compare to what's in the REST Intro guide as well as the other guides like those on Consuming REST, basically, MP REST Client offers a simpler way to programmatically access a REST service -- Instead of writing a bunch of lower level client code using e.g. JSON-P, you simply need to define an interface and inject a client.

We expect we will update the MP series of guides to use MP REST Client is using MP REST Client would be considered best practice. We can further discuss whether REST Intro itself shall be updated since it straddles the fence.

For the other guides on Consuming REST, we will keep them as-is since they deal with different ways to consume a REST service and MP REST Client is only one of them.

[yeekangc]

Certainly, I am with @lauracowen in that the value and why for the different approaches whether it is MP REST Client itself vs others or the 2 different approaches with MP REST Client itself should be clearly called out.

[evelinec]

All the issues raised here had been addressed. Closed.