2
\$\begingroup\$

I just started learning OOP from Mosh on YouTube and I looked for easiest exercises to practice. Can I get a feedback on my code please? What I did wrong and what I should change?

In the code there are two exercises: the first one is a temperature converter and the second one is a currency exchange.

let cel = "";
let fah = "";

function Termometer(kel) {
  this.kel = kel;

  this.getCelsius = function() {
    cel = this.kel - 273;
    return cel;
  },

  this.getFahrenheit = function() {
    fah = Math.floor(cel * (9/5) + 32);
    return fah;
  },

  this.init = function() {
    this.getCelsius();
    this.getFahrenheit();
  }
}



// currency exchange
var eur = "";
var usd = "";
var gbp = "";
function Valuta(sum1, sum2, sum3) {
  this.sum1 = sum1;
  this.sum2 = sum2;
  this.sum3 = sum3;

  this.getEur = function() {
    eur = Math.floor(this.sum1 / 4.72);
  };

  this.getUsd = function() {
    usd = Math.floor(this.sum2 / 4.15);
  };

  this.getGbp = function() {
    gbp = Math.floor(this.sum3 / 5.27);
  };

  this.init = function() {
    this.getEur();
    this.getUsd();
    this.getGbp();
  }
}

var valuta = new Valuta(300, 300, 300);
valuta.init();
console.log(eur, usd, gbp)
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Does this work, to the best of your knowledge? Also, please improve the title of your question. \$\endgroup\$ – Null Jun 27 '19 at 17:19
  • \$\begingroup\$ There is an example for calling Valuta but there is no example for calling Termometer. It would also be interesting to know what the base units are for both functions. \$\endgroup\$ – pacmaninbw Jun 28 '19 at 13:08
1
\$\begingroup\$

0. Little Typo

thermometer instead of termometer.

1. Global Variables

let cel = "";
let fah = "";

Variables inside a global scope can be changed from everywhere.

1.1 Example

Imagine temperature.js is your script with the global variables inside. Now someone adds a table.js to display tables on the html page.

<script src="temperature.js"></script>
<script src="table.js"></script>

One line in table.js is cel = "<td>...</td>". The author of the code has accidentally a spelling mistake and writes cel instead of cell to indicate a table cell and breaks with this line your code in temperature.js.

1.2 Quick fix

function Termometer(kelven) {
    this.kelven = kelven;
    this.cel;
    this.fah;

    /* ... */
}

With this fix we binde the variables to Thermometer. To access them you need to create an instance of Thermometer.

const thermometer = new Thermometer(0)
console.log(thermometer.cel)

This means that inside table.js the author needs to write explicite thermometer.cel = ... to break your code.

2. Intuitive API

const thermometer= new Thermometer(0)

thermometer.init()

console.log(thermometer.getCelsius())
console.log(thermometer.getFahrenheit())

To call thermometer.init() feels so wrong and it is not intuitive.

2.1 Quick fix

When new Thermometer(0) gets called, in the background a new instance gets build by the javascript runtime and it initializes all variables inside the instance.

To avoid the init method we can store the calculation in variables:

function Thermometer(kelven) {
    this.kelven = kelven;
    this.celsius = this.kelven - 273
    this.fahrenheit = Math.floor(this.celsius * (9 / 5) + 32)

    this.getCelsius = function() { return this.celsius }
    this.getFahrenheit = function() { return this.fahrenheit }
}

The code snipped above looks more intuitive with ES6-Classes:

class Thermometer {
    constructor(kelven) {
        this.kelven = kelven
        this.celsius = this.kelven - 273
        this.fahrenheit = Math.floor(this.celsius * (9 / 5) + 32)
    }

    getCelsius() { return this.celsius }
    getFahrenheit() { return this.fahrenheit }
}

Now, we can can interact with it like:

const thermometer = new Thermometer(0)

console.log(thermometer.getCelsius())
console.log(thermometer.getFahrenheit())

3. Unflexible

function Thermometer(kel) {
   this.kel = kel

   this.getCelsius = function () {
       cel = this.kel - 273;
       return cel;
   }

   /* ... */
}

Currently the Thermometer only works for kelvin..

It would be great to archive that we can pass in celcius and it still works!

3.1 Common Interface

That this works all Units needs to share a common api that gets called by Thermometer. The Thermometer could call methods or properties like inCelcius, inFahrenheit and inKelvin.

function Thermometer(temperature) {
    this.getCelsius = function () { return temperature.inCelsius }
    this.getFahrenheit = function () { return temperature.inFahrenheit }
    this.getKelvin = function () { return temperature.inKelvin }
}

Based on that we can create our units:

function Kelvin(value) {
    this.inKelvin = value
    this.inCelsius = value - 273
    this.inFahrenheit = Math.floor(this.inCelsius * (9 / 5) + 32)
}

function Fahrenheit(value) {
    this.inKelvin = Math.floor((value + 459) * (9 / 5))
    this.inCelsius = Math.floor((value - 32) / (9 / 5))
    this.inFahrenheit = value
}

function Celsius(value) {
    this.inKelvin = value + 273
    this.inCelsius = value
    this.inFahrenheit = Math.floor(value * (9 / 5) + 32)
}

Now the Thermometer works for all units:

const kelvinMeter = new Thermometer(new Kelvin(0))
const fahrenMeter = new Thermometer(new Fahrenheit(0))
const celsiMeter = new Thermometer(new Celsius(0))

4. Unexpected Behavior

this.getEur = function() {
   eur = Math.floor(this.sum1 / 4.72);
}

When I would call valuta.getEur() I would expect that the method getEur returns a value. Instead it returns undefined and changes a global variable.

const valuta = new Valuta(1, 2, 3)
const euro = valuta.getEur() // undefined
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.