search

susanBuck / e15-spring21

1 stars 0 forks source link

Project 2 Peer Review thread #28

Open susanBuck opened 3 years ago

susanBuck commented 3 years ago

In this week's videos (Week 9, Thu Apr 1), I'll cover the instructions for completing a peer review of Project 2. You will share your completed review in a reply to this thread.

adejeffchen commented 3 years ago

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

What is the production URL of the project you are reviewing? http://jfill.samjanvey.com

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

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 appropriate separation of concerns. Good job.

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

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

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

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

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

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

zrashidharvard2020 commented 3 years ago

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

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

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

http://e15p2.kerynegan.me/

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

No I could not able to produce any errors or unexpected results.

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

In views views/columnar/show.blade.php to non-display logic has been implemented a) if there is no error then alert div is not displaying b) On success if display is not null then the application is displaying the result

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

In the controller file CipherController.php on line 65 and 66 is very interesting to me. $rand = random_int(65,90); $msgarray[] = chr($rand); This block of code generates the random character. It is very simple but interesting logic .

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

I am not clear about the this block of code in controller file CipherController.php line 86 and 87 unset($temparray); $temparray = []; why in first line unset and on the second line reassigning the empty array?

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

Compare to Route::get('/columnar/create', [CipherController::class, 'create']); I should be in using Post method to submit secure data and also used the {!! csrf_field() !!} field on blade file views/columnar/show.blade.php

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

It would be a good idea if there is a comments on controller file CipherController.php at line 91 to explaine why this block of code written if($sortlr == 'right'){ foreach($newmsg as $key=>$row){ foreach($row as $k=>$value){ $rightmsg[] = $value; }; }; };

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

I think in this project the variable naming convention was not followed according to best practice guidelines.

karnaout commented 3 years ago

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

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

Are you able to produce any errors or unexpected results when using the site? No, everything works as expected

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?

Apart from a very minor one ( the page title not being injected in its specific page), everything else looked good. The homepage title tag is getting its value from the layout view file rather than getting set in the homepage view file which might seem a little counterintuitive. https://github.com/adejeffchen/e15/blob/15f3f0cdac519a6209a010a318dbbde9364a44da/p2/resources/views/layouts/main.blade.php#L5

Are there any parts of the code that you found interesting or taught you something new? Maybe. Vue script is getting set up in the view file. I don’t have any Vue experience so I’m not sure if this must be done the way it is but my basic understanding of this application, tells me that this is something that could’ve been done in the controller maybe by assigning this whole script code to a variable and pass it on to the view file or maybe by placing this script in a dedicated JS file and loading it in the view file.

Are there any parts of the code that you don't understand? No, not in the sense that there was anything confusing code structure-wise. However, I didn’t understand why Jeff used withInput() on this line below https://github.com/adejeffchen/e15/blob/08bdab70e6fd3f716bef95c53272ebe5c76303b3/p2/app/Http/Controllers/PageController.php#L80 And after doing some Googling around, I think I learned that you need to use it with redirect to be able to pass your data in with() to your redirected page.

Are there any parts of the code that you feel could be written in a more efficient or clearer manner? Only on the documentation side. Other than that, everything looked very straightforward.

Are there aspects of the code that you feel were not self-evident and would benefit from comments? No, I didn’t.

Are there any best practices discussed in course material that you feel were not addressed in the code? No really. This is probably nitpicking but the

Screen Shot 2021-04-07 at 11 41 10 AM

Do you have any additional comments not covered in the above questions? Overall, the code looked very clean, expressive, understandable, and well-documented. Looking at the code and seeing how Jeff tackled some problems, I can say, I definitely learn a few things here and there. :)

abirray-harvard commented 3 years ago

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

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

Are you able to produce any errors or unexpected results when using the site? No, I was not able to produce any errors or unexpected results with the site.

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? There is appropriate separation of concerns amongst controllers and views.

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

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

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

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

Are there any best practices discussed in course material that you feel were not addressed in the code? Unused commented code can be removed, like: https://github.com/zrashidharvard2020/e15/blob/master/p2/resources/views/bmi-calculator/index.blade.php#L32 https://github.com/zrashidharvard2020/e15/blob/master/p2/app/Http/Controllers/BmiCalculatorController.php#L58 https://github.com/zrashidharvard2020/e15/blob/master/p2/app/Http/Controllers/BmiCalculatorController.php#L60

Unused functions in BmiCalculatorController, like: https://github.com/zrashidharvard2020/e15/blob/master/p2/app/Http/Controllers/BmiCalculatorController.php#L30 https://github.com/zrashidharvard2020/e15/blob/master/p2/app/Http/Controllers/BmiCalculatorController.php#L41 https://github.com/zrashidharvard2020/e15/blob/master/p2/app/Http/Controllers/BmiCalculatorController.php#L84

Do you have any additional comments not covered in the above questions? I’m nitpicking here but I think it would be nice to include the inputted values along with the resulting BMI in the output. This way, users can confirm their inputs and be sure of their BMI.

However, I believe that the code and site were very clean and well-thought-out. The validation ensures that all user inputs conform to the rules of each input field. Also, the suggestion of valid values close to the invalid inputs was a nice touch. Excellent work!

scoutperry commented 3 years ago

https://github.com/karnaout/e15/tree/main/p2 http://e15p2.khaledarnaout.me

I entered a new "Object," and received a dump of the new entry without the site redirecting me to either the new object form page or to the objects page. Nice job on the form validation. Also the find object form works well, so this fulfills the assignment.

Separation of concerns was followed; there was no non-display logic in views, and no display logic in the controllers.

I think it's impressive that the app contains two forms. There's a lot of content here.

Similar to our pactice app bookmark, this app has this code:

        $objectData = file_get_contents(database_path('objects.json'));
        $objects = json_decode($objectData, true);

used in multiple functions in the ObjectController. Maybe a way to write it as a variable within the controller but outside a function and have the various functions that need it call the variable?

Code is clear and includes helpful comments to identify various sections.

It could be interesting to see how the search function could take in inputs other than the "slug" input type. Good Job!

kerynegan commented 3 years ago

Questions

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

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

Are you able to produce any errors or unexpected results when using the site? The only problem that I saw was that my data clears on certain validation checks. If I enter a letter in a number field, that's ok, it throws up the alert without refreshing,

image

but if I skip a field, the page refreshes and my selections are gone.

image

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? Looks good to me!

Are there any parts of the code that you found interesting or taught you something new? The date/time calculations in php are new to me, and were super interesting to explore! I fell down a rabbit-hole of documentation while writing this trying to parse those fields.

Also, I found this super interesting: If I type an 'e' in the number field, it pops an expected validation alert ("please submit a number") if the e is the first character (e101), but if I type, say 10e1 (in notation should equal 100), I don't get the first (please submit a number alert) alert, but instead get the "The video length must be between 1 and 9999 digits" alert, which I thought was interesting!

Are there any parts of the code that you don't understand? As I said, the time functions! But I understand them better now! 🙂

Are there any parts of the code that you feel could be written in a more efficient or clearer manner? Not much! The only thing I probably would have done slightly differently (and this is nitpicky - it works fine as written!!): This bit in the PageController is pretty clear:

#calculate the video length
$totalTime = (($videoLength * $numberEpisodes) - ($skipOpening + $skipEnding)) + ($break * $numberEpisodes);
$hours = floor($totalTime/60); //used floor() to round down
$hoursMinutes = $hours * 60;
$minutes = $totalTime - $hoursMinutes;

but probably could have been shortened a little bit with modulo (%):

#calculate the video length
$totalTime = (($videoLength * $numberEpisodes) - ($skipOpening + $skipEnding)) + ($break * $numberEpisodes);
$hours = floor($totalTime/60); //used floor() to round down
$minutes = $totalTime % 60;

But again, just nitpicky here.

Are there aspects of the code that you feel were not self-evident and would benefit from comments? No, I thought all of it made sense!

Are there any best practices discussed in course material that you feel were not addressed in the code? Only the validation errors clearing my inputs as mentioned above.

Do you have any additional comments not covered in the above questions? Just one about the code, at this line (again) in the Page Controller

#calculate the video length
$totalTime = (($videoLength * $numberEpisodes) - ($skipOpening + $skipEnding)) + ($break * $numberEpisodes);

For an accurate count, I think the break bit at the end of the line should be maybe instead: 

#calculate the video length
$totalTime = (($videoLength * $numberEpisodes) - ($skipOpening + $skipEnding)) + ($break * ($numberEpisodes-1));

Because I think that we wouldn't want to count the break minutes after the last episode, just the ones between episodes?

That said, I thought this project was really, really fun, and I especially loved your little tv logo. I wonder if you could integrate, say, IMDB's API, so that users could search a show name and get the full length with real values for how long each episode is and how many episodes there are, to get accurate counts instead of just estimations? But all in all, great job! 👍

caroput commented 3 years ago

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

https://github.com/abirray-harvard/e15/

there are two folders, bookmark and p2, where the code is split up

• What is the production URL of the project you are reviewing? http://18.224.107.7/index http://e15p2.abir.website/

• 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?

In the index.blade.php, lines 43-75, there are a lot of echo statements as well as routes. This should have been moved to the controller page since you are getting the form data. The SubmissionController was missing a lot of methods

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

I thought 'name' => 'required|regex:/^[a-z\s]*$/i', was an interesting way of only getting alphanumeric with spaces and hyphens for the name.

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

Originally I was a bit confused because Abir put this code in his bookmarks folder some of his bookmarks code is still in here.

There are two indexes. One is here: https://github.com/abirray-harvard/e15/tree/main/bookmark/resources/views The other is here: https://github.com/abirray-harvard/e15/blob/main/p2/index.php

I don’t think this page http://e15p2.abir.website/ is necessary either where I need to click on ‘order now’ to be taken to the order page.

Not sure why they were separated.

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

` <?php echo Form::open(array('route' => 'process')); 

                echo '<p>';
                echo Form::radio('vaccine', 'pfizer', array('id' => 'vaccine-0'));
                echo Form::label('vaccine-0', 'Pfizer-BioNTech');
                echo '</p>';
                echo '<p>';
                echo Form::radio('vaccine', 'johnson', array('id' => 'vaccine-1'));
                echo Form::label('vaccine-1', 'Johnson & Johnson');
                echo '</p>';
                echo '<p>';
                echo Form::radio('vaccine', 'moderna', array('id' => 'vaccine-2'));
                echo Form::label('vaccine-2', 'Moderna');
                echo '</p>';
                echo '<p>';
                echo Form::label('Quantity: ');
                echo Form::selectRange('quantity', 1, 5);
                echo '</p>';
                echo Form::label('Contact Name: ');
                echo Form::text('name', 'John Doe');
                echo '<br>';
                echo Form::label('Contact Number: ');
                echo Form::text('number', 1234567890);

                echo '<br>';    
                echo Form::submit('Order!');
            echo Form::close();
        ?>`

The echo statements in the index.blade.php, lines 43-75, could have been simplified if blade syntax was used.

the information about the three different vaccines, their prices, and the types were repeated in three different places. You could have created a method/ blade table that would create it for you instead of having to write it out every time.

• Are there aspects of the code that you feel were not self-evident and would benefit from comments? in index.blade.php

It seemed pretty clear.

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

On the main index page, it has the wrong link. https://github.com/abirray-harvard/e15/blob/main/p2/index.php lines 42-44

<div class="row text-center">

<a href="http://e15bookmark.loc/index">Order now</a>

</div>

It takes you to e15bookmark.loc/index, which is the local server, which is why if you click on it, you get sent to an ip address instead of the correct url name.

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

Other than the main index page, every other page of the website takes me to another ip address.

cgiansante commented 3 years ago

GitHub - caronee/p2

Application - e15p2.caroli.me

sneadthaman commented 3 years ago