Coupling and cohesion example (Refactoring the code)

Question

I know what coupling and cohesion mean.
I have got the following example and it has some maintainability issues and therefore it needs some refactoring:

The problem is, I could not relate any coupling, cohesion or any other maintainability issue other than GLOBAL VARIABLES. How can I refactor the following code without this global variables issue (because global variables increase complexity & increases the code inflexibility?!)

double value;
double min, max;

public void read()
{
    do 
    {
        value = ConsoleInput.readDouble();
    }
    while(value < min || value > max);
}

public double hyp()
{
    double x, y;
    min = 0.0;
    max = 100.0;
    read();
    x = value;
    read();
    y = value;
    return Math.sqrt(x * x + y * y);
 }

I was thinking it to refactor like below:

public void read()
{
    double value;
    double min = 0.0;
    double max = 100.0;

    do 
    {
        value = ConsoleInput.readDouble();
    }
    while(value < min || value > max);
}

public double hyp()
{
    double x, y;
    read();
    x = value;
    read();
    y = value;
    return Math.sqrt(x * x + y * y);
 }

Does this look right? Or is there any other efficient way of refactoring this?

You variable `value` is the same in method `hyp`. The refactored `read` method now does nothing, because it doesn't mutate any state nor does it return anything. So I guess you would refactor it with your suggestion into errors. Could you provide maybe more code how your method `hyp` is called? Where does its input paramter `value` come from? — DrunkenPope, May 27 '15 at 13:45
@DrunkenPope Yes actually I do agree that the code doesn't show sufficient information about calling the methods, but this is how I found the example while learning about code modularity. — Dee, May 27 '15 at 13:48
@DrunkenPope Oops actually the method `hyp()` doesn't need any input parameter, I just edited it. — Dee, May 27 '15 at 13:50
I'm voting to close this question as off-topic because working code looking to be refactored should be asked about on CodeReview.SE, not Stack Overflow. — TylerH, Jan 26 '17 at 20:24

score 2 · Answer 1 · edited May 27 '15 at 14:07

2

Refactor your read() method like this:

public double read(double min, double max)

So rather than saving the value in a global, you return it from the function

edited May 27 '15 at 14:07

Am_I_Helpful

18,735
7
49
73

answered May 27 '15 at 13:39

ControlAltDel

33,923
10
53
80

1

@ControlAltDel Do you mean something like `public double read(double x, double y)` ? but I think this won't work either if the user wants to enter different values for x and y – Dee May 27 '15 at 13:44
1

@shekharsuman '... uselessly increasing the complexity...' not true. What I've done is make the method stateless. If you don't see benefits in that, you should read more about statefulness vs. statelessness – ControlAltDel May 27 '15 at 14:03
@Dee no, I mean exactly what I wrote. So in the read call, the min and max values would be passed in as parameters, rather than defined in object variables. This makes the function stateless vs. the previous stateful state – ControlAltDel May 27 '15 at 14:05
@ControlAltDel-Actually, I went through OP's question and I've to agree that this is more maintainable and,also a stateless representation. And, I am reversing my vote---an upvote to you. – Am_I_Helpful May 27 '15 at 14:06
Could you be a bit more specific, why this increases maintainability? and what does stateless representation mean actually? If possible could I have any useful link to it? – Dee May 27 '15 at 14:13
Here's a post about the benefits to stateless programming: http://stackoverflow.com/questions/844536/advantages-of-stateless-programming – ControlAltDel May 27 '15 at 14:28

DrunkenPope · Answer 2 · 2015-05-27T15:01:49.627

0

Based on the information you provided, I would suggest the following:

public double read() {
    // set value to a not accepted value to start the while-loop
    double value = -1;
    double min = 0.0;
    double max = 100.0;

    while(value < min || value > max) {
        value = ConsoleInput.readDouble();
    }
    return value;
}

public double hyp() {
    double x = read();
    double y = read();
    return Math.sqrt(x * x + y * y);
}

edited May 27 '15 at 15:01

answered May 27 '15 at 13:50

DrunkenPope

544
10
18

But the `read()` method doesn't return double value (its void). This arises the problem at `double x = read();` and `double y = read()` ? – Dee May 27 '15 at 13:56
So the method `read()` should be changed to `public double read()` and add `return value;` statement – Dee May 27 '15 at 13:58
Yeah, sorry, missed to change that. – DrunkenPope May 27 '15 at 15:02

libik · Answer 3 · 2015-05-27T14:21:39.313

0

Having "global variables" like min and max is absolutely fine, if they are "global" only inside its own class - thats the way the class variables are supposed to be used.

However "read" value should return value, not insert it into some class variable and then use. Also it would be nice to create instance of class with min and max in constructor for maximum flexibility, but also having default constructor.

public class GoodApp {

    private double min, max;

    public GoodApp(double min, double max){
        this.min = min;
        this.max = max;
    }

    public GoodApp(){
        this(1,100);
    }

    public double read() {
        double value;
        do {
            value = ConsoleInput.readDouble();
        } while (value < min || value > max);
        return value;
    }


    public double hyp() {
        double x, y;
        x = read();
        y = read();
        return Math.sqrt(x * x + y * y);
    }
}

edited May 27 '15 at 14:21

answered May 27 '15 at 13:53

libik

22,239
9
44
87

I didn't quite get what `public GoodApp()` method is doing here? I mean I didn't get `this(1,100);` ? – Dee May 27 '15 at 14:07
Dee - those are constructors, google them – libik May 27 '15 at 14:16
I understand thats a constructor, but what does `this(1,100)` mean? – Dee May 27 '15 at 14:17
I understood it after the edit, but the min and max should be double so the constructor should be something like `this(0.0, 100.0)` – Dee May 27 '15 at 14:18
Integer is imlicitly casted to double, therefore there is no need to change 100 to 100.0 – libik May 27 '15 at 14:21

Ouney · Answer 4 · 2015-05-27T15:50:45.583

Here is my version. Few points to keep in mind - A method should expose its dependencies properly (What it depends on - like dependency injection), otherwise it can be a liar. Also, your read method is not making use of the state of current object (i.e. it is not employing this reference). So, it can be static (but that makes unit testing tough - if it is a concern for you).

So, I would suggest the following (This may seem to be overkill for this small program - but good in real time projects. It reduces coupling because you can push any implementation of ReadData) -

enum Bound{

 MAX(100.0), MIN(0.0);

 public double value(){
     return this.value;
 }

 private final double value;

 private Bound(double value){
     this.value = value;
 }

}


public class CalcHyp{

ReadData readData;

CalcHyp(ReadData readData){
    this.readData = readData;
}

public double hyp() {
    double x = readData.read();
    double y = readData.read();
    return Math.sqrt(x * x + y * y);
}

public static void main(String[] args) {
    CalcHyp calcHyp = new CalcHyp(new ReadData());//Declare the dependencies.(Dependency Injection)
    System.out.println(calcHyp.hyp());
}

}

class ReadData{ //Can declare an interface in real time, and various implementations based on your requirement.

  double read() {

    double value = Bound.MAX.value()+1;
    while(value < Bound.MIN.value() || value > Bound.MAX.value()) {
        value = ConsoleInput.readDouble();
    }
    return value;
}
}

Coupling and cohesion example (Refactoring the code)

4 Answers4