Natalia Pavlovic Pull Request #7

NataliaPavlovic · 2018-01-16T04:28:13Z

My Pull Request includes all the changes I made to the PythonWebServer files.

ExtraGravity

Hey congratulations on being the first to submit a pull request! I know it must've taken a lot of effort to learn everything required in this training so definitely kudos to you. There are quite a few things to change but that's ok, by the end of it this pull request will be perfect :)

I'm looking forward to the revisions you make! And just so you know, you don't need to make a new branch and a new pull request, if you just commit on the same branch and push again, this pull request will be updated with all your new changes.

Don't hesitate to comment on this pull request or contact me directly if you have any questions or want clarification, I'm here to help!

Also last note, branch names are supposed to be <your-name>/<informative-name>, so for this branch, a better name would have been NataliaP/training

ExtraGravity · 2018-01-18T20:23:46Z

Training/PythonWebServer/index.html

+    	<br><br>
+
+        <script>
+        function loadDoc() {


In a <script> tag, usually javascript is written in between the tags to run on the page, which is what you've done. But the problem with script tags is that it mixes some different languages in a single file and in general is less organized.

Generally, all javascript goes in the javascript files, in this case index.js. Putting it in the js files runs it the same as if it was in a <script> tag in the html!

And .js files aren't magically loaded! For us, the javascript file was loaded in on line 9 in this HTML file.

<script src="/resources/index.js" type="text/javascript"></script>

ExtraGravity · 2018-01-18T20:33:13Z

Training/PythonWebServer/webserver.py

+       	s3 = str(round(psutil.virtual_memory().total/(2**30), 2));
+        s4 = str(round(psutil.virtual_memory().used/(2**30), 2));
+
+        self.wfile.write(bytes(s0, "utf-8"));


Web is a very high level form of programming and instead of writing a single stream of bytes, there is a much more structured way of passing data around the web (like between a webserver and a web page in POST requests)! This data format is called JSON and it looks like this.

{ "title": "some fruits!", "cost": 5, "fresh": false, "fruits": [ "apples", "bananas", "oranges" ], "owner": { "name": "Enoch", "age": 43 } }

Notice there are things like arrays, numbers, booleans. So it's very structured, the best part is that javascript is very good at parsing JSON! So from a webserver side of things, the best way to respond to a POST request is in JSON format.

Also since JSON is so popular, there's already a python library to format any data we have into JSON format. It's just called json and it can be imported with import json. Here's the official documentation page https://docs.python.org/3/library/json.html.

ExtraGravity · 2018-01-18T20:37:19Z

Training/PythonWebServer/webserver.py

+        s1 = str(psutil.cpu_count());
+        s2 = str(psutil.cpu_percent(interval=1));
+       	s3 = str(round(psutil.virtual_memory().total/(2**30), 2));
+        s4 = str(round(psutil.virtual_memory().used/(2**30), 2));


It's important to pick descriptive variable names, s4 doesn't really tell a reader what kind of data is in the variable. For example, the s4 variable would be better named virtual_memory_used. The other variables in this function could use some more descriptive names too :)

ExtraGravity · 2018-01-18T20:41:27Z

Training/PythonWebServer/index.html

+        <script>
+        function loadDoc() {
+          var xhttp = new XMLHttpRequest();
+          xhttp.onreadystatechange = function() {


All the dollar signs in the .js file indicate that the jQuery library is being used, it's been imported on line 8 of the this HTML file.

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.3/jquery.min.js"></script>

jQuery is a pretty standard library to make alot of things simpler. One thing it does is provide a simplified wrapper around AJAX requests, I would definitely recommend using that.

Here is the documentation for making an AJAX post request with jQuery https://api.jquery.com/jquery.post/.

ExtraGravity · 2018-01-18T20:51:24Z

Training/PythonWebServer/index.html

+              document.getElementById("Time3").innerHTML = this.responseText[2];
+              document.getElementById("Time4").innerHTML = this.responseText[3];
+              document.getElementById("Time5").innerHTML = this.responseText[4];
+              document.getElementById("Time6").innerHTML = this.responseText[5];


So a couple things here.

The jQuery library can simplify this call! The convenient dollar sign provided by jQuery lets us use CSS selectors to select elements. So to select the element with the Time1 id, we can use $("#Time1") (note the # symbol), instead of document.getElementById("Time1").

And the second thing, individual elements aren't required for each character. A single element could be used and then take a substring from the response text.

So for example, both those points together, something could look like:

var fruits = "applebanana"; $("#firstFruit").text(fruits.substring(0, 5));

…d jQuery to make an AJAX post request.

ExtraGravity

Looking good!

ExtraGravity · 2018-02-12T20:01:59Z

Training/PythonWebServer/webserver.py

+	        "Total CPUs": cpu_count,
+	        "CPU Usage": cpu_percent,
+	        "Total RAM": virtual_memory_total,
+	        "Used RAM": virtual_memory_used


These attributes are better named without spaces, something more like a variable like usedRam so that javascript can access it like a object.

data.usedRam

ExtraGravity · 2018-02-12T20:02:42Z

Training/PythonWebServer/psutil

+303 17
+0
+0
+562A2D202433579FC3BFC2C0BC8F5020242D203672A8C3C8C7C29F54292D20242E4689B9C9CBCB


This file probably shouldn't be here, is it being used for anything?

ExtraGravity · 2018-02-12T20:03:28Z

Training/PythonWebServer/webserver.py

+import psutil
+from datetime import datetime
+import pytz
+import json


In general, imports are best in the order

system

third party

internal

With a space separating each section.

ExtraGravity · 2018-02-12T20:03:36Z

Training/PythonWebServer/webserver.py


    def do_POST(self):
-        self.wfile.write("Send your POST response here!")
+        #Send your POST response here!


This comment isn't necessary anymore.

ExtraGravity · 2018-02-12T20:03:47Z

Training/PythonWebServer/webserver.py


 def main():
    port = int(sys.argv[1]) # Error check this!
-    print("Starting server on port " + str(port) + "...")


Didn't like this? xD

ExtraGravity · 2018-02-12T20:14:30Z

Training/PythonWebServer/index.html

 <body>
-    <h1>Hello world!</h1>
+	<div class='wrapper'>
+        <button class="button" onclick="loadDoc()">UPDATE</button>


I think don't think onclick attribute is necessary, since your javascript is already catching it.

ExtraGravity · 2018-02-12T20:15:21Z

Training/PythonWebServer/index.html

+    	<div class="display">
+        Total CPUs: 
+        <p id = "totalCPU"></p>
+        <br> <br>


It's better to use CSS to control spacing compared to using <br> tags, since different platforms/browsers might interpret <br> tags differently.

ExtraGravity · 2018-02-12T20:16:26Z

Training/PythonWebServer/resources/stylesheet.css

+    border: none;
+    padding: 10px 75px;
+    font-size: 40px;
+    display: block;


CSS spacing was a nightmare before, but then came flexbox! I'd highly recommend trying to use flexbox to get all your spacing right.

https://css-tricks.com/snippets/css/a-guide-to-flexbox/

ExtraGravity · 2018-02-12T20:18:59Z

Training/PythonWebServer/resources/stylesheet.css

+	display:inline-block;
+	letter-spacing: -2px;
+
+	margin-top: 3cm;


Generally cm should be avoided, because it comes off very different on different screens.

In order of preference, I use em, rem, px, vh. They all have different purposes (you can read about them yourself) but in general that is the preference because that is the order in how nicely they play on different screen sizes.

ExtraGravity · 2018-02-12T20:20:19Z

Training/PythonWebServer/resources/stylesheet.css

+}
+}
+@media screen and (min-width: 1500px) and (max-width: 1600px){
+	.display{


Spacing is a little weird in this file too!

NataliaP721 added 2 commits January 15, 2018 21:21

Made changes

ed198c9

Completed changes to all files

df532d8

ExtraGravity self-requested a review January 16, 2018 19:24

ExtraGravity self-assigned this Jan 16, 2018

ExtraGravity requested changes Jan 18, 2018

View reviewed changes

Changed variable names to be descriptive, data format to JSON and use…

f144f5b

…d jQuery to make an AJAX post request.

ExtraGravity requested changes Feb 12, 2018

View reviewed changes

Natalia Pavlovic Pull Request #7

Are you sure you want to change the base?

Natalia Pavlovic Pull Request #7

Uh oh!

Conversation

NataliaPavlovic commented Jan 16, 2018

Uh oh!

ExtraGravity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExtraGravity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ExtraGravity left a comment •

edited

Loading