Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.javascript > #37555

Re: Weather App reworked

Show all headers | View raw

On Sunday, October 13, 2019 at 5:31:46 AM UTC-5, luserdroog wrote:
> I reworked the Weather App to try to make better use of Object Orientation.
> It seems to really help to group related things together. My use of the
> controller seems a little weird, though. I got all the control functions
> grouped nicely, but they're not in a class. I didn't see any easy way to
> do that given the way it all works. I'm not sure if I need more than one
> control so that may be moot.
> 
> The MVC classes have been supplemented with a SessionModel that backs the
> value up in session storage, a DependentModel that observes other Models
> and modifies its own value before notifying any views, and a few helpful
> functions in the View class.
> 
> The app itself starts by making a few customized Models. Then all the
> model instances are defined. Then all the views on those models. Then
> the controller is extracted from the primary view and used to hold
> the UI functions. 
> 
> There still remain a few unencapsulated functions but they are all pure
> utility functions it doesn't seem too bad to leave them vanilla.
> 
> https://luser-dr00g.github.io/weather4/
> https://github.com/luser-dr00g/luser-dr00g.github.io/tree/master/weather4
> 
> Comments welcome.
> 

1. XHR vs Fetch

XmlHttpRequest is becoming legacy. Consider using Fetch instead:

<https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch>


2. Conventions

The convention for variable and method names is camelCase:

show_current_weather -> showCurrentWeather

3. Subclassing

You're still not using classes properly:

```javascript
var control = report.getController();
Object.assign( control, {
  search: function(){ location.value = this.getView().find('.i_search').value },
  unit_imperial: function(){ unit.value = 'imperial' },
  unit_metric:   function(){ unit.value = 'metric'   },
  unit_standard: function(){ unit.value = 'standard' },
  current:   function(){ report_type.value = 'current'   },
  forecast2: function(){ report_type.value = 'forecast2' },
  forecast3: function(){ report_type.value = 'forecast3' },
  forecast4: function(){ report_type.value = 'forecast4' },
  forecast5: function(){ report_type.value = 'forecast5' },
  show_server_data:function(){ showing_server_data.value = true  },
  hide_server_data:function(){ showing_server_data.value = false },
});
```

Create a new subclass instead. I assume this is the intent:

```javascript
class WeatherController extends Controller {
  search(){ ... }
  unitImperial(){ ... }
  unit_metric(){ ... }
  unit_standard(){ ... }
  current(){ ... }
  forecast2(){ ... }
  forecast3(){ ... }
  forecast4(){ ... }
  forecast5(){ ... }
  showServerData(){ ... }
  hideServerData(){ ...}
}

var foo = new WeatherController()
```

4. use the DOM instead of innerHTML

You're doing things like the following:

```javascript
format_forecast_data( data, days ){
  return `<h1>${days} days forecast for \
<nobr>${data.city.name},</nobr> ${data.city.country}</h1>
<div class=report_data>${this.format_forecast_results(data,days)}</div>`;
}
```

Instead, have the element in the page, then update it directly.

You have the find(...) method now, so use it:

```javascript
...
this.find('.report_data').textContent = ...;
```

5. Switches are not Object Oriented.

You have the following:

```javascript
switch( report_type.value ){
  case 'current':   this.show_current_weather( value ); break;
  case 'forecast2': this.show_forecast( value, 2 );     break;
  case 'forecast3': this.show_forecast( value, 3 );     break;
  case 'forecast4': this.show_forecast( value, 4 );     break;
  case 'forecast5': this.show_forecast( value, 5 );     break;
}
```

This is a hint that you have a missing class or are not using inheritance. You need to refactor this so that it becomes a single method call on an object. Something which may help with understanding why would be a blog post of mine on a related issue:

<https://thenewobjective.com/the-great-maze-of-jquery/>

6. Don't pass a function to super

```javascript
  constructor( model ){
    super( model,
      function( value ){
        ...
      });
  }
```

This is a hint that you should override/extend your class.

```javascript
class View {
  ...
  constructor( model, update ){
    if( model ) this.setModel( model )
    if( update ) this.onUpdate = update;
  }
  update( data ){
    if( typeof( this.onUpdate ) == 'function' ) this.onUpdate( data );
    this._subViews.forEach( s=> s.update( data ) );
  }
   ...
}
```

Something you can do here is define an onUpdate method that does nothing by default. This would eliminate the need for an `if` statement in the update method and the need to pass it to the constructor:


```javascript
class View {
  ...
  constructor( model ){
    if( model ) this.setModel( model )
  }
  update( data ){
    this.onUpdate( data );
    this._subViews.forEach( s=> s.update( data ) );
  }
  onUpdate(value) { ... }
   ...
}

class WeatherView extends View {
  ...
  onUpdate(value) { 
    switch( report_type.value ){
      case 'current':   this.show_current_weather( value ); break;
      ...
    }
 }
  ...
}

```

Notice that the WeatherView will no longer need an explicit constructor defined

Now do the same thing for all of the Views.

```javascript
var unit_controls = new View( unit,
  function( value ){
    console.log( value );
    this.show( this.find('.unit_imperial') );
    this.show( this.find('.unit_metric') );
    this.show( this.find('.unit_standard') );
    this.hide( this.find('.unit_' + value) );
  }
);

// becomes:

class UnitControls extends View {
   ...
   onUpdate(value){...}
   ...
}
```

7. View in context of an element

If you associate each view with a specific element, many of your methods
can be simplified greatly:

```javascript
class View {
  _htmlElement
  ...
  find(sel){
    return this._htmlElement.querySelector(sel)
  }
  show(){
    this._htmlElement.style.display = 'block'
  }
  hide(){
    this._htmlElement.style.display = 'none'
  }
  ...
}
```

and so on.

8. Utility functions are just undefined objects

You have a lot of these. Take a look at what they operate on and you can
see what they should be methods of.  For example the date functions can be defined as methods of your own Date subclass:

```javascript
class Chrono extends Date {
  dayOfWeek(){
    let w = ['Sunday','Monday','Tuesday','Wednesday','Thursday','Friday','Saturday'];
    return w[ this.getDay() ];
  }
  ...
}
```

There is more, but I'm out of time for this round. HTH

Back to comp.lang.javascript | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Weather App reworked luserdroog <luser.droog@gmail.com> - 2019-10-13 03:31 -0700
  Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-13 05:49 -0700
    Re: Weather App reworked luserdroog <luser.droog@gmail.com> - 2019-10-14 10:53 -0700
      Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-14 21:22 -0700
  Re: Weather App reworked John G Harris <niam@jghnorth.org.uk.invalid> - 2019-10-13 14:46 +0100
    Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-13 07:02 -0700
      Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-13 07:08 -0700
        Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-13 07:16 -0700
          Re: Weather App reworked "Michael Haufe (TNO)" <tno@thenewobjective.com> - 2019-10-13 10:10 -0700
            Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-13 10:34 -0700
          Re: Weather App reworked John G Harris <niam@jghnorth.org.uk.invalid> - 2019-10-13 19:55 +0100
            Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-14 05:56 -0700
              Re: Weather App reworked John G Harris <niam@jghnorth.org.uk.invalid> - 2019-10-15 18:21 +0100
                Re: Weather App reworked Julio Di Egidio <julio@diegidio.name> - 2019-10-15 12:32 -0700
                Re: Weather App reworked John G Harris <niam@jghnorth.org.uk.invalid> - 2019-10-16 16:06 +0100
  Re: Weather App reworked "Michael Haufe (TNO)" <tno@thenewobjective.com> - 2019-10-13 14:02 -0700
    Re: Weather App reworked dr.j.r.stockton@gmail.com - 2019-10-14 13:28 -0700
      Re: Weather App reworked "Michael Haufe (TNO)" <tno@thenewobjective.com> - 2019-10-14 14:58 -0700
    Re: Weather App reworked luserdroog <luser.droog@gmail.com> - 2019-10-15 09:00 -0700

csiph-web