1
\$\begingroup\$

Right now I am finding ways to write better code in javascript and I see many coding styles used by different authors but right now I am thinking to write code which adheres to the object oriented paradigm better suited for this type of language.

Code

// Terminal

'use strict';

class Terminal {
  constructor() {
    this.cols = process.stdout.columns || 80
    this.rows = process.stdout.rows || 60
  }

  width() {
    return this.cols
  }

  height() {
    return this.rows
  }
}

module.exports = Terminal


// Box

'use strict';

const Terminal = require('./terminal')

class Box {
  constructor(opts) {
    let terminal = new Terminal()
    const defaults = {
      w: terminal.width(),
      h: terminal.height(),
      t: '══',
      tr: '╗',
      r: '║',
      br: '╝',
      b: '══',
      bl: '╚',
      l: '║',
      tl: '╔',
      fill: '░░'
    }
    this.settings = Object.assign(defaults, opts)
  }

  toString() {
    const res = [];
    const { t, tr, r, br, b, bl, l, tl } = this.settings
    const { w , h } = this.settings
    // Filling row wise
    for (let i = 0; i < h; i++) {
      for (let j = 0; j < w; j++) {
        if (i === 0 && j === 0) {
          res.push(tl)
        }
        else if (i === 0 && j === w - 1) {
          res.push(tr)
        }
        else if ((i === h - 1) && (j === w - 1)) {
          res.push(br)
        }
        else if ((i === h - 1) && j === 0) {
          res.push(bl)
        }
        else if (this.isFirstRow(i) || this.isLastRow(i)) {
          res.push(t)
        }
        else if (this.isFirstCol(j) || this.isLastCol(j)) {
          res.push(l)
        }
        else {
          const { fill } = this.settings
          res.push(fill)
        }
      }
      res.push('\n')
    }
    return res.join('')
  }

  isFirstRow(row) {
    return row === 0
  }

  isLastRow(row) {
    const { h } = this.settings
    return row === h - 1
  }

  isFirstCol(col) {
    return col === 0
  }

  isLastCol(col) {
    const { w } = this.settings
    return col === w - 1
  }
}

class TopRight {
  constructor() {
    this.symbol = '╗'
  }

  toString() {
    return this.symbol
  }
}

class BottomRight {
  constructor() {
    this.symbol = '╝'
  }

  toString() {
    return this.symbol
  }
}

class BottomLeft {
  constructor() {
    this.symbol = '╚'
  }

  toString() {
    return this.symbol
  }
}

class TopLeft {
  constructor() {
    this.symbol = '╔'
  }

  toString() {
    return this.symbol
  }
}

module.exports = Box

// O/P
/*
> const B = require('./box')
> console.log(new B({w: 8, h: 8, fillSymbol: '||'}).toString())
╔════════════╗
║░░░░░░░░░░░░║
║░░░░░░░░░░░░║
║░░░░░░░░░░░░║
║░░░░░░░░░░░░║
║░░░░░░░░░░░░║
║░░░░░░░░░░░░║
╚════════════╝
*/

I would like to know how to improve the above code to make better use of prototypal inheritance and delegation which is widely advocated in javascript world.

Note

I thought to use classes for printing symbols but I see that they make code more bloated. I left them there hear opinion of others.

\$\endgroup\$
  • 1
    \$\begingroup\$ This seems really over-engineered. It looks like you want a function that, given a width and height, returns a string box of that width and height, filled with some symbol? Is that correct? Because if so, that should be a single function which is 5-10 lines long. \$\endgroup\$ – Jonah Jun 30 '17 at 15:58
  • \$\begingroup\$ @Jonah show me the code. \$\endgroup\$ – CodeYogi Jun 30 '17 at 16:08
5
\$\begingroup\$

This can be order of magnitudes simpler. You want a function that takes a width, height, and optional fill character, and returns a string (your box).

Try this:

function box(w, h, fill='░░') {
  const top    = '╔' + '══'.repeat(w-2) + '╗\n' 
  const row    = '║' + fill.repeat(w-2) + '║\n'
  const bottom = '╚' + '══'.repeat(w-2) + '╝'
  return top + row.repeat(h-2) + bottom
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ And here it goes, you hard coded the symbols. \$\endgroup\$ – CodeYogi Jun 30 '17 at 17:04
  • \$\begingroup\$ Nothing is going anywhere. That's trivial to fix with an optional parameter. This is the reason I asked you what you wanted in the comment of the OP. Are there more requirements than specifying fill and corner symbols? \$\endgroup\$ – Jonah Jun 30 '17 at 17:06
  • \$\begingroup\$ Hi, I now see a serious flaw in my thinking process which is directly jumping into the complex solution in the need to become perfectionist. I think it would be good to try the simplest solution possible first followed by refactoring to make the code more flexible and testable, right? \$\endgroup\$ – CodeYogi Jul 1 '17 at 4:07
  • \$\begingroup\$ Yes, always start with the simplest possible thing. This will 1) clarify your thinking and 2) may be all you end up needing. Re: testing, note that the above is trivially testable. In fact, as a general rule, nothing is easier to test than pure functions: you know what output you expect for given input, and you write tests to assert that, when given the input, your function produces the expected output. When you make objects testable, you typically do so with dependency injection, which is really just a fancy way of passing in input. \$\endgroup\$ – Jonah Jul 1 '17 at 4:11
  • \$\begingroup\$ Yes, and I am really trying hard to learn this process of thinking, can you please suggest me some book or course? and let me tell you this is part of a bigger problem and I am learning to break the problem in smaller pieces as part of the learning process. \$\endgroup\$ – CodeYogi Jul 1 '17 at 4:25
2
\$\begingroup\$

I think you and @jonah just have to meet in the middle. So if you'll both allow me:

  1. Yes, using classes for each printing symbol is definitely too much. As Jonah says, it is better to just let that be an optional parameter and let it be used as a simple string. To put it simply, your TopRight, BottomRight, TopLeft, and BottomLeft classes definitely violate the principle of DRY. If those characters needed more functionality than simply storing the character to be outputted, then I would suggest (in this case), having a single class that had four instances (i.e. local variables) named TopRight, BottomRight, etc.. The only thing different between those classes is the class name and default character: no reason for a separate class as-is.
  2. Jonah's code really is orders of magnitude simpler. Now granted, you are trying to practice proper OOP design, and his example does the opposite (making it all one quick function), but his algorithm is a much more elegant solution. It will have better performance and it is substantially easier to understand. So I would suggest you effectively replace your to_string method with his box method. Granted, you still want to work within your OOP paradigm and replace his hard-coded characters with the settings that come from your object, but his simpler algorithm will do your package a whole lot of good.
  3. Here's your biggest problem from an OOP design principle:

let terminal = new Terminal()

This is a clear violation of the 'D' in solid, the dependency inversion principle. What if the developer is operating within a non-terminal interface, but still wants to use your code? How are you going to test the default behavior of your system in your testing environment? You are always going to get this terminal object, whos behavior will be very undetermined in anything other than a console environment. Inversion of control can be a surprisingly large topic to unpack, so this is something best left to google if you are unfamiliar with its principles. Obviously you have other ways for the "user" to set the width and height of the box, but that line is still a very clear violation of IoC.

As a potential solution that might better adhere to the SRP, you could change the constructor argument for your Box class from opts (a simple object) to a settings interface, which has those same defined keys. Then you could build a terminal class which implements the settings interface and loads its width and height out of the console by default and also accepts an optional width, height, characters, etc. Therefore, your calling sequence would look like this:

const B = require('./box')
const Terminal = require('./terminal');
console.log( new B( new Terminal( {w: 8, h:8, fillSymbol: '||'} ) ).toString() )

Those are my thoughts, anyway. Obviously, I've left out some details, but I think the intent should be clear. Obviously javascript doesn't have interfaces, but the same concepts apply.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The problem is this just isn't a good example to practice OOP design on. A utility function is the perfect fit. If you really want to make my design OO, what you'd do have is have a constructor function to which you'd pass w, h, fill (+ other optional args if desired) , which returns a Box object, with methods like toString() (returns what my example does), width(), etc. Having classes like TopRight, BottomLeft, etc, is just terrible design. My intention here is not to be mean, but it's important OP realize this, and sugar-coating it isn't doing him any favors. \$\endgroup\$ – Jonah Jun 30 '17 at 18:28
  • 1
    \$\begingroup\$ I agree: this is easily a utility function without need for any fuss. However, you can try to apply OOP principles to anything, and simpler things are easier to break down, so it isn't a crazy question to ask. I don't have any problem with your answer, and don't think you are trying to be mean. My goal was simply to answer the question as asked, which is why I mentioned pretty much those same points you have here. \$\endgroup\$ – Conor Mancone Jun 30 '17 at 18:44
  • \$\begingroup\$ Yeah, it's tough. I see what you're trying to do. And in theory you're right -- it's a fine question to ask. The problem is, practically speaking, I look at the code and I know the best advice that can possibly be given, if the person is willing to listen, is: Don't. Just stop right there. Do it simply. Do it with functions. You don't need OOP. My current belief is that it's only possible to write decent OO code if you already know FP well and can approach OO with a largely immutable style. But if you know FP that well, you won't even desire to do it. \$\endgroup\$ – Jonah Jun 30 '17 at 18:52
  • \$\begingroup\$ @ConorMancone since javascript doesn't have interfaces so I think you mean duck typing, right? Also, If I think more about it passing symbols like t, tr, r etc feels weird because those are nor properties of a terminal but can be properties of a box, wdyt? \$\endgroup\$ – CodeYogi Jul 2 '17 at 2:41
  • \$\begingroup\$ @CodeYogi yes, duck typing is the way to go. Regarding passing symbols (if I understand you correctly), what you are getting at is exactly the point of SOLID. Right now your class is very specific: print out a box for a terminal. By taking the terminal out though, and simply specifying the some properties of the object that gets passed in, it becomes much more reusable and testable. It can be reused for any number of cases that you don't specifically have to plan for: anyone who wants to use it just has to abide by the "contract" for your input. \$\endgroup\$ – Conor Mancone Jul 2 '17 at 10:34

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.