Project 2 Peer Review thread

[susanBuck]

In this week's assignment (Week 9, Mar 29), I'll prompt you to complete a peer review of a classmate’s Project 2. You will share your completed review in a reply to this thread.

Full instructions of the peer review process are in the Week 9 assignment. Peer review spreadsheet...

[dauger27]

What is the Github URL of the project you are reviewing?

https://github.com/gabichuela85/e15/tree/main/p2

What is the production URL of the project you are reviewing?

http://e15p2.gabybrown.me/

Are you able to produce any errors or unexpected results when using the site?

Yes

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

No, view and controller logic are properly split up logically.

Are there any parts of the code that you found interesting or taught you something new?

I found that the Arr::sort functionality used within the index function to be really interesting. I hadn’t known that you could set the attribute that you’d sort on for a multidimensional array like that.

Are there any parts of the code that you don't understand?

Is there a reason why you created the list of movies as links (/movies) to pages that did not exist rather than just listing out the text? Any consideration for making those links jump to the review page instead?

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

The logical flow of the application makes sense to me, I couldn’t think of any changes I would make to the application.

Are there aspects of the code that you feel were not self-evident and would benefit from comments?

Explanation of what the functions do without needing to review the code to find out.

Are there any best practices discussed in course material that you feel were not addressed in the code?

Code has no comments to explain any functionality. You have to review the functionality to determine what happens within any given function.

Do you have any additional comments not covered in the above questions?

The main blade references a css/movie.css which doesn’t exist within the project. Likely a holdover before bootstrap was used.

[pllealfunes]

Review for archerdave P2

Github URL: https://github.com/archerdave/e15/tree/main/p2 Production URL: http://e15p2.dharvill.me/

Review

I was unable to produce any unexpected errors. I did not find an issue when it came to separation of concerns and there wasn't any unnecessary code or comments left over in the views or controllers. The project followed best practices and was mostly clear.

I liked that you used one controller for the assignment and there wasn't a lot of code to work through. However, I believe the BackgroundController could benefit to at least one comment to quickly explain what is going on in the first function as it took me a good minute to fully understand what was going on. Still a great way to handle the session data.

I am curious though why in the form.blade.php for the first and last name values why they don't use, "old"

Other than that your project was creative and fun. Good job!

[luannebe]

Review for Hobbyist

1. Github URL: https://github.com/pllealfunes/e15/tree/main/p2
2. Production URL: http://e15p2.lefthandedcat.me/

3. Errors/unexpected results?

The app worked almost perfectly. I stumbled across just one small error...

If the user, chooses the Food category and asks for 10 suggestions, Laravel returns the following "ValueError" - array_rand(): Argument #2 ($num) must be between 1 and the number of elements in argument #1 ($array)

and highlights line 35 in the SearchController.php $arrayResults = array_rand($filterCategory,$suggestionNumber);

The php documentation for array_rand() states:

Trying to pick more elements than there are in the array will result in an E_WARNING level error, and NULL will be returned.

The error is happening because there are just seven Food hobbies in the .json file.

4. Separation of concerns between controllers/view?

Yes! the code is very well organized.

5. Interesting/illuminating code?

I learned about the php array methods array_filter() and array_rand() in SearchController.php. Thanks!

6. Any difficult to understand code?

Nope, all was very easy to understand. The code was self-evident, and the comments helpful and to the point.

7. Any code that could be more efficient or clear?

In the SearchController, the block that begins on line 33 creates an array of suggestions ($arrayResults), then pushes the suggestions into a second array ($searchResults). Perhaps the conditional code based on $suggestionNumber is not needed, and just one array would suffice:

Foreach ($hobbies as $hobby) {
     $filterCategory = array_filter($hobby[$categories]);
    // separate issue: perhaps handle case where $suggestionNumber exceeds number of items in array. See 3 above.
     $searchResults = array_rand($filterCategory,$suggestionNumber);
}

8. Code that should be commented (not self-evident)? 9. Overlooked best practices? It all looks great to me!

10. Additional comments:

Well done! A lovely app and elegantly written code.

The app provided many appealing ideas for fun hobbies, and I especially enjoyed the motivational quotes ... very uplifting ... thank you!!!

[mgarimelHES]

Hi luannebe,

Thanks for the opportunity to review you app and share my thoughts based on my understanding. It is great solution covering the project needs in a concise manner. My review follows -

Questions

1. What is the Github URL of the project you are reviewing?

https://github.com/luannebe/e15/tree/main/p2

2. What is the production URL of the project you are reviewing?

http://e15p2.flyingdog.nu/

3. Are you able to produce any errors or unexpected results when using the site?

I am not sure if it is in the scope or not, getting 404 error while doing the http://e15p2.flyingdog.nu/abc

4. Is their appropriate separation of concerns amongst controllers and views? E.g.is there any non-display specific logic in views, or display-specific code in controllers?

I dont know it could be related to separation of concerns, other way could be using the controller to process this logic in the view 'welcome.blade.php'

@php // change "voice" of question to make it sound like a response $response = str_ireplace(' my ', ' your ', $question); $response = rtrim($response,'?'); @endphp

5. Are there any parts of the code that you found interesting or taught you something new?

Yes, I liked the way it is designed and implemented concisely as per the project-2 requirements.

6. Are there any parts of the code that you don't understand?

Well commented and clean & concise. I liked it.

7. Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

I am not sure when the user could select only '1, 3 or 5', but in the logic it is going up to 10. For example in the view 'welcome.blade.php' code

        @case('9')
                        <div class="relative [transform-style:preserve-3d] animate-flip9">
                        @break
                    @case('10')
                        <div class="relative [transform-style:preserve-3d] animate-flip10">
                        @break

8. Are there aspects of the code that you feel were not self-evident and would benefit from comments?

Pretty self explanatory, clean and clear.

9. Are there any best practices discussed in course material that you feel were not addressed in the code?

I think some comments could be removed or changed to help to understand the code. For example, in the following comments in the 'PageController.php' lines 54 -56

// do stuff with the info
    // for each toss get a random number between 3 and 10
    // and put into an array

For example, the toss allows 1, 3 or 5. But in the code it is allowed range is 3 - 10. It could be related to next phase?

10. Do you have any additional comments not covered in the above questions?

    Thanks for the opportunity to review your application, it is an awesome app as per the needs of project-2 and appreciate your time to review my thoughts.

When applicable, provide context in the form of code snippets or reference to specific files/line numbers when answering these questions.  Question 3 2

[luannebe]

Hi @mgarimelHES,

Thank you for your comments on my P2 project! I very much appreciate your identifying and calling attention to areas that need improvement.

It IS unclear, I see now ... The '1, 3, 5' refers to the number of coins tossed, while 3-10 is the number of times a coin 'flips' before it "lands." An even number of flips will end up as 'heads'; odd as 'tails'. The variation in number of flips was intended to add some suspense while waiting to see how the coins land.

Item 4: I wasn't sure about this either ... perhaps @susanBuck could let us know.

Item 3: Drawing a blank here ... what does /abc refer to?

Thanks again for your insights and kind words :)

[YvaGithub]

What is the Github URL of the project you are reviewing?

https://github.com/mgarimelHES/e15/tree/main/p2

What is the production URL of the project you are reviewing?

http://e15p2.hesweb.me/

Are you able to produce any errors or unexpected results when using the site?

No

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

No, the seperation of concern practice is fully applied.

Are there any parts of the code that you found interesting or taught you something new?

Not really, the author has pretty much followed the materials covered in the lectures.

Are there any parts of the code that you don't understand?

No, I understand everything.

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

Everything makes total sense.

Are there aspects of the code that you feel were not self-evident and would benefit from comments?

No, everything is clear.

Are there any best practices discussed in course material that you feel were not addressed in the code?

No

Do you have any additional comments not covered in the above questions?

I believe Murthy has done a wonderful job. Especially the way he commented out every piece of the code to explain what it does. That is something that I will apply in my future works.

[archerdave]

Review of the P2 application written by @xul889

What is the Github URL of the project you are reviewing?

https://github.com/xul889/e15/tree/main/p2

What is the production URL of the project you are reviewing?

http://e15p2.hiwoocafe.com/

Are you able to produce any errors or unexpected results when using the site?

Running the application did not produce any bugs, or unexpected errors. However, there were some circumstances where I expected errors but didn’t get one. For example, when I put 123-45=00.7 into the “Drink that you want" field, the application happily accepted the input instead of rejecting it.

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

Yes, the appropriate types of code are in the appropriate types of files.

Are there any parts of the code that you found interesting or taught you something new?

• I liked the use of JSON to store and access data for the application.
• Good use of the CSRF method.

Are there any parts of the code that you don't understand?

The input section displays inputs on three rows. However only the middle one is inside of a div with class “row”. I’m not sure if this is an accident, or on purpose for a specific stylistic effect.

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

A few places, yes.

• When assigning request data to local variables (FoodController, lines 21 - 25), you are specifying that the variables should be assigned null if values are not available from the request variable. However, the lines immediately above (15 - 19) had just validated all of them as required, guaranteeing that they do in fact have some value stored.

  $request->validate([
  'drink' => 'required',
  'ingredient' => 'required',
  'spicy' => 'required',
  ]);

  $ingredient = $request->input('ingredient',null);
  $spicy = $request->input('spicy',null);
  $drink = $request->input('drink',null);
  $foodData = file_get_contents(database_path('food.json'));
  $food = json_decode($foodData,true);

• When checking the ingredient value against the JSON data (FoodController, lines 28 - 33), you loop through every object in the JSON data, regardless of whether you have already found a match.

  $foodResult = [];
  foreach ($food as $key => $value) {
  if(strtolower($value['ingredient']) == strtolower($ingredient)){
      $foodResult[] = $value;
  }
  }

• When checking the ingredient value against the JSON data (FoodController, line 30, see above comment block), strtolower() is performed on both the input and the internal data. However, the input is from a Select list, and you are in control of what values go into it. You could have used values that match what is in your JSON data set, and not needed to run the function on either piece of data at all.

• When returning from the process(Request $request) function (FoodController, lines 35 - 40) the ingredient value alone obtains its value from the request object. However, the $ingredient variable had already pulled its value from the request object, just like the drink and spicy values in this return block.

  $request->validate([
  'drink' => 'required',
  'ingredient' => 'required',
  'spicy' => 'required',
  ]);
  $ingredient = $request->input('ingredient',null);
  $spicy = $request->input('spicy',null);
  $drink = $request->input('drink',null);
  $foodData = file_get_contents(database_path('food.json'));
  $food = json_decode($foodData,true);

  and then

  return redirect('/')->with([
  'drink' => $drink,
  'ingredient' => $request->ingredient,
  'spicy' => $spicy,
  'food' => $foodResult,
  ]);

  Are there aspects of the code that you feel were not self-evident and would benefit from comments?

• The loop over the JSON data (FoodController, lines 28 - 33) would benefit from some explanation.

  $foodResult = [];
  foreach ($food as $key => $value) {
  if(strtolower($value['ingredient']) == strtolower($ingredient)){
      $foodResult[] = $value;
  }
  }

• In some cases when using a default value, it is written as old(‘foo’, $foo), and sometimes as old('foo', 'foo'). It's not clear why the change of use is occurring.

Are there any best practices discussed in course material that you feel were not addressed in the code?

• Old commented-out code has been left in both the controller file and the view file.
• Code indenting is inconsistent. For example, in the specific view (hw.blade.php) there is an @if / @endif block (lines 49 - 55) where all of the code is indented between those tags. Right below that is another @if / @endif block (lines 57 - 65) where the internal code is at the same indentation level as the tags.

  @if(count($errors) > 0)
  <ul class='alert'>
      @foreach ($errors->all() as $error)
          <li for="error">{{ $error }}</li>
      @endforeach
  </ul>
  @endif

  vs

  @if($food)
  <ul class="list-group">
  <li>Drink: {{$drink}}</li>
  <li>Ingredient: {{$ingredient}}</li>
  <li>Meal Flavor: {{$spicy}}</li>
  <li>Food Choosen: {{$food[0]['name']}}</li>
  <li>Food description: {{$food[0]['description']}}</li>
  </ul>
  @endif

Do you have any additional comments not covered in the above questions?

• In the application output field, the ingredient values and the spiciness values are simply echoed back to the user, unchanged from their internal states. This means that I see ingredients such as “vegetable”, even though your internal stores as “Vegetable”, and the spiciness output can read as “notspicy”. (Notice the lack of space). Presentation to the user can be more pretty.
• The master view file does not have the closing body or html tags.
• In the hw.blade.php file, the li elements in the errors section (line 52) uses a “for” attribute inside of an li element. <li for="error">. This is not a valid attribute for this element.
• I would have liked to see a stronger validation of input on the drink name element. Maybe a check against alpha, or alphanumeric?
• This code performs a look up based on one of the three inputs. The other two are simply echoed back to the user, and have no impact on the output.

Overall, the code has been written neatly. There are some details as noted above that could use some polishing up, but in general it is quite clear what the code is doing.

[archerdave]

@pllealfunes thank you for the review!

You asked: I am curious though why in the form.blade.php for the first and last name values why they don't use, "old"

They do not use the old() in the blade file because I already ran the input through old() in the createImage() method, which is called from accessing /.

Route::get('/', [BackgroundController::class, 'createImage']);

public function createImage(Request $request)
{
    $firstName   = old('firstName', null);

At this point $firstName has either a validated value (if it came from a submitted form) or null if this is the first time the page is loading.

In form.blade.php, the code reads

<input type='text' id='firstName' name='firstName' placeholder='Terry' autofocus value='{{$firstName, null}}'/>

and $firstName at this point is already either a value from old() or is null, so old() isn't repeated here. Come to think of it, I don't need the default null value either!

And finally, I had intended to remove the old() method from some of the other form inputs, and apparently overlooked that change.

[gkorodi]

What is the Github URL of the project you are reviewing?

https://github.com/patrickgarsow-harvard/e15/tree/main/p2

What is the production URL of the project you are reviewing?

http://e15p2.patrickgarsow.me/

Are you able to produce any errors or unexpected results when using the site?

Yes. Several links point to 404 pages. However, validation of available forms are working. The template used has built in validation. The "Page Content" field on the "Departments" CRUD page is not being saved, where it is supposed to be saved. (don't have access to the .env file, but the template used should have some sort of DB backend?) Many of the links, including the "Register" link mentioned on the README.md file is not working.

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

yes, since Vue.js views are used, which is more JS specific. No PHP views, so there is clear separation

Are there any parts of the code that you found interesting or taught you something new?

I am still getting familiar with "Traits" in Laravel. Using it once for password validation might not follow the pattern it is designed, but I can see how a validator "trait" could be a great addition for handling data.

Are there any parts of the code that you don't understand?

I am not familiar vith Vue and the view file structure in resources/js directory

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

I could not find the backend handling code, where the models get persisted. Perhaps some /api routes for CRUD operations? It is perhaps handled by one of the packages in the vendor directory.

Are there aspects of the code that you feel were not self-evident and would benefit from comments?

Many of the Fortify authentication scaffolding could be grouped together, in a better way perhaps, or in a single class?

Are there any best practices discussed in course material that you feel were not addressed in the code?

Not that I know of.

Do you have any additional comments not covered in the above questions?

I am sorry I could not provide a better review due to my lack of understanding of Vue.js and its layout requirements.

[xul889]

What is the Github URL of the project you are reviewing? https://github.com/luannebe/e15/tree/main/p2

What is the production URL of the project you are reviewing? http://e15p2.flyingdog.nu/

Are you able to produce any errors or unexpected results when using the site? NO.

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers? No

Are there any parts of the code that you found interesting or taught you something new? TailWind CSS is used which is inspiring for my next project. It is powerful and easy to use.

Are there any parts of the code that you don't understand? No.

Are there any parts of the code that you feel could be written in a more efficient or clearer manner? In welcome.blade.php, from line 62 to 89, there might be a way to loop the style to make it cleaner.

Are there aspects of the code that you feel were not self-evident and would benefit from comments? No. Code is clear and easy to understand.

Are there any best practices discussed in course material that you feel were not addressed in the code? No. I love this project.

Do you have any additional comments not covered in the above questions? Yes. The layout before a user click the button should look more in the center. Right now, it is left aligned. I would make the coins show in the below, if I were him, then align all the items in the center. In that case, the layout will be consistent.

[patrickgarsow-harvard]

Review for Dauger27

What is the Github URL of the project you are reviewing?

• https://github.com/dauger27/e15/tree/main/p2

What is the production URL of the project you are reviewing?

• http://e15p2.dauger.me

Are you able to produce any errors or unexpected results when using the site?

• Was able to produce an unexpected result when chosing option "Both" but not selecting the checkbox for Scamble 2/3 letter. Multiple clicks of the process button will scramble the letters in a pattern that keeps going whether or not scramble is chosen.

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

• Everything is separated appropriately...views are in the resource->views area and the functions for the application are in the controller.

Are there any parts of the code that you found interesting or taught you something new?

• The building out of the controller was different than how I would find myself handling it so that was very helpful in learning a new way to handle the functions in the application. In dauger27's controller there is an overarching process function that handles determining what options are selected in the form and running the appropriate functions based off of that. I think I would have handled it different based on the form options chosen to have a different controller function handle the work and then handle the redirection.

Are there any parts of the code that you don't understand?

• I am initially finding it difficult to follow the stripping out of the non-alpha characters and how they are being replaced. It looks like they might impact the result though of the outcome of the application functions so not sure if that is expected. Meaning if a shift occurs around a number did it really shift x characters? Would that be considered correct or incorrect?

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

• I feel like the best practice would be to clean up the input when it is not in the acceptable format. That way I know what is being passed to my worker functions is clean and requires less error handling. This should also help with cutting back on code duplication or excessive logic functions like for loops and if/else statements.

Are there aspects of the code that you feel were not self-evident and would benefit from comments?

• Commenting was done well in the application.

Are there any best practices discussed in course material that you feel were not addressed in the code?

• I think best practice was followed and the code is formatted in a logical way.

Do you have any additional comments not covered in the above questions?

• I liked the form...while simple it was nicely done to give it a very fresh, interactive feeling.

[mgarimelHES]

Hi @mgarimelHES,

Thank you for your comments on my P2 project! I very much appreciate your identifying and calling attention to areas that need improvement.

It IS unclear, I see now ... The '1, 3, 5' refers to the number of coins tossed, while 3-10 is the number of times a coin 'flips' before it "lands." An even number of flips will end up as 'heads'; odd as 'tails'. The variation in number of flips was intended to add some suspense while waiting to see how the coins land.

Item 4: I wasn't sure about this either ... perhaps @susanBuck could let us know.

Item 3: Drawing a blank here ... what does /abc refer to?

Thanks again for your insights and kind words :)

Hi , it is now clear to me about the number of flips. Regarding item-3, I am referring to any string (ex- 'abc') which is suffixed to the URL, it will return 404. just fyi.

Thanks for the opportunity

[mgarimelHES]

What is the Github URL of the project you are reviewing?

https://github.com/mgarimelHES/e15/tree/main/p2

What is the production URL of the project you are reviewing?

http://e15p2.hesweb.me/

Are you able to produce any errors or unexpected results when using the site?

No

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

No, the seperation of concern practice is fully applied.

Are there any parts of the code that you found interesting or taught you something new?

Not really, the author has pretty much followed the materials covered in the lectures.

Are there any parts of the code that you don't understand?

No, I understand everything.

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

Everything makes total sense.

Are there aspects of the code that you feel were not self-evident and would benefit from comments?

No, everything is clear.

Are there any best practices discussed in course material that you feel were not addressed in the code?

No

Do you have any additional comments not covered in the above questions?

I believe Murthy has done a wonderful job. Especially the way he commented out every piece of the code to explain what it does. That is something that I will apply in my future works.

Hi Yvagithub, appreciate your comments and time taken to review my application. Have a great rest of the Spring 2022!

Thanks

[patrickgarsow-harvard]

@gkorodi Thank you for reviewing my app. I am sorry that you experienced the issue with buttons like the register button not working. In this case I am not sure how to fix it as I am able to get it running on a different server environment but just not our development servers. I am investigating a resolution and will post an update once I figure it out.

I am sorry that I wrote the views in Vue.js as I didn't know there would be peer reviewing going on and I knew the instructor had experience with Vue.js. I really appreciate you working through the review of the views and hopefully you seen and learned a little from the implementation. I will say I really like using Vue.js with Laravel because of how the views get organized with the JS code...ie each "view" gets a template area for html, etc and also a script area for your on page JS code.

Finally, the backend connections weren't done exactly as a project should be as you have now learned from the lessons following project 2. I did not create migrations for all of the tables because I was still learning how to do that and not everything would work so I manually edited the database config. The only thing that was up and working for P2 was the department database. That did have a model connected with it and the routes were represented in the web.php routes file.

Again, @gkorodi thank you for reviewing my project I appreciate the input and will look to improve upon my next iteration of the project.

[gkorodi]

Patrick,

Don't be sorry. I have meant my comments as more like guiding questions, rather than comments. I am not sure if they are errors, or if my lack of knowledge makes me a bad tester.

I am sure your great-looking website would be a great success, and my "issues" will help rather than prevent you from completing it.

Gabe

On Wed, Apr 6, 2022 at 9:22 PM patrickgarsow-harvard < @.***> wrote:

@gkorodi https://github.com/gkorodi Thank you for reviewing my app. I am sorry that you experienced the issue with buttons like the register button not working. In this case I am not sure how to fix it as I am able to get it running on a different server environment but just not our development servers. I am investigating a resolution and will post an update once I figure it out.

I am sorry that I wrote the views in Vue.js as I didn't know there would be peer reviewing going on and I knew the instructor had experience with Vue.js. I really appreciate you working through the review of the views and hopefully you seen and learned a little from the implementation. I will say I really like using Vue.js with Laravel because of how the views get organized with the JS code...ie each "view" gets a template area for html, etc and also a script area for your on page JS code.

Finally, the backend connections weren't done exactly as a project should be as you have now learned from the lessons following project 2. I did not create migrations for all of the tables because I was still learning how to do that and not everything would work so I manually edited the database config. The only thing that was up and working for P2 was the department database. That did have a model connected with it and the routes were represented in the web.php routes file.

Again, @gkorodi https://github.com/gkorodi thank you for reviewing my project I appreciate the input and will look to improve upon my next iteration of the project.

— Reply to this email directly, view it on GitHub https://github.com/susanBuck/e15-spring22/issues/45#issuecomment-1090981417, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB6NARD3HO4LOQBIRUL6ILVDY2D3ANCNFSM5R77ZDPQ . You are receiving this because you were mentioned.Message ID: @.***>

[xul889]

Review of the P2 application written by @xul889

What is the Github URL of the project you are reviewing?

https://github.com/xul889/e15/tree/main/p2

What is the production URL of the project you are reviewing?

http://e15p2.hiwoocafe.com/

Are you able to produce any errors or unexpected results when using the site?

Running the application did not produce any bugs, or unexpected errors. However, there were some circumstances where I expected errors but didn’t get one. For example, when I put 123-45=00.7 into the “Drink that you want" field, the application happily accepted the input instead of rejecting it.

Is their appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers?

Yes, the appropriate types of code are in the appropriate types of files.

Are there any parts of the code that you found interesting or taught you something new?

• I liked the use of JSON to store and access data for the application.
• Good use of the CSRF method.

Are there any parts of the code that you don't understand?

The input section displays inputs on three rows. However only the middle one is inside of a div with class “row”. I’m not sure if this is an accident, or on purpose for a specific stylistic effect.

Are there any parts of the code that you feel could be written in a more efficient or clearer manner?

A few places, yes.

• When assigning request data to local variables (FoodController, lines 21 - 25), you are specifying that the variables should be assigned null if values are not available from the request variable. However, the lines immediately above (15 - 19) had just validated all of them as required, guaranteeing that they do in fact have some value stored.

$request->validate([
    'drink' => 'required',
    'ingredient' => 'required',
    'spicy' => 'required',
]);

$ingredient = $request->input('ingredient',null);
$spicy = $request->input('spicy',null);
$drink = $request->input('drink',null);
$foodData = file_get_contents(database_path('food.json'));
$food = json_decode($foodData,true);

• When checking the ingredient value against the JSON data (FoodController, lines 28 - 33), you loop through every object in the JSON data, regardless of whether you have already found a match.

$foodResult = [];
foreach ($food as $key => $value) {
    if(strtolower($value['ingredient']) == strtolower($ingredient)){
        $foodResult[] = $value;
    }
}

• When checking the ingredient value against the JSON data (FoodController, line 30, see above comment block), strtolower() is performed on both the input and the internal data. However, the input is from a Select list, and you are in control of what values go into it. You could have used values that match what is in your JSON data set, and not needed to run the function on either piece of data at all.
• When returning from the process(Request $request) function (FoodController, lines 35 - 40) the ingredient value alone obtains its value from the request object. However, the $ingredient variable had already pulled its value from the request object, just like the drink and spicy values in this return block.

$request->validate([
    'drink' => 'required',
    'ingredient' => 'required',
    'spicy' => 'required',
]);
$ingredient = $request->input('ingredient',null);
$spicy = $request->input('spicy',null);
$drink = $request->input('drink',null);
$foodData = file_get_contents(database_path('food.json'));
$food = json_decode($foodData,true);

and then

return redirect('/')->with([
    'drink' => $drink,
    'ingredient' => $request->ingredient,
    'spicy' => $spicy,
    'food' => $foodResult,
]);

Are there aspects of the code that you feel were not self-evident and would benefit from comments?

• The loop over the JSON data (FoodController, lines 28 - 33) would benefit from some explanation.

$foodResult = [];
foreach ($food as $key => $value) {
    if(strtolower($value['ingredient']) == strtolower($ingredient)){
        $foodResult[] = $value;
    }
}

• In some cases when using a default value, it is written as old(‘foo’, $foo), and sometimes as old('foo', 'foo'). It's not clear why the change of use is occurring.

Are there any best practices discussed in course material that you feel were not addressed in the code?

• Old commented-out code has been left in both the controller file and the view file.
• Code indenting is inconsistent. For example, in the specific view (hw.blade.php) there is an @if / @endif block (lines 49 - 55) where all of the code is indented between those tags. Right below that is another @if / @endif block (lines 57 - 65) where the internal code is at the same indentation level as the tags.

@if(count($errors) > 0)
    <ul class='alert'>
        @foreach ($errors->all() as $error)
            <li for="error">{{ $error }}</li>
        @endforeach
    </ul>
@endif

vs

@if($food)
<ul class="list-group">
    <li>Drink: {{$drink}}</li>
    <li>Ingredient: {{$ingredient}}</li>
    <li>Meal Flavor: {{$spicy}}</li>
    <li>Food Choosen: {{$food[0]['name']}}</li>
    <li>Food description: {{$food[0]['description']}}</li>
</ul>
@endif

Do you have any additional comments not covered in the above questions?

• In the application output field, the ingredient values and the spiciness values are simply echoed back to the user, unchanged from their internal states. This means that I see ingredients such as “vegetable”, even though your internal stores as “Vegetable”, and the spiciness output can read as “notspicy”. (Notice the lack of space). Presentation to the user can be more pretty.
• The master view file does not have the closing body or html tags.
• In the hw.blade.php file, the li elements in the errors section (line 52) uses a “for” attribute inside of an li element. <li for="error">. This is not a valid attribute for this element.
• I would have liked to see a stronger validation of input on the drink name element. Maybe a check against alpha, or alphanumeric?
• This code performs a look up based on one of the three inputs. The other two are simply echoed back to the user, and have no impact on the output.

Overall, the code has been written neatly. There are some details as noted above that could use some polishing up, but in general it is quite clear what the code is doing.

Thank you for your comments. It is very detailed and helpful. It make all sense and I will learn from it to become a better programmer.

[gabichuela85]

• What is the Github URL of the project you are reviewing? https://github.com/archerdave/e15/tree/main/p2

• What is the production URL of the project you are reviewing? http://e15p2.dharvill.me/

• Are you able to produce any errors or unexpected results when using the site? The instructions on the first page say that the avatar field is optional, but if you do not enter anything in the field the form fails because it states 'The icon field is required.' If this were pre-selected to the 'no-icon' option it would not produce the error.

• Is there appropriate separation of concerns amongst controllers and views? E.g. is there any non-display specific logic in views, or display-specific code in controllers? There is an appropriate separation of concerns.

• Are there any parts of the code that you found interesting or taught you something new? I appreciated the use of $errors->first in the views file, this produced a line by line error as opposed to the list of errors that my project output at the bottom of the forms page.

• Are there aspects of the code that you feel were not self-evident and would benefit from comments? I found the code to be very straightforward.

• Are there any best practices discussed in course material that you feel were not addressed in the code? There could be more comments throughout the code, though I also need to work on commenting out my own code.

[archerdave]

@gabichuela85 thank you for taking the time to look over my project. Feedback from a fresh set of eyes is always helpful.

I wanted to respond to your third bullet, about requiring the avatar. Not because I feel my code needs defending, but to illustrate a particular scenario. My intent with those settings was "You have to specify what you want, even if what you want is nothing." What I ended up with was a conflicting set of indicators for the user, and appropriate confusion. I probably should have added a few sentences to the application to explain this. The lesson here is "The user will NEVER read your mind."