Euclid algorithm in bash #497

Ariana1729 · 2018-10-12T00:35:29Z

umm quite useless but slightly better than writing in bf haha, also displayed nicely on gitbook too:>

jiegillet · 2018-10-12T00:36:25Z

contents/thomas_algorithm/thomas_algorithm.md

@@ -1,4 +1,4 @@
-# Thomas Algorithm
+s# Thomas Algorithm


you typed an s here

Wait this file should not be here at all!

oops got to undo commits

jiegillet · 2018-10-12T00:42:02Z

contents/thomas_algorithm/thomas_algorithm.md

@@ -111,8 +111,6 @@ You will find this algorithm implemented [in this project](https://scratch.mit.e
 [import, lang:"java"](code/java/thomas.java)
 {% sample lang="hs" %}
 [import, lang:"haskell"](code/haskell/thomas.hs)
-{% sample lang="go" %}
-[import, lang:"go"](code/golang/thomas.go)


You're still removing lines, you shouldn't touch thomas_algorithm.md at all

should i use let or $(()), imo let is nicer to look at but still pretty disgusting

leios

I think the code looks pretty clear, but I'll leave it up for a bit in case anyone else wants to review it. Here's just a quick fix.

leios · 2018-10-12T01:08:11Z

book.json

@@ -163,6 +163,10 @@
        {
          "lang": "lolcode",
          "name": "LOLCODE"
+        },
+        {
+          "lang": "bash",


We usually make lang the file extension so sh in this case

berquist

First shell script, you brave soul.

It is very easy to mix them, because everyone does it, but don't write sh constructs when there are newer better bash ones.
I'll submit an EditorConfig PR (which everyone should be using), but bash should use spaces for indentation, not tabs, and I prefer 4 rather than 2 spaces.
All shell scripts should have the shebang at the top:

#!/usr/bin/env bash

berquist · 2018-10-12T01:21:57Z

contents/euclidean_algorithm/code/bash/euclid.sh

@@ -0,0 +1,38 @@
+abs() {
+	local ret=$1
+	if [ $ret -lt 0 ]; then


[ ... ] is sh (the original Bourne shell). Please use [[ ... ]] for consistency.

Though, you can do even better considering the ((...)) syntax (left as an exercise for the reader.

berquist · 2018-10-12T01:28:29Z

contents/euclidean_algorithm/code/bash/euclid.sh

+abs() {
+	local ret=$1
+	if [ $ret -lt 0 ]; then
+		let "ret = $ret * -1"


While we are working with integers, do general arithmetic instead, using more modern Bash:

((ret *= -1))

berquist · 2018-10-12T01:29:58Z

contents/euclidean_algorithm/code/bash/euclid.sh

+		let "b = $a % $b"
+		let "a = tmp"
+	done
+	echo $a


Prefer printf for function returns. The echos at the end of the script are fine.

berquist · 2018-10-12T01:30:50Z

contents/euclidean_algorithm/code/bash/euclid.sh

+	echo $a
+}
+
+result=$(euclid_mod 143 693)


Use the other examples for consistency:

result=$(euclid_mod $((64 * 67)) $((64 * 81)))

etc.

berquist · 2018-10-12T01:39:54Z

Oh, and
4. rename the extension from *.sh to *.bash. This is another terrible habit that everyone has (including me).

Ariana1729 · 2018-10-12T01:44:11Z

all my bash scripts are also .sh haha, ok editing

berquist · 2018-10-12T01:48:12Z

Not specific to the review, and I'll document it elsewhere, but

It is good practice to use ShellCheck on all your shell scripts. Because of their nature, linters/static analysis to help find problems or smells are more important for shell scripts than most programming languages.
A good style guide to follow is https://github.com/bahamas10/bash-style-guide.

leios · 2018-10-12T01:52:23Z

Ok. Yeah, you guys are right. The scripts don't need to be .sh. Sorry for that.

Ariana1729 · 2018-10-12T01:53:17Z

oh wow shellcheck seems quite cool, giong to update code based on that

berquist · 2018-10-12T02:00:57Z

The most common shellcheck error is about quoted variables. While this is the most important thing for daily scripting, it isn't important here at all, so ignore it.

berquist · 2018-10-12T03:17:00Z

You still need to fix the filename.

berquist · 2018-10-12T03:22:52Z

contents/euclidean_algorithm/euclidean_algorithm.md

@@ -59,6 +59,8 @@ The algorithm is a simple way to find the *greatest common divisor* (GCD) of two
 [import:2-17, lang:"emojicode"](code/emojicode/euclidean_algorithm.emojic)
 {% sample lang="lolcode" %}
 [import:25-40, lang="LOLCODE"](code/lolcode/euclid.lol)
+{% sample lang="bash" %}
+[import:10-22, lang="bash"](code/bash/euclid.bash)


You've swapped the subtraction and modulus versions in the text.

berquist · 2018-10-12T03:23:25Z

contents/euclidean_algorithm/code/bash/euclid.sh

+#!/usr/bin/env bash
+abs() {
+    local ret=$1
+    if (( $ret < 0 )); then


No need for $ in any math expression.

Ariana1729 · 2018-10-12T03:34:52Z

Ok should be fixed

berquist · 2018-10-13T01:00:46Z

I can't pick it apart further, thanks!

Ariana1729 added 3 commits October 12, 2018 00:23

Added thomas algo in js

9dcaf93

Removed travis algo(someone else doing)

c5cb003

Added euclid in bash

a23dd47

jiegillet reviewed Oct 12, 2018

View reviewed changes

Fixed random commit typo

91b3cfe

jiegillet reviewed Oct 12, 2018

View reviewed changes

berquist self-assigned this Oct 12, 2018

Ariana1729 added 4 commits October 12, 2018 08:53

Probably slightly nicer

36c8333

should i use let or $(()), imo let is nicer to look at but still pretty disgusting

oops inconsistant '\n's

1ea7343

and umm line numbers

ae6a1c0

undone thomas algo

6435b7f

leios reviewed Oct 12, 2018

View reviewed changes

Updated lang

85dd93a

berquist requested changes Oct 12, 2018

View reviewed changes

Fixed line nos

c198611

berquist added Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) Hacktoberfest The label for all Hacktoberfest related things! labels Oct 12, 2018

Ariana1729 added 2 commits October 12, 2018 10:01

and fixed code

02f514a

more pretty conditionals

f2bdb54

berquist requested changes Oct 12, 2018

View reviewed changes

Ariana1729 added 2 commits October 12, 2018 11:33

Updated file

f3be8ed

Fixed line nos

0508ea2

berquist approved these changes Oct 13, 2018

View reviewed changes

berquist merged commit 594b016 into algorithm-archivists:master Oct 13, 2018

Ariana1729 deleted the euclidalgo_sh branch October 13, 2018 08:43

Ariana1729 mentioned this pull request Oct 15, 2018

Added bubblesort in bash #509

Merged

Uh oh!

Euclid algorithm in bash #497

Euclid algorithm in bash #497

Uh oh!

Conversation

Ariana1729 commented Oct 12, 2018

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!

leios 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!

berquist 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!

berquist commented Oct 12, 2018

Uh oh!

Ariana1729 commented Oct 12, 2018

Uh oh!

berquist commented Oct 12, 2018

Uh oh!

leios commented Oct 12, 2018

Uh oh!

Ariana1729 commented Oct 12, 2018

Uh oh!

berquist commented Oct 12, 2018

Uh oh!

berquist commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ariana1729 commented Oct 12, 2018

Uh oh!

berquist commented Oct 13, 2018

Uh oh!

Uh oh!