• Recent
  • Tags
  • Unsolved
  • Solved
  • MagicMirror² Repository
  • Documentation
  • 3rd-Party-Modules
  • Donate
  • Discord
  • Register
  • Login
MagicMirror Forum
  • Recent
  • Tags
  • Unsolved
  • Solved
  • MagicMirror² Repository
  • Documentation
  • 3rd-Party-Modules
  • Donate
  • Discord
  • Register
  • Login
A New Chapter for MagicMirror: The Community Takes the Lead
Read the statement by Michael Teeuw here.

Please **review** my code, since I lack experience - mmm_velib, A module for bike sharing schemes in Europe

Scheduled Pinned Locked Moved Development
9 Posts 4 Posters 4.1k Views 4 Watching
Loading More Posts
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • P Offline
    poutr
    last edited by Feb 6, 2017, 12:18 PM

    Hi,

    I finished this small module and I would greatly appreciate if people with experience could review the code. It is based on default module architecture calendar and newsfeed.

    It’s a live update of (chosen by users) bike sharing stations. Should cover quite a few cities too.

    https://github.com/Poutrathor/mmm_velib

    I think it is good right now but :

    • almost no experience on JS, html, css and node.
    • Not that objective on my own work.

    I let the logs and comment to help with reviewing.
    I hope the readme.md is clear enough.

    Thank you very much for your time :)

    PS : Should I post it now to showcase or should I wait someone has checked for issues before ?

    S 1 Reply Last reply Feb 6, 2017, 1:00 PM Reply Quote 0
    • S Offline
      strawberry 3.141 Project Sponsor Module Developer @poutr
      last edited by Feb 6, 2017, 1:00 PM

      @poutr in thefetcher you still name the functions like news, would be better to name it something with bike
      the module js has unused methods like capitalizeFirstLetter and getTranslations, you can remove them completely and you could shorten your switch case to something like:

      
      this.stations = {
        PARIS: "Velib'",
        LYON: "Vélo'v",
        MULHOUSE: "VéloCité",
        BESANCON: "VéloCité",
        MARSEILLE: "Le vélo",
        TOULOUSE: "VélôToulouse",
        ROUEN: "Cy'clic",
        AMIENS: "Velib",
        NANTES: "Bicloo",
        NANCY: "vélOstan'lib",
        "CERGY-PONTOISE": "VélO2",
        "PLAINE-COMMUNE": "Velcom",
        CRETEIL: "Cristolib"
      };
      
      
      var marque = "Cyclocity";
      var contractName = this.stationsData[0].contract_name.toUpperCase();
      if(this.stations.hasOwnProperty(contractName)){
        marque = contractName;
      }
      
      

      Please create a github issue if you need help, so I can keep track

      Y 1 Reply Last reply Feb 6, 2017, 2:04 PM Reply Quote 1
      • P Offline
        poutr
        last edited by Feb 6, 2017, 1:36 PM

        Thanks,

        Yup, I have to clean this up. I get your fancy code over the switch case, that’s nice writing, I like it.

        Anything else I should change, double proof or something ?

        1 Reply Last reply Reply Quote 0
        • Y Offline
          yawns Moderator @strawberry 3.141
          last edited by yawns Feb 6, 2017, 2:04 PM Feb 6, 2017, 2:04 PM

          @strawberry-3.141
          I shouldn’t go out with the dog. The long switch block was my first spot too, but you beat me to it :)

          1 Reply Last reply Reply Quote 1
          • P Offline
            poutr
            last edited by Feb 6, 2017, 7:23 PM

            Ok, I have done the code clean up.

            I use this call below to get the property value and not the propetry name, e.g. to get “Vélib’” and not “PARIS”. Am I right ?

            var contractName = this.stationsData[0].contract_name.toUpperCase();
            if (this.contractList.hasOwnProperty(contractName)) {
            	marque = this.contractList[contractName];
            }
            

            If there is no other issues spotted, that’s great :)

            Stupid question : how does one update his 3rd party module from github ? I used git clone https://github.com/Name/repo to install it on my mirror, but I don’t get how to update locally when the master on github is updated. Fetch failed me (no commit in common or something). I deleted and clone again but that’s so ugly.

            1 Reply Last reply Reply Quote 0
            • Y Offline
              yawns Moderator
              last edited by Feb 6, 2017, 7:25 PM

              Enter your module folder:

              git pull origin master
              
              P 1 Reply Last reply Feb 6, 2017, 8:08 PM Reply Quote 0
              • P Offline
                poutr @yawns
                last edited by Feb 6, 2017, 8:08 PM

                @yawns ty, i will try it !

                1 Reply Last reply Reply Quote 0
                • B Offline
                  Brice
                  last edited by Brice Feb 17, 2017, 2:02 AM Feb 17, 2017, 2:01 AM

                  Suggestions :

                  • format header correctly (class name = module-header)
                  • remove station number from name
                    Great module btw !
                  P 1 Reply Last reply Feb 21, 2017, 6:21 PM Reply Quote 0
                  • P Offline
                    poutr @Brice
                    last edited by Feb 21, 2017, 6:21 PM

                    @Brice Thanks!

                    ok for the format, will do that.

                    I am uneasy about removing the number from the name :

                    • It comes as it from the JCDecaux server. Any parsing from my part would be under risk from future modifications.
                    • The number is the ID, it feels more precise than a ‘name’.

                    I would need more users’ feedback for this point but with a grand total of one user ‘me’, I think I will let it like this, don’t you agree?

                    cheers

                    1 Reply Last reply Reply Quote 0
                    • 1 / 1
                    • First post
                      Last post
                    Enjoying MagicMirror? Please consider a donation!
                    MagicMirror created by Michael Teeuw.
                    Forum managed by Sam, technical setup by Karsten.
                    This forum is using NodeBB as its core | Contributors
                    Contact | Privacy Policy