Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General "janitorial" fixes #6

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

General "janitorial" fixes #6

wants to merge 11 commits into from

Conversation

rbrito
Copy link

@rbrito rbrito commented Jul 18, 2015

Hi, @esangel.

As mentioned in the forums, here are some "janitorial" changes to the code. I tried to make each change relatively independend of the others (which is the reason why there is a good amount of them), in the spirit of "Keep It Simple".

Hint: do not try to look at the changes all at once, but each commit in separation of the others, so that you are not overwhelmed with a lot of code changes in just one go.

If you like these changes, then there are more to come, with some of them trying to avoid repetitive code (e.g., the code to sum and to subtract vectors is almost the same and I think that it could be unified in a simple way to avoid repetition).

Anyway, first, let me know what you think of these changes, and then we can proceed to improve the rest of the code.

Thanks,

Rogério Brito.

Rogério Brito added 11 commits July 18, 2015 17:28
Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
In JavaScript, differently from C/C++, varibles are function-scoped instead
of block-scoped.

This patch unifies the declaration of the same variables so that static
analyzers don't complain about the code.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
I missed this one in the previous commit.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
If we have an `if`-`else` command where we never continue the execution from
the `if` block, then we can be free to unwrap the `else` body from the else.

This improves readability by reducing one level of indentation.

Note that there was one instance of this removal where the body of the
`else` was not even indented, which made reading the code confusing
(especially for those that program in languages like Python where
indentation matters).

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
This fixes some messages that are shown to the user.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
While JavaScript implicitly "inserts" missing semicolons under some
conditions, it is better to make them explicitly, especially if we ever have
the hope of running the code under tools like JSLint.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
This was automated with the `dos2unix` tool and while it may look enourmous,
it *only* changes the EOL characters.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
Some parts of the code had funky and inconsistent indentation.  This change
is trivial in the sense that it was automated by Emacs's indent-region
command.

This patch tries to make the indentation consistent with the standard of 4
spaces per level.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
We may want to use an automatic indenter in the near future to take care of
some other inconsistencies in the code.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
@esangel
Copy link
Owner

esangel commented Jul 18, 2015

Thanks. I’ll get to them at some point soon. Since they are janitorial, there is no rush. If there are some that are causing problems now, please let me know.

I should put everything under GitHub first though.


Ed Angel

Founding Director, Art, Research, Technology and Science Laboratory (ARTS Lab)
Professor Emeritus of Computer Science, University of New Mexico

1017 Sierra Pinon
Santa Fe, NM 87501
505-984-0136 (home) angel@cs.unm.edu mailto:angel@cs.unm.edu
505-453-4944 (cell) http://www.cs.unm.edu/~angel http://www.cs.unm.edu/~angel

On Jul 18, 2015, at 3:26 PM, Rogério Brito notifications@github.com wrote:

Hi, @esangel https://github.com/esangel.

As mentioned in the forums, here are some "janitorial" changes to the code. I tried to make each change relatively independend of the others (which is the reason why there is a good amount of them), in the spirit of "Keep It Simple".

Hint: do not try to look at the changes all at once, but each commit in separation of the others, so that you are not overwhelmed with a lot of code changes in just one go.

If you like these changes, then there are more to come, with some of them trying to avoid repetitive code (e.g., the code to sum and to subtract vectors is almost the same and I think that it could be unified in a simple way to avoid repetition).

Anyway, first, let me know what you think of these changes, and then we can proceed to improve the rest of the code.

Thanks,

Rogério Brito.

You can view, comment on, or merge this pull request online at:

#6 #6
Commit Summary

Remove executable bits from non-executable files.
Common: Avoid variable redeclarations in same scope.
Common: Avoid another variable redeclaration.
Common: Remove else's after if's that stop function executions.
Common: Fix spelling and mistakes in user visible messages.
Common/webgl-utils.js: Add missing semicolon after assignment.
Common: Remove pesky trailing whitespaces.
Common/webgl-utils.js: Trivial conversion of DOS to Unix line ends.
Common: Fix indentation.
File Changes

T Chap10/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-0 (0)
T Chap10/mandelbrot1.html https://github.com/esangel/WebGL/pull/6/files#diff-1 (0)
T Chap10/mandelbrot1.js https://github.com/esangel/WebGL/pull/6/files#diff-2 (0)
T Chap10/mandelbrot2.html https://github.com/esangel/WebGL/pull/6/files#diff-3 (0)
T Chap10/mandelbrot2.js https://github.com/esangel/WebGL/pull/6/files#diff-4 (0)
T Chap10/particleSystem.html https://github.com/esangel/WebGL/pull/6/files#diff-5 (0)
T Chap10/particleSystem.js https://github.com/esangel/WebGL/pull/6/files#diff-6 (0)
T Chap11/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-7 (0)
T Chap11/patches.js https://github.com/esangel/WebGL/pull/6/files#diff-8 (0)
T Chap11/teapot1.html https://github.com/esangel/WebGL/pull/6/files#diff-9 (0)
T Chap11/teapot1.js https://github.com/esangel/WebGL/pull/6/files#diff-10 (0)
T Chap11/teapot2.html https://github.com/esangel/WebGL/pull/6/files#diff-11 (0)
T Chap11/teapot2.js https://github.com/esangel/WebGL/pull/6/files#diff-12 (0)
T Chap11/teapot3.html https://github.com/esangel/WebGL/pull/6/files#diff-13 (0)
T Chap11/teapot3.js https://github.com/esangel/WebGL/pull/6/files#diff-14 (0)
T Chap11/teapot4.html https://github.com/esangel/WebGL/pull/6/files#diff-15 (0)
T Chap11/teapot4.js https://github.com/esangel/WebGL/pull/6/files#diff-16 (0)
T Chap11/teapot5.html https://github.com/esangel/WebGL/pull/6/files#diff-17 (0)
T Chap11/teapot5.js https://github.com/esangel/WebGL/pull/6/files#diff-18 (0)
T Chap11/teapot6.html https://github.com/esangel/WebGL/pull/6/files#diff-19 (0)
T Chap11/teapot7.html https://github.com/esangel/WebGL/pull/6/files#diff-20 (0)
T Chap11/vertices.js https://github.com/esangel/WebGL/pull/6/files#diff-21 (0)
T Chap2/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-22 (0)
T Chap2/gasket1.html https://github.com/esangel/WebGL/pull/6/files#diff-23 (0)
T Chap2/gasket1.js https://github.com/esangel/WebGL/pull/6/files#diff-24 (0)
T Chap2/gasket1v2.html https://github.com/esangel/WebGL/pull/6/files#diff-25 (0)
T Chap2/gasket1v2.js https://github.com/esangel/WebGL/pull/6/files#diff-26 (0)
T Chap2/gasket2.html https://github.com/esangel/WebGL/pull/6/files#diff-27 (0)
T Chap2/gasket2.js https://github.com/esangel/WebGL/pull/6/files#diff-28 (0)
T Chap2/gasket3.html https://github.com/esangel/WebGL/pull/6/files#diff-29 (0)
T Chap2/gasket3.js https://github.com/esangel/WebGL/pull/6/files#diff-30 (0)
T Chap2/gasket4.html https://github.com/esangel/WebGL/pull/6/files#diff-31 (0)
T Chap2/gasket4.js https://github.com/esangel/WebGL/pull/6/files#diff-32 (0)
T Chap2/shaders/fshader21.glsl https://github.com/esangel/WebGL/pull/6/files#diff-33 (0)
T Chap2/shaders/vshader21.glsl https://github.com/esangel/WebGL/pull/6/files#diff-34 (0)
T Chap3/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-35 (0)
T Chap3/cad1.html https://github.com/esangel/WebGL/pull/6/files#diff-36 (0)
T Chap3/cad1.js https://github.com/esangel/WebGL/pull/6/files#diff-37 (0)
T Chap3/cad2.html https://github.com/esangel/WebGL/pull/6/files#diff-38 (0)
T Chap3/cad2.js https://github.com/esangel/WebGL/pull/6/files#diff-39 (0)
T Chap3/gasket5.html https://github.com/esangel/WebGL/pull/6/files#diff-40 (0)
T Chap3/gasket5.js https://github.com/esangel/WebGL/pull/6/files#diff-41 (0)
T Chap3/square.html https://github.com/esangel/WebGL/pull/6/files#diff-42 (0)
T Chap3/square.js https://github.com/esangel/WebGL/pull/6/files#diff-43 (0)
T Chap3/squarem.html https://github.com/esangel/WebGL/pull/6/files#diff-44 (0)
T Chap3/squarem.js https://github.com/esangel/WebGL/pull/6/files#diff-45 (0)
T Chap3/triangle.html https://github.com/esangel/WebGL/pull/6/files#diff-46 (0)
T Chap3/triangle.js https://github.com/esangel/WebGL/pull/6/files#diff-47 (0)
T Chap4/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-48 (0)
T Chap4/cube.html https://github.com/esangel/WebGL/pull/6/files#diff-49 (0)
T Chap4/cube.js https://github.com/esangel/WebGL/pull/6/files#diff-50 (0)
T Chap4/cubeq.html https://github.com/esangel/WebGL/pull/6/files#diff-51 (0)
T Chap4/cubeq.js https://github.com/esangel/WebGL/pull/6/files#diff-52 (0)
T Chap4/cubev.html https://github.com/esangel/WebGL/pull/6/files#diff-53 (0)
T Chap4/cubev.js https://github.com/esangel/WebGL/pull/6/files#diff-54 (0)
T Chap4/trackball.html https://github.com/esangel/WebGL/pull/6/files#diff-55 (0)
T Chap4/trackball.js https://github.com/esangel/WebGL/pull/6/files#diff-56 (0)
T Chap4/trackballQuaterion.html https://github.com/esangel/WebGL/pull/6/files#diff-57 (0)
T Chap4/trackballQuaterion.js https://github.com/esangel/WebGL/pull/6/files#diff-58 (0)
T Chap5/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-59 (0)
T Chap5/hat.html https://github.com/esangel/WebGL/pull/6/files#diff-60 (0)
T Chap5/hat.js https://github.com/esangel/WebGL/pull/6/files#diff-61 (0)
T Chap5/hata.html https://github.com/esangel/WebGL/pull/6/files#diff-62 (0)
T Chap5/hata.js https://github.com/esangel/WebGL/pull/6/files#diff-63 (0)
T Chap5/ortho.html https://github.com/esangel/WebGL/pull/6/files#diff-64 (0)
T Chap5/ortho.js https://github.com/esangel/WebGL/pull/6/files#diff-65 (0)
T Chap5/ortho1.html https://github.com/esangel/WebGL/pull/6/files#diff-66 (0)
T Chap5/ortho1.js https://github.com/esangel/WebGL/pull/6/files#diff-67 (0)
T Chap5/ortho2.html https://github.com/esangel/WebGL/pull/6/files#diff-68 (0)
T Chap5/ortho2.js https://github.com/esangel/WebGL/pull/6/files#diff-69 (0)
T Chap5/perspective.html https://github.com/esangel/WebGL/pull/6/files#diff-70 (0)
T Chap5/perspective.js https://github.com/esangel/WebGL/pull/6/files#diff-71 (0)
T Chap5/perspective1.html https://github.com/esangel/WebGL/pull/6/files#diff-72 (0)
T Chap5/perspective1.js https://github.com/esangel/WebGL/pull/6/files#diff-73 (0)
T Chap5/perspective2.html https://github.com/esangel/WebGL/pull/6/files#diff-74 (0)
T Chap5/perspective2.js https://github.com/esangel/WebGL/pull/6/files#diff-75 (0)
T Chap5/shadow.html https://github.com/esangel/WebGL/pull/6/files#diff-76 (0)
T Chap5/shadow.js https://github.com/esangel/WebGL/pull/6/files#diff-77 (0)
T Chap6/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-78 (0)
T Chap6/shadedCube.html https://github.com/esangel/WebGL/pull/6/files#diff-79 (0)
T Chap6/shadedCube.js https://github.com/esangel/WebGL/pull/6/files#diff-80 (0)
T Chap6/shadedSphere1.html https://github.com/esangel/WebGL/pull/6/files#diff-81 (0)
T Chap6/shadedSphere1.js https://github.com/esangel/WebGL/pull/6/files#diff-82 (0)
T Chap6/shadedSphere2.html https://github.com/esangel/WebGL/pull/6/files#diff-83 (0)
T Chap6/shadedSphere2.js https://github.com/esangel/WebGL/pull/6/files#diff-84 (0)
T Chap6/shadedSphere3.html https://github.com/esangel/WebGL/pull/6/files#diff-85 (0)
T Chap6/shadedSphere3.js https://github.com/esangel/WebGL/pull/6/files#diff-86 (0)
T Chap6/shadedSphere4.html https://github.com/esangel/WebGL/pull/6/files#diff-87 (0)
T Chap6/shadedSphere4.js https://github.com/esangel/WebGL/pull/6/files#diff-88 (0)
T Chap6/shadedSphereEyeSpace.html https://github.com/esangel/WebGL/pull/6/files#diff-89 (0)
T Chap6/shadedSphereEyeSpace.js https://github.com/esangel/WebGL/pull/6/files#diff-90 (0)
T Chap6/shadedSphereObjectSpace.html https://github.com/esangel/WebGL/pull/6/files#diff-91 (0)
T Chap6/shadedSphereObjectSpace.js https://github.com/esangel/WebGL/pull/6/files#diff-92 (0)
T Chap6/wireSphere.html https://github.com/esangel/WebGL/pull/6/files#diff-93 (0)
T Chap6/wireSphere.js https://github.com/esangel/WebGL/pull/6/files#diff-94 (0)
T Chap7/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-95 (0)
T Chap7/SA2011_black.gif https://github.com/esangel/WebGL/pull/6/files#diff-96 (0)
T Chap7/bumpMap.html https://github.com/esangel/WebGL/pull/6/files#diff-97 (0)
T Chap7/bumpMap.js https://github.com/esangel/WebGL/pull/6/files#diff-98 (0)
T Chap7/bumpMap2.html https://github.com/esangel/WebGL/pull/6/files#diff-99 (0)
T Chap7/bumpMap2.js https://github.com/esangel/WebGL/pull/6/files#diff-100 (0)
T Chap7/cubet.html https://github.com/esangel/WebGL/pull/6/files#diff-101 (0)
T Chap7/cubet.js https://github.com/esangel/WebGL/pull/6/files#diff-102 (0)
T Chap7/hatImage1.html https://github.com/esangel/WebGL/pull/6/files#diff-103 (0)
T Chap7/hatImage1.js https://github.com/esangel/WebGL/pull/6/files#diff-104 (0)
T Chap7/hatImage2.html https://github.com/esangel/WebGL/pull/6/files#diff-105 (0)
T Chap7/hatImage2.js https://github.com/esangel/WebGL/pull/6/files#diff-106 (0)
T Chap7/hawaiiImage.html https://github.com/esangel/WebGL/pull/6/files#diff-107 (0)
T Chap7/hawaiiImage.js https://github.com/esangel/WebGL/pull/6/files#diff-108 (0)
T Chap7/hawaiiIsobel.html https://github.com/esangel/WebGL/pull/6/files#diff-109 (0)
T Chap7/hawaiisobel.js https://github.com/esangel/WebGL/pull/6/files#diff-110 (0)
T Chap7/honolulu256.js https://github.com/esangel/WebGL/pull/6/files#diff-111 (0)
T Chap7/honolulu4.js https://github.com/esangel/WebGL/pull/6/files#diff-112 (0)
T Chap7/pickCube.html https://github.com/esangel/WebGL/pull/6/files#diff-113 (0)
T Chap7/pickCube.js https://github.com/esangel/WebGL/pull/6/files#diff-114 (0)
T Chap7/pickCube2.html https://github.com/esangel/WebGL/pull/6/files#diff-115 (0)
T Chap7/pickCube2.js https://github.com/esangel/WebGL/pull/6/files#diff-116 (0)
T Chap7/pickCube3.html https://github.com/esangel/WebGL/pull/6/files#diff-117 (0)
T Chap7/pickCube3.js https://github.com/esangel/WebGL/pull/6/files#diff-118 (0)
T Chap7/pickCube4.html https://github.com/esangel/WebGL/pull/6/files#diff-119 (0)
T Chap7/pickCube4.js https://github.com/esangel/WebGL/pull/6/files#diff-120 (0)
T Chap7/pickCube5.html https://github.com/esangel/WebGL/pull/6/files#diff-121 (0)
T Chap7/pickCube5.js https://github.com/esangel/WebGL/pull/6/files#diff-122 (0)
T Chap7/reflectiingCube.html https://github.com/esangel/WebGL/pull/6/files#diff-123 (0)
T Chap7/reflectingCube.js https://github.com/esangel/WebGL/pull/6/files#diff-124 (0)
T Chap7/reflectingSphere.html https://github.com/esangel/WebGL/pull/6/files#diff-125 (0)
T Chap7/reflectingSphere.js https://github.com/esangel/WebGL/pull/6/files#diff-126 (0)
T Chap7/reflectionMap.html https://github.com/esangel/WebGL/pull/6/files#diff-127 (0)
T Chap7/reflectionMap.js https://github.com/esangel/WebGL/pull/6/files#diff-128 (0)
T Chap7/reflectionMap2.html https://github.com/esangel/WebGL/pull/6/files#diff-129 (0)
T Chap7/reflectionMap2.js https://github.com/esangel/WebGL/pull/6/files#diff-130 (0)
T Chap7/render1.html https://github.com/esangel/WebGL/pull/6/files#diff-131 (0)
T Chap7/render1.js https://github.com/esangel/WebGL/pull/6/files#diff-132 (0)
T Chap7/render1v2.html https://github.com/esangel/WebGL/pull/6/files#diff-133 (0)
T Chap7/render1v2.js https://github.com/esangel/WebGL/pull/6/files#diff-134 (0)
T Chap7/render3.html https://github.com/esangel/WebGL/pull/6/files#diff-135 (0)
T Chap7/render3.js https://github.com/esangel/WebGL/pull/6/files#diff-136 (0)
T Chap7/render4.html https://github.com/esangel/WebGL/pull/6/files#diff-137 (0)
T Chap7/render4.js https://github.com/esangel/WebGL/pull/6/files#diff-138 (0)
T Chap7/render5.html https://github.com/esangel/WebGL/pull/6/files#diff-139 (0)
T Chap7/render5.js https://github.com/esangel/WebGL/pull/6/files#diff-140 (0)
T Chap7/textureCube1.html https://github.com/esangel/WebGL/pull/6/files#diff-141 (0)
T Chap7/textureCube1.js https://github.com/esangel/WebGL/pull/6/files#diff-142 (0)
T Chap7/textureCubev2.html https://github.com/esangel/WebGL/pull/6/files#diff-143 (0)
T Chap7/textureCubev2.js https://github.com/esangel/WebGL/pull/6/files#diff-144 (0)
T Chap7/textureCubev3.html https://github.com/esangel/WebGL/pull/6/files#diff-145 (0)
T Chap7/textureCubev3.js https://github.com/esangel/WebGL/pull/6/files#diff-146 (0)
T Chap7/textureCubev4.html https://github.com/esangel/WebGL/pull/6/files#diff-147 (0)
T Chap7/textureCubev4.js https://github.com/esangel/WebGL/pull/6/files#diff-148 (0)
T Chap7/textureSquare.html https://github.com/esangel/WebGL/pull/6/files#diff-149 (0)
T Chap7/textureSquare.js https://github.com/esangel/WebGL/pull/6/files#diff-150 (0)
T Chap9/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-151 (0)
T Chap9/figure.html https://github.com/esangel/WebGL/pull/6/files#diff-152 (0)
T Chap9/figure.js https://github.com/esangel/WebGL/pull/6/files#diff-153 (0)
T Chap9/robotArm.html https://github.com/esangel/WebGL/pull/6/files#diff-154 (0)
T Chap9/robotArm.js https://github.com/esangel/WebGL/pull/6/files#diff-155 (0)
M Common/MV.js https://github.com/esangel/WebGL/pull/6/files#diff-156 (389)
T Common/README.txt https://github.com/esangel/WebGL/pull/6/files#diff-157 (0)
M Common/initShaders.js https://github.com/esangel/WebGL/pull/6/files#diff-158 (47)
M Common/initShaders2.js https://github.com/esangel/WebGL/pull/6/files#diff-159 (70)
M Common/webgl-utils.js https://github.com/esangel/WebGL/pull/6/files#diff-160 (326)
Patch Links:

https://github.com/esangel/WebGL/pull/6.patch https://github.com/esangel/WebGL/pull/6.patch
https://github.com/esangel/WebGL/pull/6.diff https://github.com/esangel/WebGL/pull/6.diff

Reply to this email directly or view it on GitHub #6.

@AntonKhorev
Copy link
Contributor

Removing elses and forcing line breaks after if/for are purely stylistic changes. In my opinion they don't improve anything.

@esangel
Copy link
Owner

esangel commented Jul 19, 2015

I agree. I won’t get them done until after the course unless I can get my student to do them.


Ed Angel

Founding Director, Art, Research, Technology and Science Laboratory (ARTS Lab)
Professor Emeritus of Computer Science, University of New Mexico

1017 Sierra Pinon
Santa Fe, NM 87501
505-984-0136 (home) angel@cs.unm.edu mailto:angel@cs.unm.edu
505-453-4944 (cell) http://www.cs.unm.edu/~angel http://www.cs.unm.edu/~angel

On Jul 19, 2015, at 10:30 AM, Anton Khorev notifications@github.com wrote:

Removing elses rbrito@25f0931 and forcing line breaks after if/for rbrito@da83565 are purely stylistic changes. In my opinion they don't improve anything.


Reply to this email directly or view it on GitHub #6 (comment).

@rbrito
Copy link
Author

rbrito commented Jul 20, 2015

Hi, Anton.

On Jul 19 2015, Anton Khorev wrote:

Removing
elses

The main reason for that is to reduce the indentation levels.

and forcing line breaks after
if/for

The main reason for that is we have nested loops and it breaking the lines
was just to make it clear that we have nested loops.

are purely stylistic changes.

Yes, indeed, they are.

In my opinion they don't improve anything.

In my opinion, they do. Who wins? :)

Kidding aside, they improve the style of the code and that is very important
(though not as important as the code being correct).

In the words of a very important Number Theorist (G.H. Hardy):

"The mathematician’s patterns, like the painter’s or the poet’s must be
beautiful; the ideas like the colours or the words, must fit together in
a harmonious way. Beauty is the first test: there is no permanent place
in the world for ugly mathematics."

Similarly to Prof. Hardy, I also see that computer code (especially that
which is used for teaching purposes) should be as beautiful and elegant as
possible. And I am proposing some "frivolous" changes that I think that do
improve the code.

Anyway, by reading the code doing such "frivolous" changes to improve style
and fix small things, I have saw some flaws of the code (e.g., some
functions that don't handle reasonable inputs) and I would send those more
substantial changes in a future pull request.

Anyway #2, I can revert those particular changes if there is intention of
using the rest of this pull request.

Anyway #3, I can simply withdraw the pull request at all and use a forked
version of the code. It is MIT licensed, anyway. :)

Regards,

Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://cynic.cc/blog/ : github.com/rbrito : profiles.google.com/rbrito
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br

@esangel
Copy link
Owner

esangel commented Jul 20, 2015

Since the code goes out to lots of users of my textbook, I do want to clean up the code but it may take me awhile to get to it since the Coursera course is taking a lot of time and SIGGRAPH is coming up in a couple of weeks.

As for the Common code, some of the webgl utilities are not mine so I would rather look for an updated version on the web at either mozilla or google. MV.js needs some serious work. Most of it was done by my coauthor quite awhile ago while we were doing the first draft of the book and we never really cleaned it up. There are some inconsistencies on things like error checking I need to go over. Also I need to add a lot more comments to many of the examples.

I really appreciate both of your feedback.

Ed


Ed Angel

Founding Director, Art, Research, Technology and Science Laboratory (ARTS Lab)
Professor Emeritus of Computer Science, University of New Mexico

1017 Sierra Pinon
Santa Fe, NM 87501
505-984-0136 (home) angel@cs.unm.edu mailto:angel@cs.unm.edu
505-453-4944 (cell) http://www.cs.unm.edu/~angel http://www.cs.unm.edu/~angel

On Jul 20, 2015, at 2:23 PM, Rogério Brito notifications@github.com wrote:

Hi, Anton.

On Jul 19 2015, Anton Khorev wrote:

Removing
elses

The main reason for that is to reduce the indentation levels.

and forcing line breaks after
if/for

The main reason for that is we have nested loops and it breaking the lines
was just to make it clear that we have nested loops.

are purely stylistic changes.

Yes, indeed, they are.

In my opinion they don't improve anything.

In my opinion, they do. Who wins? :)

Kidding aside, they improve the style of the code and that is very important
(though not as important as the code being correct).

In the words of a very important Number Theorist (G.H. Hardy):

"The mathematician’s patterns, like the painter’s or the poet’s must be
beautiful; the ideas like the colours or the words, must fit together in
a harmonious way. Beauty is the first test: there is no permanent place
in the world for ugly mathematics."

Similarly to Prof. Hardy, I also see that computer code (especially that
which is used for teaching purposes) should be as beautiful and elegant as
possible. And I am proposing some "frivolous" changes that I think that do
improve the code.

Anyway, by reading the code doing such "frivolous" changes to improve style
and fix small things, I have saw some flaws of the code (e.g., some
functions that don't handle reasonable inputs) and I would send those more
substantial changes in a future pull request.

Anyway #2, I can revert those particular changes if there is intention of
using the rest of this pull request.

Anyway #3, I can simply withdraw the pull request at all and use a forked
version of the code. It is MIT licensed, anyway. :)

Regards,

Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://cynic.cc/blog/ : github.com/rbrito : profiles.google.com/rbrito
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br

Reply to this email directly or view it on GitHub #6 (comment).

@AntonKhorev
Copy link
Contributor

When I have a function with two branches and both of them return something, I don't consider it an improvement to unindent one branch. It makes comparing the code of the branches more difficult and it doesn't really save you any horizontal space because there's that other branch. Additionally, you can't fold the unindented branch in editors that support code folding.

Of course I would have written the code differently. Of course I can write a longer post arguing that my way is better and maybe reference some famous person.

@rbrito
Copy link
Author

rbrito commented Aug 30, 2017

@esangel, are there any commits here that you think are worth cherry-picking? If not, I will withdraw my pull request completely.

Otherwise, I can rebase the changes on top of your master branch...

I hope that you do find something worthwhile, since the code base could certainly be improved (like, for example removing the executable bits from the source files, removing .DS_Store crap, the Git.zip file and so on, just to name a few things).

@esangel
Copy link
Owner

esangel commented Sep 1, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants