Handling a spinner for multiple REST calls Context
In an Angular application, I have three <select> boxes that are fed from the database with a relatively small data set. These boxes are part of a filter Component that is displayed next to a server-side data table. The component outputs the filter asynchronously to the parent component, which then pipes into the table, invokes server, filters out SQL etc.
Focusing on the scope, when the control is loaded, I want to invoke the server with three GET invocations. Until they complete, I want to display a spinner.
Since I am using a library of controls, part of the code is already done. I want to ask here if I am handling the Observables correctly from the Angular point of view
Code I wrote so far
The code works
export class MyComponent implements OnInit, OnDestroy {

  public filterLoading$: BehaviorSubject<boolean>;
  public select1: Observable<string[]>;
  public select2: Observable<string[]>;
  public select3: Observable<string[]>;
  private filterLoadingSub$?: Subscription;

  constructor(private remoteService: RemoteService) {
    this.select1 = remoteService.getFirst();
    this.select2 = remoteService.getSecond();
    this.select3 = remoteService.getThird();
    this.filterLoading$ = new BehaviorSubject<boolean>(true);
  }

  ngOnInit(): void {
    this.filterLoadingSub$ = forkJoin([this.select1, this.select2, this.select3])
      .subscribe(() => this.filterLoading$.next(false))
    ;
  }

  ngOnDestroy(): void {
    if (!!this.filterLoadingSub$) {
      this.filterLoadingSub$.unsubscribe();
    }
  }

}

Explanation:

selectX are bound to the front end with <option *ngFor="let x of (selectX | async)" ...>
The subject filterLoading$ is used to display the spinner. Actually my custom accordion handles it for me <acme-filter-panel isLoaded="!(filterLoading$ | async)>
Then I used forkJoin operator to wait for the completion of all three calls. I don't care about the last returned valued, only that the three have completed. But this leads to a Subscription
Under the philosophy of dispose your own garbage, every time I hit a Subscription I always write code to unsubscribe. However, these are just HTTP calls that return only once when invoked.

I truly understand that I am not handling error logic, but really that goes beyond the scope of the question
Questions
My question is about two aspects.
Apart from moving code from constructor to ngOnInit, on reasons of symmetry, am I writing too much code or is there some simplification I can use to say Angular to unlock the spinner when all of the three invocations occurred?
Is it always necessary to always unsubscribe from subscriptions? Or are there cases in which a dangling subscription harms none? I learned that subscriptions use memory, so I deem necessary to clean up resources whenever possible, even if modern machines have gigabytes of RAM.
More in general, is it possible to write this piece in a more concise way? For three data sets I had to instantiate a fourth member, which is actually needed to monitor the spinner, and a fifth member used only on destruction to clean up. I often work with Observables & co.

It is in theory not necessary to unsubscribe all subscriptions. For example if you call the backend with the Angular HttpClient then you will get an observable, that will automaticly complete after it emits the first result. As soon as it is completed, the observable is gone and does not strain your ressources anymore.
That means, if the Observable will complete itself in a timely manner, then you do not have to unsubscribe.
BUT: In most cases you have to dig a bit deeper to see that an observable will complete itself. That means the reader of your code will not easily know if you forgot the unsubscribe or if it could be skipped. Therefore i personaly decided for safety (but more code) and unsubscribe always.
Not unsubscribing will result in unnecessary memory consumption and/or can create unexpected sideeffects. The function that should be executed at emits, is stored at the Subject. That means even when your component is long ago destroyed, the function will still "live" at the Subject. And will be executed at each emit...
But you could make unsubscribing a bit nicer with a Subscription Object
private subscriptions: Subscription = new Subscription();
...
this.subscriptions.add( myOberservable.subscribe(value => doMagic(value)) );
this.subscriptions.add( myOtherOberservable.subscribe(value => doMoreMagic(value)) );
...
this.subscriptions.unsubribe() // will unsubscribe all added subscriptions at once

About the general question of implementing the spinner. If you always want a spinner, you could attach it in an interceptor, activating the spinner before the call is fired and deactivated as soon as you get a respond.
Or using a central method for backend calls and doing the the toggle there.
In both cases you would need a spinner service that needs to remember how often it was "activated", so that if 3 calls are triggered at once, the first response does not deactivate the trigger.
May ways are possible, it depends a bit on your use case and your general architecture.
But your specific example i would write in the following way:
export class MyComponent implements OnInit, OnDestroy {

  private subscriptions: Subscription = new Subscription();
  private filterLoading$:BehaviorSubject<boolean> = new BehaviorSubject<boolean>(true);

  ngOnInit(){
      this.subscriptions.add(this.callAll().subscribe(() => this.toggleSpinner()));
  }

  ngOnDestroy(){
      this.subscriptions.unsubscribe();
  }

  private callAll():Observable[]{
    const listOfCalls: Observable[] = [
          remoteService.getFirst(),
          remoteService.getSecond(),
          remoteService.getThird(),
    ];
    return forkJoin(listOfCalls);
  }

  private toggleSpinner():void{
      this.filterLoading.next(false)
  }
}

Its nearly the same way you used. Only a few changes.

If there is no need to declare something public, then its private.
I make use of the "Subscription" Object, even if in this example there is just one subscription to be stored. I try to be consisten with solutions. Also it makes it easier to extend the component when a second subscription is needed.
I like to hide code behind methods. That way someone reads the methodname and then can decide if he/she is interested in the details or not.

All in all its more code. But i think its easier to understand. And in my experience, code is read quite more often then writen. And "read" really means "reading and understanding". And thats an very expensive task. :-)
As always, the solution totaly depends on the context. So, choose the things that suit you well and skip the rest. :-)


Use of templates with templated Deck class I have previous revisions of my deck of cards project, but I may not need to link them here since the emphasis is on the use of templates.

I've never used them before until now, and I like how intuitive my implementation has become.  However, I'm also aware that there are many pitfalls in C++ templates.  Although I don't think this implementation uses them heavily, I'd still like to know if I'm at a good start before I use them again.

I'm also a bit unsure about some of my naming now, considering this is a template class.  For instance, I've changed add()'s parameter from card to item because it cannot be assumed that the user will only use cards.  Although it makes sense to just use cards (or anything representing cards), it would not seem practical to limit the implementation to just that usage.  I would prefer this to be reusable.

Of course, I'm okay with any other improvements that can be made.

Deck.hpp

#ifndef DECK_HPP
#define DECK_HPP

#include <algorithm>
#include <stdexcept>
#include <vector>

template <class T> class Deck
{
private:
    std::vector<T> original;
    std::vector<T> playable;

public:
    typedef typename std::vector<T>::size_type SizeType;

    void add(T const&);
    T draw();
    void reset();
    void shuffle();
    SizeType size() const { return playable.size(); }
    bool empty() const { return playable.empty(); }
};

template <class T> void Deck<T>::add(T const& item)
{
    original.push_back(item);
    playable.push_back(item);
}

template <class T> T Deck<T>::draw()
{
    if (playable.empty())
    {
        throw std::out_of_range("deck empty");
    }

    T top(playable.back());
    playable.pop_back();
    return top;
}

template <class T> void Deck<T>::reset()
{
    playable = original;
}

template <class T> void Deck<T>::shuffle()
{
    if (!playable.empty())
    {
        std::random_shuffle(playable.begin(), playable.end());
    }
}

#endif

Bottom line: your code here is focused and solid. It's a well-written introductory use of templates. However I'm not sure it's a great scenario in which to use them. First I'll talk about ways to improve its reusability, and then I'll say more about why I think it's a questionable scenario.
As you start trying to implement your own templatized data structure, there are a lot of concerns to think about. Above all else you need to consider how your data structure will be used; what operations will consumers of it expect, what limitations will they have to put up with, and whether any of the data structure's implementation choices lock them into bad patterns. In general I would say you would do well to follow the lead of the STL data structures unless you have specific reasons not to. Here are a few of the places where you currently differ:
void Deck<T>::add(T const &)
This one actually is pretty much spot on. But I want to talk about the implications of how it's implemented. You are requiring T to be copyable. Thus if a consumer of this class needs to compare cards, T will need to compare itself through means other than by its address. This isn't particularly unusual, but I just wanted to be clear that comparing T instances by address is invalidated by Deck<T>'s implementation.
The biggest question mark here is the lack of a range insertion, perhaps via constructor or add(InputIterator first, InputIterator last). Similarly there's no index-based insertion, but that wouldn't mix well with the two vectors that are often out of sync.
T Deck<T>::draw()
Before C++11 introduced move semantics, there was not necessarily a low cost way to examine and remove an item from a data structure. This is why vector and other data structures let you examine the front or back, but don't return the removed item on a pop_back(). While it's perfectly good to wrap the functionality of the underlying storage, as the cost of copying T goes up, so does the cost of this implementation of draw(). Implementing it in some other fashion (typically as two separate methods for peeking and removing) is suggested before C++11.

Taking into account what Corbin and Loki Astari point out, it's more relevant to consider weak and strong exception guarantees (essentially the ability to reason about correctness in unusual situations) instead of the cost of copying (thanks especially to RVO). Whether you need to care about either is still completely dependent on your use case in practice, and thus only of extreme importance in general purpose data structures, or in ones you know need to offer it.

Inside draw()
Your implementation of draw() verifies that the playable deck has items remaining. This is certainly good for catching errors, but leads to repeated checks for the same data. Either the calling code will look like while (true) { ::: draw(); ::: } and will have to use a try/catch to find out when the deck was empty, or it will look like while (!deck.empty()) { ::: draw(); ::: }. The former is a questionable way to use exceptions, as an empty deck is hardly an unexpected condition, but the latter means both the caller and callee are repeating the empty check. It's also possible that the caller knows this in other ways, such as having put 52 cards in the deck and then looping 52 times to empty it.
As an aside, it can still be useful to have this sort of check available by another name (consider the difference between vector<T>::operator[] and vector<T>::at), or available through a compile-time option (such as a DEBUG build). But it's typically good to make an unchecked version available (vector's operator[] case).
void Deck<T>::reset()
I'm lumping a couple comments in here, and this is less about following the STL's lead. Like in draw(), as the cost of copying T goes up or the size of your deck goes up, so does the cost of reset(); is it possible that you don't need to save the original order? If you don't, then perhaps it would be better to consider a different implementation:
one vector<T> containing the order of the deck, one vector<T>::const_iterator or vector<T>::size_type containing the index/iterator through which you've already played. This would have a lot of repercussions, so I'm going to show the methods that it would impact, assuming the class has a size_type deck_top, and that we remove original. I like the shorter code, but that has less to do with the index and more to do with the other changes.
template <class T> bool Deck<T>::empty() const
{
    return deck_top >= playable.size();
}

template <class T> T const & Deck<T>::draw() // note we can now return a reference
{
    return playable[deck_top++];
}

template <class T> void Deck<T>::reset()
{
    deck_top = 0;
}

template <class T> void Deck<T>::shuffle()
{
    std::random_shuffle(playable.begin() + deck_top, playable.end());
}

This of course has multiple tradeoffs besides the ones I already mentioned. Perhaps the biggest is that there's still no way to remove cards (temporarily or permanently), so it's hard to model a game that shuffles a partial deck while people are still holding some cards in their hands. I think you will rapidly find that it's hard to make this data structure usable for all the scenarios you think you want it to support without making it almost as generic as vector itself. At which point you will have to consider whether it's worth reusing the data structure, or if it's really just for a single program's use.
Final notes
As a generic comment, I'm undecided on card vs. item. I think card conveys the intended use better, and doesn't really get in the way if someone wants, say, a "Deck" of tiles instead. But I do agree in general with the point about not boxing yourself in by your parameter names.


Makefile to build library I'm starting with make, and I made my first makefile to build my project. I don't know if it's the best way to do it or not, so I share with you my makefile to get your opinion on it:

Directory structure:

├── LICENSE
├── Makefile
├── objs
│   └── parser.o
├── out
│   ├── libsip.a
│   └── parser.h
├── README.md
├── src
│   ├── parser.c
│   ├── parser.d
│   └── parser.h
└── tests
    ├── a.out
    ├── config.ini
    ├── libsip.a
    ├── parser.h
    └── test.c

Makefile:

SHELL = /bin/sh

CC := gcc
CFLAGS := -Wall

SRCSDIR := src
OBJSDIR := objs
OUTDIR := out

SRCS := $(wildcard $(SRCSDIR)/*.c)
OBJS := $(SRCS:$(SRCSDIR)/%.c=$(OBJSDIR)/%.o)

$(OUTDIR)/libsip.a : $(OBJS)
    @mkdir -p $(@D)
    ar cr -o $@ $^
    cp -f src/parser.h $(OUTDIR)

$(OBJSDIR)/%.o : $(SRCSDIR)/%.c
    @mkdir -p $(@D)
    $(CC) $(CFLAGS) $< -c -o $@
    $(CC) -MM $< > $(patsubst %.c, %.d, $<) && \
    sed -i \ '1s|^|$(@D)/|' $(patsubst %.c, %.d, $<)
# use sed to add path to object dependencies

.PHONY : clean
clean :
    rm -rf $(OBJSDIR) $(OUTDIR)

-include $(SRCS:%.c=%.d)

Use of the naked utilities (cp, mkdir, sed, and especially rm) is usually frowned upon. It makes your makefile vulnerable to a path attack; aliasing may also result in some strangeness (for example, many distros come with rm aliased to rm -i); also keep in mind that GNU make works nativly in Windows environment, which lacks those utilities.

Typically what you do is to define them like RM := /bin/rm (conditionally tailoring for all expected platforms), etc and forget all the complications once and forever. Or use autotools.
mkdir fails if the target directory exists. -mkdir is a usual practice.
I prefer to have separate rules for dependencies and objects. Having them combined, make builds the .o file based on old dependencies. You may end up in an inconsistent state.


Rails create create default database records In my application I have an accounts model:

class Account < ActiveRecord::Base
  belongs_to :user
  has_many :invoices
end

The Account model belongs to the User model. After the user is created, some default accounts must be created:

class User < ActiveRecord::Base
  has_many :accounts

  after_create do
    Account.create(name: "Repairs",user: self)
    Account.create(name: "Supplies", user: self)
  end
end

Invoices can then be assigned to one of these initial accounts:

class Invoice < ActiveRecord::Base
  belongs_to :accounts
end

Is this the proper way to to create the default accounts?

Yeah! you are on right path but we can improve it like :

class User < ActiveRecord::Base
  has_many :accounts

  after_save do
    self.accounts.create([{name: "Repairs"}, {name: "Supplies"}])
  end
end

Hopefully! This will help.


Comparator classes supporting operations on sets I have a class Comparator that does several operations (union, intersection, etc) on 'sets'.  In this specific case each 'set' is a 3 dimensional array.

in order to do these operations, I have to use a nested foreach to loop through each dimension before getting to the data I need to compare against.

I figure there are two ways to implement this.  One is to simply do the loop in each operation, like so:

class Comparator  {

    //union is the set of unique elements from both setA and setB
    public function union()  {
        //initially add $setB to $union resultset.
        $union = self::$setB;

        foreach (self::$setA as $k => $v)  {
            foreach ($v as $k2 => $v2)  {
                foreach ($v2 as $k3 => $element)  {
                    if (!isset($union[$k][$k2][$k3]))  {
                        //add $element to $union resultset if it does not already exist in $union.
                    }
                }
            }
        }

        return $union
    }

    //Set of elements in setB that are not in setA.
    public function rightComplement()  {
        $complement = self::$setB

        foreach (self::$setA as $k => $v)  {
            foreach ($v as $k2 => $v2)  {
                foreach ($v2 as $k3 => $element)  {
                    //if $element is in setA, remove it from $complement.
                    unset($complement[$k][$k2][$k3]);
                }
            }
        }

        return $complement;
    }
}

Problem is I have about 9 other operations that all operate off of an identical loop... just a different thing is done inside the final foreach.

The other way is bit drier, but I feel somewhat confusing:

class Comparator  {

    //union is the set of unique elements from both setA and setB
    public function union($args = null)  {
        //initially add $setB to $union resultset.
        $union = self::$setB;

        if ($args === null)  {
            $this->iterate(self::$setA, __FUNCTION__);
        }  else  {
            if (!isset($union[$args['k']][$args['k2']][$args['k3']]))  {
                //add $element to $union resultset if it does not already exist in $union.
            }
        }

        return $union
    }

    //Set of elements in setB that are not in setA.
    public function rightComplement($args = null)  {
        $complement = self::$setB

        if ($args === null)  {
            $this->iterate(self::$setA, __FUNCTION__);
        }  else  {
            unset ($complement[$args['k']][$args['k2']][$args['k3']]);
        }

        return $complement;
    }

    public function iterate($obj, $function)  {
        foreach($obj as $k => $v)  {
            foreach ($v as $k2 => $v2)  {
                foreach ($v2 as $k3 => $element)  {
                    //call the method named in $function, pass back the keys and final value.
                    $this->$function(array(
                        'k1' => $k,
                        'k2' => $k2,
                        'k3' => $k3,
                        'element' => $element
                    ));
                }
            }
        }
    }
}

So my question is which (if either) is the better approach?  Is the second approach too DRY?  Too confusing?  I'm open to other approaches as well.

I don't know if something can be too DRY.  Too abstract perhaps.

    public function rightComplement($args = null)  {
        $complement = self::$setB

        if ($args === null)  {
            $this->iterate(self::$setA, __FUNCTION__);
        }  else  {
            unset ($complement[$args['k']][$args['k2']][$args['k3']]);
        }

PHP has closures.  You don't need this pattern.  Consider

    public function rightComplement()  {
        $complement = self::$setB;

        $this->iterate(self::$setA, function($i, $j, $k, $element) use ($complement) {
            unset($complement[$i][$j][$k]);
        });

Now rather than having two ways to call the function, one of which indirectly calls the other, there is one way.  We use the closure to provide the single element behavior instead.

I like this better than either of your choices, as it is more direct about what it does than the second.  And it is DRYer than the first version.  We call iterate with a closure that handles each element.

You don't complete the implementation for the other function, so I didn't try to duplicate it.


Simple image capture program This program is eventually going to be a screen recorder, but right now I just want it to capture screen images.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
using System.Drawing;
using System.Windows.Forms;

namespace ScreenRecorder
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        public static bool recording = false;
        public static string saveAddress = @"C:\Users\UserName\Desktop\something.bmp";

        public MainWindow()
        {
            InitializeComponent();
        }

        private void onClick(object sender, RoutedEventArgs e)
        {
            System.Drawing.Image currentImage;
            if (recording == false)
            {
                currentImage =  this.CaptureScreen();
                recording = true;
                recorderButton.Content = "Stop Recording!";
                currentImage.Save(saveAddress);
            }
            else
            {
                recording = false;
                recorderButton.Content = "Start Recording!";
            }
        }

        private System.Drawing.Image CaptureScreen()
        {
            System.Drawing.Rectangle screenSize = Screen.PrimaryScreen.Bounds;
            Bitmap target = new Bitmap(screenSize.Width, screenSize.Height);
            using (Graphics g = Graphics.FromImage(target))
            {
                g.CopyFromScreen(0, 0, 0, 0, new System.Drawing.Size(screenSize.Width, screenSize.Height));
            }
            return target;
        }
    }
}

Considering there is almost no code to review, this will be purely stylistic. Remember that criticism is a sign of love.

/// <summary>
/// Interaction logic for MainWindow.xaml
/// </summary>

The comment isn't needed, we all know what a codebehind is for. Comments should explain the why, not the what.
You should (almost) never have public fields. If they are actually needed outside of the current object's scope then you should make them properties, otherwise private.
Are they consciously static? This will mess up your program if you execute it multiple times simultaneously. A static modifier on a field is something you probably won't need very often and should be avoided unless needed.
You're hardcoding the user's path. It would be more userfriendly to let them pick this through a FolderBrowserDialog.
C#'s naming conventions dictate that methods are UpperCamelCase, not lowerCamelCase.
You've already imported System.Drawing, remove all the explicit namespace naming in your code.
currentImage is only used inside the first part of your if block so you should declare it there. Always restrict the visibility of variables to the most-restrictive scope possible. You'll notice you won't even need the temporary variable and might as well use CaptureScreen().Save(saveAddress).
if(recording == false) can be less verbosely written as if(!recording).
Don't use this unless you need it to distinguish between members of different scopes.
Use resource files to store your buttons their text so you can easily re-use messages and add additional languages. It also allows you to centrally manage all messages.
CaptureScreen() indicates a long-running operation with perhaps events raised to return information. That's not the case here. I suggest naming it GetScreenCapture().
Some people prefer using var to - again - be less verbose.


RubyWarrior beginner epic solution This is my RubyWarrior solution. I think RubyWarrior is well known to Ruby coders.  I am starting to learn Ruby and I would appreciate some help from you to improve the organization or the mistakes that I could have. I don't need another example as the Internet is full of them, but what I need is some advice that I can use as a beginner.

class Player
  attr_accessor :health, :direction, :n, :view, :warrior
  def initialize
    @health, @direction, @warrior  = health , direction , warrior
    @direction  = [:forward , :backward]
    @health, @view = [] , []
  end

  def play_turn(warrior)
    @health << warrior.health
    @view = warrior.look
    @backview = warrior.look :backward
    senses = warrior.feel @direction[@n]
      if ( senses.wall? && next_empty?(warrior) ) || senses.wall?
        warrior.pivot!
      elsif @backview[1].to_s.include?("Captive")
        warrior.walk! :backward
      elsif @backview[0].to_s.include?("Captive")
        warrior.rescue! :backward
      elsif next_empty?(warrior) && senses.stairs? || safe_step?
        warrior.walk!
        puts "Hero :D Finally I made it!"
      elsif senses.captive?
        warrior.rescue!
        puts "Hero ;)   feel free to face your destiny"
      elsif seek_and_shoot?
        warrior.shoot!
        puts "Hero >< BAAANNG!! Die already ranged scumbag"
      elsif warrior.health < 15 && safe?(warrior)
        warrior.rest!
        puts "Hero :(   I need to heal my wounds"
      elsif next_empty?(warrior)
        warrior.walk!
        puts "Hero :|   go go go!"
      else
        warrior.attack!
        puts "Hero :x   I will kill you evil #{warrior.feel.to_s.upcase}!"
      end

    life_bar_on_screen
    puts @view
    puts @backview
  end

  def loosing_life? warrior
    @health[-1] < @health[-2] ? true : false
  end

  def next_empty? warrior
    warrior.feel.empty?
  end

  def safe? warrior
    next_empty?(warrior) && !loosing_life?(warrior)
  end

  def life_bar_on_screen
    puts "_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ LIFE #{@health.last}"
    (1..@health.last).each { |x| print "▒ "}
    puts ""
  end

  def seek_and_shoot?
    foe = [ "Wizard" , "Archer", "Sludge"]
    o, oo, ooo = @view[0].to_s , @view[1].to_s , @view[2].to_s
    case
      when o == "nothing" && foe.include?(oo) then true
      #when o == "nothing" && ranged_foe.include?(oo) then true
      #when o == "nothing" && oo == friend then false
      when o == "nothing" && oo == "nothing" && foe.include?(ooo) then true
      else false
    end
  end

  def safe_step?
    friend = "Captive"
    o, oo, ooo = @view[0].to_s , @view[1].to_s , @view[2].to_s
    case
      when o == "nothing" && oo == friend then true
      when o == "nothing" && oo == "nothing" && ooo == friend then true
      else false
    end
  end

end

Shell output

CONGRATULATIONS! You have climbed to the top of the tower and rescued the fair maiden Ruby.
Level Score: 74
Time Bonus: 6
Clear Bonus: 16
Level Grade: A
Total Score: 503 + 96 = 599
Your average grade for this tower is: A

  Level 1: S
  Level 2: A
  Level 3: S
  Level 4: A
  Level 5: A
  Level 6: A
  Level 7: A
  Level 8: S
  Level 9: A

def loosing_life? warrior
    @health[-1] < @health[-2] ? true : false
  end

  def next_empty? warrior
    warrior.feel.empty?
  end

  def safe? warrior
    next_empty?(warrior) && !loosing_life?(warrior)
  end

Why I see no reason not to just make these part of the warrior class. It would make calling these much cleaner as you're never passing anything but a warrior into them.

  def life_bar_on_screen
    puts "_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ LIFE #{@health.last}"
    (1..@health.last).each { |x| print "▒ "}
    puts ""
  end

The life bar is a really cool idea. I like that a lot.

  elsif @backview[1].to_s.include?("Captive")
    warrior.walk! :backward
  elsif @backview[0].to_s.include?("Captive")

Anytime the same string literal shows up more than once, I recommend using a constant instead. It will protect you from spelling errors that can cause runtime problems. Apply the same advice to your seek_and_shoot? method.

  def seek_and_shoot?
    foe = [ "Wizard" , "Archer", "Sludge"]
    o, oo, ooo = @view[0].to_s , @view[1].to_s , @view[2].to_s
    case
      when o == "nothing" && foe.include?(oo) then true
      #when o == "nothing" && ranged_foe.include?(oo) then true
      #when o == "nothing" && oo == friend then false
      when o == "nothing" && oo == "nothing" && foe.include?(ooo) then true
      else false
    end
  end

The variable names here are pretty obtuse. What exactly are o, oo, and ooo? They're related somehow, but that's all I can readily gather from the names.

Also, commented out code is dead code. Remove dead code. If you're worried that you'll need it, then I recommend you start using source control of some kind. Personally, I like Git.


Simulated race among three robots making random steps At a job interview, I was asked to code the following algorithm:

Given are three robots which advance across a 1km lane at a different pace respectively. Each pace made by a robot is of a random distance.
All three robots stop once one of them is one pace away from reaching 1km. Once they stop, the robots' steps nearest to the middle of the lane are compared, and depending whose step throughout the journey was closest to the middle while exceeding it, a winner is declared. The winner robot's color is then printed out.

It took me a while to wrap my head around this quirky task, but I emerged with this solution:

var robot = function(color) {
    this.color = color;

    this.d1 = 0;
    this.robo1 = 0;
    this.a1 = [];
    while (this.robo1 < 1000 && this.d1 != -1) {

        // all robots begin at 0
        this.robo1 = this.robo1 + this.d1;

        // add starting point to array
        this.a1.push(this.robo1);

        // generate new deltas
        this.d1 = Math.floor(Math.random() * 100);

        // check if adding delta to new robos would exceed 1000
        // to determine whether the next iteration
        this.d1 = (this.d1 + this.robo1 > 1000) ? -1 : this.d1;

    }
    // log all arrays
    console.log(this.a1);

    // determine which element of a1 is closest to half of path
    console.log('smallest distance for a1:');
    this.smallestDistanceFromMiddle = 1000; // setting initial state

    this.cleanedA1 = [...this.a1];
    this.cleanedA1 = this.a1.filter(item => item > 500); // ###
    //console.log('cleanedA1 is \t' + this.cleanedA1);

    this.cleanedA1.forEach((index, item) => {
        this.iteratedValue = Math.abs(500 - this.cleanedA1[item]);
        this.smallestDistanceFromMiddle = this.iteratedValue < this.smallestDistanceFromMiddle ? this.iteratedValue : this.smallestDistanceFromMiddle;
        //console.log(iteratedValue);
    });
    console.log(this.smallestDistanceFromMiddle);
    return this.smallestDistanceFromMiddle;
}

let robot1 = new robot('red');
let robot2 = new robot('purple');
let robot3 = new robot('blue');

console.log("the robots' smallest distance to the middle is:");
console.log(robot1.color, robot1.smallestDistanceFromMiddle + '\n' +
    robot2.color, robot2.smallestDistanceFromMiddle + '\n' +
    robot3.color, robot3.smallestDistanceFromMiddle + '\n');

let nearestRobot = Math.min(robot1.smallestDistanceFromMiddle, robot2.smallestDistanceFromMiddle, robot3.smallestDistanceFromMiddle);

switch (nearestRobot) {
    case robot1.smallestDistanceFromMiddle:
        console.log(robot1.color, 'robot wins! \n');
        break;
    case robot2.smallestDistanceFromMiddle:
        console.log(robot2.color, 'robot wins! \n');
        break;
    case robot3.smallestDistanceFromMiddle:
        console.log(robot3.color, 'robot wins! \n');
        break;
    default:
        console.log('no robots defined');
}

Nevertheless, I feel like there is a more eloquent, easy on the eyes solution and I would very dearly appreciate improvement feedback.

You did not model the race correctly. The specification says:

  All three robots stop once one of them is one pace away from reaching 1km.

In your code, each robot runs the whole distance. The effect is that if you have one fast robot with a constant pace of 90 and one slow robot with a constant pace of 1, your code will make the slow robot win.

But at the time the race is over (after 12 steps), and at this point the slow robot is nowhere near the middle.

(Oh, there's one thing. When the paces are random, how can I know in advance whether the robot is one pace away? That gets complicated, but it is possible.)

Following the specification by the letter, the race has to be simulated very closely to the wording. That is, all robots start at the start, and in each step, each of them advances by its own pace.

Here is how I modeled the race:

'use strict';

class Robot {
    constructor(color, pace) {
        this.color = color;
        this.pace = pace;
        this.position = 0;
    }

    advance(distance) {
        this.position += distance;
    }
}

class Race {
    constructor(distance, robots) {
        this.distance = distance;
        this.middle = distance / 2;
        this.robots = robots;
        this.over = false;
        this.minMiddleDistance = this.middle;
        this.bestRobot = robots[0]; // Technically, it's a tie.
    }

    step() {
        // Calculate the robot's paces for this step in advance,
        // before actually advancing the robot's positions,
        // to satisfy the "one pace away from reaching" requirement
        // in combination with the "each pace is random" requirement.
        const paces = this.robots.map(robot => robot.pace());

        const raceLength = this.distance;
        this.over = this.robots.some((robot, i) => robot.position + paces[i] >= raceLength);
        if (this.over) {
            return;
        }

        this.robots.forEach((robot, i) => {
            robot.advance(paces[i]);
            const middleDistance = Math.abs(robot.position - this.middle);
            if (middleDistance < this.minMiddleDistance) {
                this.minMiddleDistance = middleDistance;
                this.bestRobot = robot;
            }
        })
    }
}

function rnd100() {
    return (100 * Math.random()) | 0; // only integer steps
}

// The specification does not say how each robot's pace is determined.
// The lambda functions in the following code can easily be replaced
// with rnd100 or any other number generator.
const robots = [
    new Robot('red', () => 90),
    new Robot('green', () => 7),
    new Robot('blue', () => 45)
];

const race = new Race(1000, robots);
while (!race.over) {
    race.step();
}
console.log('The winner is ' + race.bestRobot.color + ' with a distance of ' + race.minMiddleDistance + '.');

// Blue wins.
// The red robot crosses the finishing line after ceil(1000/90) == 12 steps.
// After 6 steps, the red robot was closest to the 500 mark, at 540.
// After 11 steps, the blue robot was at 11*45 == 495.
// The green robot is too slow to be a competitor.

I made the Robot class pretty small because the robot should not be involved in calculating the outcome of the race. Who knows, maybe one of them would try to cheat?

All the calculations happen in the Race class, which coordinates the robots.


Analyzer for compression algorithms (or tools) This file contains the main functions of my tool, rac. In particular, I care a lot about the Shannon function.

Would you please give some hints to make the file  cleaner? I have a perversion for concise and good indented code.

#include <math.h>
#include <stdio.h>
#include "headers/utils.h"

double ccount(char * foo)
{
  FILE * file = fopen(foo, "r");
  int a = 0, b;
  while ((b = getc(file)) != EOF)
    a++;

  fclose(file);
  return a;
}

int cmp(double foo, double bar)
{
  if (foo >= bar)
    return 1;
  else
    return 0;
}

double ratio(double foo, double bar)
{
  return (100*(foo - bar)/ foo);
}

void stat(char * file, double vector[256])
  {
    int c;
    FILE * ftopen = fopen(file, "r");
    while ((c = getc(ftopen)) != EOF)
      {
        if (c >= 0 && c <= 256)
            vector[c]++;
      }

      fclose(ftopen);
  }

void freq(char * foo, double vector[256])
{
  int i;
  for (i = 0; i < 256; i++)
  {
    if (vector[i] == 0)
      continue;
    else
      {
        vector[i] = (vector[i] / ccount(foo));
      }
  }
}

double shannon(double vector[256])
{
  int i;
  double shan = 0.0;
  for (i = 0; i < 256; i++)
  {
    if (vector[i] == 0.0)
      continue;
    else
      {
        shan = shan + (log2(pow(vector[i], vector[i])));
      }
  }
  return (shan * -1.0); //entropy of the file
}

MAGIC NUMBERS
The number 256 is used in multiple places as an array size. Rather than using the raw number
256, it would be better to use a symbolic constant. Raw numbers in code are sometimes referred to as magic numbers, because it isn't clear what they mean.
There are at least two reasons for this,

It identifies what the number means.
If at some point in the future the number has to be changed for all uses, the code only  needs to be changed in one place.

An Example in C:

#define ARRAY_SIZE 256

void stat(char * file, double vector[ARRAY_SIZE])
{
    int c;
    FILE * ftopen = fopen(file, "r");
    while ((c = getc(ftopen)) != EOF)
      {
        if (c >= 0 && c <= ARRAY_SIZE)
            vector[c]++;
      }

      fclose(ftopen);
}

An Example in C++:

constexpr int ARRAY_SIZE = 256;

Use Meaningful Variable and Function Names
The code contains foo and bar in a number of places. These are not meaningful names.
The names should indicate what the variables are, so that if you or someone else needs
to change the code 2 years in the future it is an easy task rather than having to go
through all the code to see what needs to be changed.

Another exmaple of this is the function ccount(), which could be renamed file_size().

A third example is the array vector. Other than array if doubles it is not clear what
this array is used for.

Inconsistent Coding Style
In a professional setting where multiple people may have to maintain the code it is best
to be as consistent as possible.

The else clauses almost always have { block}` but the if statements do not.
This is true whether or not the block contains multiple statement or not.
You should choose one way or the other.

In the for loops the { is immediately under the for, but in the else clauses it
is indented. Choose one or the other. The most common usage is how you do the for loops.

This is partially a matter of choice, I use { block } on all control statements:

if ()
{
}
else
{
}

for ()
{
}

while ()
{
}

do
{
} while ();

I do this even for single statements because if the code needs to be updated by adding a statement
to a block it is easier for anyone to see where the line of code should be inserted. This was
also required as part of a coding standard at a company where I worked.

Empty Clauses in IF/ELSE
In the function shannon() the code contains:

    if (vector[i] == 0.0)
      continue;
    else
      {
        shan = shan + (log2(pow(vector[i], vector[i])));
      }

This would be clearer if it was written

    if (vector[i] != 0.0)
      {
        shan = shan + (log2(pow(vector[i], vector[i])));
      }

An else clause can be added at a later time if it is needed. The if portion of the current
implementation is meaningless and would be optimized out by the compiler.


Is it possible to remove variable repetition here? /**
    * Class stringHelperK
    * Helper Class to deal with small string manipulations.
*/
class stringHelperK {

    public function normalizeStringK($string) {
        return $this->removeAccents(
            $this->replaceSpacesWithSlashes($string)
        );
    }

    /**
     * @param $string
     * @return string
     */
    public function removeAccents($string){
        return strtr($string,'àáâãäçèéêëìíîïñòóôõöùúûüýÿÀÁÂÃÄÇÈÉÊËÌÍÎÏÑÒÓÔÕÖÙÚÛÜÝ','aaaaaceeeeiiiinooooouuuuyyAAAAACEEEEIIIINOOOOOUUUUY');
    }

    /**
     * @param $string
     * @return string
     */
    public function replaceSpacesWithSlashes($string) {
        return strtolower(str_replace(" ", "-",$string));
    }
}

I have several questions regarding this code:

$string is used across all class methods. Should it be a property?  If so, how can I transform this accordingly? I mean, I see $string everywhere and it feels isn't correct.

Is it, something like this, better:

/**
    * Class stringHelperK
    * Helper Class to deal with small string manipulations.
*/
class stringHelperK {

    public $string;

    public function normalizeStringK($string) {
        return $this->removeAccents(
            $this->replaceSpacesWithSlashes($this->string)
        );
    }

    /**
     * @param $string
     * @return string
     */
    public function removeAccents($string){
        return strtr($this->string,'àáâãäçèéêëìíîïñòóôõöùúûüýÿÀÁÂÃÄÇÈÉÊËÌÍÎÏÑÒÓÔÕÖÙÚÛÜÝ','aaaaaceeeeiiiinooooouuuuyyAAAAACEEEEIIIINOOOOOUUUUY');
    }

    /**
     * @param $string
     * @return string
     */
    public function replaceSpacesWithSlashes($string) {
        return strtolower(str_replace(" ", "-",$his->string));
    }
}

Or should it be, private $_string, and we should use getters and setters?

This class looks like a utility class, and you could use static functions: you then wouldn't need to instantiate it:

class StringHelperK {

    public static function normalizeStringK($string) {
        return static::removeAccents(
            static::replaceSpacesWithSlashes($string)
        );
    }

    public static function removeAccents($string){
        return strtr($string,'àáâãäçèéêëìíîïñòóôõöùúûüýÿÀÁÂÃÄÇÈÉÊËÌÍÎÏÑÒÓÔÕÖÙÚÛÜÝ','aaaaaceeeeiiiinooooouuuuyyAAAAACEEEEIIIINOOOOOUUUUY');
    }

    public static function replaceSpacesWithSlashes($string) {
        return strtolower(str_replace(" ", "-",$string));
    }
}

To normalize a string, you would then do: StringHelperK::normalizeStringK("blabla").

And no, $string should definitely not be a property, and I see nothing wrong with passing is as an argument to each function.


Data Class code smell? Java object is modified by multiple methods in a row I am sending a series of SOAP messages to a server and capturing the results of the SOAP transaction.  The issue that I am having is that I need to perform five SOAP calls, and each call requires some data from the previous call.  This has led me to create a Data Class that holds Strings accessible with Getters and Setters that gets modified after each call.  This code seems "smelly" to me, there is a lot of state change going on in the class and I'm just wondering if there is a better way to go about this.
Here is what's going on, I have the following method in an abstract class:
protected SoapBodyParameters soapBodyParameters;

public void release(String primaryConfigurationIdentifier) throws ReleaseManagerException {
    this.soapBodyParameters.setPrimaryConfigurationItemIdentifier(primaryConfigurationIdentifier);

    // Implemented by child - Will throw an exception if bad configuration
    verify();

    // Get user full name and append soapBodyParameters
    String userFullName = sendGetUserInfoSoapMessage(this.soapBodyParameters);
    this.soapBodyParameters.setUserFullName(userFullName);

    // Create a release package on the server - no return needed
    sendCreateReleasePackageFromLocalFilesSoapMessage(this.soapBodyParameters);

    // Find the release package ID we just made and append soapBodyParameters
    String releasePackageSystemId = sendGetReleasePackagesSoapMessage(this.soapBodyParameters);
    this.soapBodyParameters.setReleasePackageSystemId(releasePackageSystemId);

    // Open the release package and find the task model information
    TaskModel taskModel = sendOpenReleasePackageSoapMessage(this.soapBodyParameters);
    this.soapBodyParameter.setTaskModelFile(taskModel.getIARTaskModel());
    this.soapBodyParameters.setTaskModelId(taskModel.getTaskModelId());

    // Implemented by child - Update the task model with required info
    updateLocalTaskModel(this.soapBodyParameters);

    // Save the task model to the release package on the server
    sendSaveTaskModelSoapMessage(this.soapBodyParameters);
}

So with each of these calls modifying soapBodyParameters and each following call relying on the previous call to modify soapBodyParameters.... I don't know, it just feels smelly.  Like I am faking a global parameter in the class.
Is there a better way to do this?  I'm trying to get better at recognizing when and where a design pattern would be more appropriate.

Basically, whenever you find yourself doing something like:
MyDataStructure data;

call1(data); // modifies data
call2(data); // modifies data
call3(data); // modifies data

you have a big indicator, that the data structure should indeed be a class:
MyDataClass dataClass;

dataClass.call1();
dataClass.call2();
dataClass.call3();

If the call chain depends on a given order, encapsulate this in the class as well:
MyDataClass {

   public void doComplexTask() {
       call1();
       if (resultIsNotGood) throw Exception...

       call2();
       ...
       // you get it
   }

   private call1() { ... } // not exposed any more
}


JavaScript class that generates HTML form with validation I'm trying to create a complex application—Table generator. And the first part if it is form that lets user input data source. As I figure source should be an absolute http url, so I have to give user <input> with validation, and <button> which will reflect validation state.

Eventually I came up with this:

class Form {
  constructor () {
    this.button            = document.createElement('button')
    this.button.innerHTML  = 'Upload'
    // Initially it's empty so it's invalid
    this.button.disabled   = true

    this.input             = document.createElement('input')
    this.input.placeholder = 'Path to data'
    // Let's assign our custom validator
    // since validation props are read only
    this.input.oninput     = () => this.validate()
  }
  validate () {
    // Tribute @LukeP
    // https://stackoverflow.com/a/34695026/2626747
    // Let's use anchor superpowers
    // rather then google `url regex`
    const a = document.createElement('a')
    a.href  = this.input.value
    // If typed value couldn't be parsed as an absolute url,
    // it'd be considerd as a path on current host
    this.button.disabled = !(a.host && a.host != window.location.host)
  }
  render () {
    const form = document.createElement('form')
    form.appendChild(this.input)
    form.appendChild(this.button)
    return form
  }
}

const form = new Form()
document.getElementById('app').appendChild(form.render())

I'm not satisfied with this code, and I can not explain why. I don't have somewhat strong experience in JS, so I'm relying on your peer review.

Help me make Web better, starting with my app.

I'd just move the render part to the constructor and provide it with the id of the parent element.

class Form {

   constructor( id ) {

      const button = document.createElement('button');
      button.value = "Upload";
      button.disabled = true;

      const input = document.createElement('input');
      input.placeholder = 'Path to data';
      input.oninput = () => this.validate();

      this.input = input;
      this.button = button;

      const form = document.createElement('form');
      form.appendChild(input);
      form.appendChild(button);
      document.getElementById(id).appendChild(form);

   }

   validate () {

      const a = document.createElement('a');
      a.href = this.input.value;
      this.button.disabled = !(a.host && a.host != window.location.host);

   }

}

const form = new Form("app");

It depends what you want to do with the form afterwards.


Count rows of a table I am trying to count the rows of two tables generated by SQL connections in Excel VBA. My plan was to use ListObject.DataBodyRange.Rows.Count however, this errors when a table is empty, which one often is. I found it completely impossible to use On Error GoTo ... twice within one sub so I pulled out the counts as functions like this. I based my error handling pattern on this answer. Is this the best way to go?

Function CountCurrencyTrades() As Integer

    Dim sheetCurrencyTable As Worksheet
    Set sheetCurrencyTable = Worksheets("CurrencyTable")

    Dim tblCurrencyTrades As ListObject
    Set tblCurrencyTrades = sheetCurrencyTable.ListObjects("CurrencyQuery")
    On Error GoTo CurrencyTradesEmpty
    CountCurrencyTrades = tblCurrencyTrades.DataBodyRange.Rows.Count

CleanExit:

    Exit Function

CurrencyTradesEmpty:

    CountCurrencyTrades = 0
    Resume CleanExit

End Function

Just some things that jump out at me:

You don't reset OnError

Huge red flag right there. If you're going to change OnError Goto you should immediately write the corresponding lines to set it back to normal.

Specifically: (Square brackets for emphasis)

    On Error GoTo CurrencyTradesEmpty
    CountCurrencyTrades = tblCurrencyTrades.DataBodyRange.Rows.Count
    [On Error Goto 0]

CleanExit:

    Exit Function

CurrencyTradesEmpty:
    [On Error Goto 0]
    CountCurrencyTrades = 0
    Resume CleanExit

Your function has too many responsibilities

Single Responsibility Principle. Your function has a hardcoded worksheet, hardcoded listObject and then handles the counting of said object. I would have the function take a ListObject as an argument and let another function handle the retrieval of said objects. Like so:

    Option Explicit

    Public Function GetCurrencyTradesCount() As Long

        Dim sheetCurrencyTable As Worksheet
        Set sheetCurrencyTable = Worksheets("CurrencyTable")

        Dim tblCurrencyTrades As ListObject
        Set tblCurrencyTrades = sheetCurrencyTable.ListObjects("CurrencyQuery")

        GetCurrencyTradesCount = CountListRows(tblCurrencyTrades)

    End Function

    Private Function CountListRows(ByRef targetList As ListObject) As Long
        '/ On Error (I.E. empty list), returns 0

        On Error GoTo CleanFail
        CountListRows = targetList.DataBodyRange.Rows.Count
        On Error GoTo 0

CleanExit:
        Exit Function

CleanFail:
        On Error GoTo 0
        CountListRows = 0
        Resume CleanExit

    End Function

Scoping

Your function is implicitly public. You should explicitly set a Public or Private scope for it.

Integer

Is officially deprecated. The compiler will silently convert all Integers to Longs, so just use Long instead.

Error Handling

Do you really want your function to return 0 for all errors? What if it's given an empty object reference? I would rename your label to CleanFail: and think about what other failure scenarios it might want to deal with.


HTTP GET requests to API I've written a variety of libraries that query and work with data from APIs. While the classes and objects being passed around vary from project to project, I'm generating GET requests in the same manner in each project.

I have a basic method to generate a GET request:

private string GenerateWebRequest(string conn)
{
    try
    {
        using (var webClient = new WebClient())
        {
            return webClient.DownloadString(conn);
        }
    }
    catch (Exception e)
    {
        Console.WriteLine("There was a problem generating the web request to the API server.");
        Console.WriteLine(e.StackTrace);
        return string.Empty;
    }
}

To make life easier and avoid code repetition, I create a Config object in the primary constructor for each library, which looks something like:

public class Config
{
    public Config()
    {
        localhostNearest = string.Empty;
        localhostRoute = string.Empty;
        localhostTrip = string.Empty;
        processLocation = string.Empty;
        workingDirectory = string.Empty;
        datasource = string.Empty;
    }

    public string localhostNearest { get; set; }

    public string localhostRoute { get; set; }

    public string localhostTrip { get; set; }

    public string processLocation { get; set; }

    public string workingDirectory { get; set; }

    public string datasource { get; set; }
}

And the instantiation of the Config object:

Config cfg = new Config
{
    localhostNearest = "http://localhost:5000/nearest/v1/driving/",
    localhostRoute = "http://localhost:5000/route/v1/driving/",
    localhostTrip = "http://localhost:5000/trip/v1/driving/",
    processLocation = installationFolder + "\\osrm-routed.exe",
    workingDirectory = workingFolder + @"data\"
};

The string conn that gets passed as a parameter to the GenerateWebRequest() method is generated in individual methods, dependent on what should be queried and then deserialized (each response is a JSON string). A sample conn generation would be:

string response = GenerateWebRequest($"{_config.localhostNearest}{lat},{lon}?apiKey={key}");

which would come out as:

http://localhost:5000/route/v1/nearest/-3.3431053161621094,52.50671376947057?apiKey=XXXXX

The connection string is typically made up of three parts:

  Base URL. Such as http://localhost:5000/api/
  User query, which part of the API is being queried.
  API key appended to the end of the URL.

I feel like there is a cleaner way of doing this in each project, and some methods end up being overly verbose due to the creation of the URL.

Are there any recommendations that could be made to improve this?

Why are there Console.WriteLine calls? Use a logging library instead of binding this to a specific usage.

Also, returning string.Empty; instead of throwing an exception seems a bit of a bad idea. You wouldn't know there's an issue unless you'd look at your logs.

Properties should be PascalCase: localhostNearest, etc.

Why bother with this in the constructor?

localhostNearest = string.Empty;

Why can't it be null?

Do not concatenate when working with paths:

processLocation = installationFolder + "\\osrm-routed.exe",
workingDirectory = workingFolder + @"data\"

Instead, use Path.Combine().

GenerateWebRequest() does more than what its name implies.

What is conn? Use descriptive names for variables, parameters, properties, etc.

There is a lot of repetition here:

localhostNearest = "http://localhost:5000/nearest/v1/driving/",
localhostRoute = "http://localhost:5000/route/v1/driving/",
localhostTrip = "http://localhost:5000/trip/v1/driving/",

If all URLs follow that pattern, why not work with a "template" -- e.g. $"http://localhost:5000/{type}/{version}/driving/" -- and use string interpolation or String.Format()?

Also, why are these hardcoded in your code? Shouldn't these URLs come from a .config file?


Merge the two sorted array of numbers into single array I have written a code were I pass two sorted array of numbers from command line, and out is one array with numbers sorted.

the code is as follows,

(function(){
    'use strict';

    var getCommandLineVariables = getCommandLineVariables;

    var mainFunction = mainFunction;

    mainFunction();

    function mainFunction(){
        var array = getCommandLineVariables();
        var firstArray = array[0],
            secondArray = array[1],
            mergedArray = [], i = 0, j = 0;

        for(i = 0; i < firstArray.length; i++){
            for(j = j; j < secondArray.length; j++){

                if(firstArray[i] < secondArray[j]){ // if number from first array is less than number of second array
                    mergedArray.push(firstArray[i]);
                    break;
                }else{ // if number of second array is less than number of first array
                    mergedArray.push(secondArray[j]);
                }

            } // end of for

            /*
                The below condition executes when
                length of first array is greater than second array
            */
            if(j >= secondArray.length){
                mergedArray.push(firstArray[i]);
            }
        } // end of for

        /*
            If length of second array is greater than
            first array
        */
        for(j = j; j < secondArray.length;j++){
            mergedArray.push(secondArray[j])
        }

        console.log(mergedArray);
    } // mainFunction

    /*
        Capture the numbers passed as commandline arguments
    */
    function getCommandLineVariables(){
        var arg_array = process.argv.slice(2),
            someArray = [];

        if(arg_array.length !== 2){
            throw "Pass two comma seperated numbers";
        }

        for(var i = 0; i < arg_array.length; i++){

            someArray.push((function(someString){
                var someArray = someString.split(",");

                someArray = someArray.map(function(str){
                    return parseInt(str);
                });

                return someArray;
            })(arg_array[i]));

        } // end of for

        return someArray;
    } // end of getCommandLineVariables
})();

The output of the above code is as follows,

E:\DataStructuresAndAlgorithms\array>node mergeSortedArray01.js 1,3,100 2,4,5,7,10
[ 1, 2, 3, 4, 5, 7, 10, 100 ]

E:\DataStructuresAndAlgorithms\array>

One more output

E:\DataStructuresAndAlgorithms\array>node mergeSortedArray01.js 3,27,100 2,4,5,7,10
[ 2, 3, 4, 5, 7, 10, 27, 100 ]

E:\DataStructuresAndAlgorithms\array>

Can you please review my code a check is there any room for improvement,
Can the above problem be solved in other ways.

Walkthrough
Let me walk through the code with you and see what can be improved.
    var getCommandLineVariables = getCommandLineVariables;

    var mainFunction = mainFunction;

These are already available in your scope, so you do not need to reassign them to a local variable with the same name.
            mergedArray = [], i = 0, j = 0;

        for(i = 0; i < firstArray.length; i++){
            for(j = j; j < secondArray.length; j++){

You initialize i and j, then you reinitialize i in the outer for loop, then do nothing with j in the inner one.  You don't need to write an initialization step in the for loop if you don't want to.
            throw "Pass two comma seperated numbers";

Here you throw a string.  You can throw any value, but it's more helpful for debugging purposes if you throw an Error object.  Just pass this string into the Error constructor to make your own error message.
            someArray.push((function(someString){
                var someArray = someString.split(",");

                someArray = someArray.map(function(str){
                    return parseInt(str);
                });

                return someArray;
            })(arg_array[i]));

You pass an immediately invoked function expression (IIFE) to the push method, but this is totally unnecessary.  You can execute the code above the push method and make things considerably more readable.  Also, you redeclare the variable someArray inside this IIFE and use it for something else, which is pretty confusing.  It'd be better to use a different variable name.
Time Complexity
Your current algorithm is O(n2) because of the nested for loops.  A more efficient algorithm would be to step through both arrays at the same time, adding the next smallest element to the merged array.  This approach would be O(n), so an improvement!
Changes
Altogether, this is how I would restructure your existing code:
(function(){
    'use strict';

    mainFunction();

    function mainFunction(){
        var array = getCommandLineVariables();
        var firstArray = array[0],
            secondArray = array[1],
            mergedArray = [], i = 0, j = 0;

        for(; i < firstArray.length; i++){
            for(; j < secondArray.length; j++){

                if(firstArray[i] < secondArray[j]){ // if number from first array is less than number of second array
                    mergedArray.push(firstArray[i]);
                    break;
                }else{ // if number of second array is less than number of first array
                    mergedArray.push(secondArray[j]);
                }

            } // end of for

            /*
                The below condition executes when
                length of first array is greater than second array
            */
            if(j >= secondArray.length){
                mergedArray.push(firstArray[i]);
            }
        } // end of for

        /*
            If length of second array is greater than
            first array
        */
        for(; j < secondArray.length;j++){
            mergedArray.push(secondArray[j])
        }

        console.log(mergedArray);
    } // mainFunction

    /*
        Capture the numbers passed as commandline arguments
    */
    function getCommandLineVariables(){
        var arg_array = process.argv.slice(2),
            someArray = [];

        if(arg_array.length !== 2){
            throw new Error("Pass two comma separated numbers");
        }

        for(var i = 0; i < arg_array.length; i++){
            var inputArray = arg_array[i].split(",");

            inputArray = inputArray.map(function(str){
                return parseInt(str);
            });

            someArray.push(inputArray);
        } // end of for

        return someArray;
    } // end of getCommandLineVariables
})();

And this is how I would implement the more efficient algorithm I described above:
function mainFunction(){
    var array = getCommandLineVariables();
    var firstArray = array[0],
        secondArray = array[1],
        mergedArray = [], i = 0, j = 0;

    while(i < firstArray.length && j < secondArray.length) {
        if(firstArray[i] < secondArray[j]) {
            mergedArray.push(firstArray[i]);
            i++;
        } else {
            mergedArray.push(secondArray[j]);
            j++;
        }
    }

    while(i < firstArray.length) {
        mergedArray.push(firstArray[i]);
        i++;
    }

    while(j < secondArray.length) {
        mergedArray.push(secondArray[j]);
        j++;
    }

    console.log(mergedArray);
} // mainFunction


Simple string scrambling Task: Take every 2nd char from the string, then the other chars, that are not every 2nd char, and concat them as new String.
Do this n times!

Examples:

"This is a test!", 1 -> "hsi  etTi sats!"
"This is a test!", 2 -> "hsi  etTi sats!" -> "s eT ashi tist!"

Code:

const split = (text) => {
  const firstPart = text.split('').filter((element, index) => index % 2 != 0).join('')
  const secondPart = text.split('').filter((element, index) => index % 2 == 0).join('')

  return firstPart + secondPart
}

const encrypt = (text, n) => {
  if (text == null) {
    return null
  }

  let result = text

  for (let i = 0; i < n; i++) {
    result = split(result)
  }

  return result
}

const decrypt = (text, n) => {
  if (text == null) {
    return null
  }

  for (let i = 0; i < n; i++) {
    let firstPartLength = text.length % 2 == 0 ? text.length / 2 : (text.length - 1) / 2 + 1
    let secondPartLenght = text.length % 2 == 0 ? text.length / 2 : (text.length - 1) / 2
    let firstPart = text.substring(0, firstPartLength)
    let secondPart = text.substring(secondPartLenght, text.length)

    secondPart = secondPart.split('').map(element => element + ' ').join('')
    let position = -1
    secondPart = secondPart.split('').map((element, index) => index % 2 != 0 ? firstPart[++position] : element).join('').substring(0, text.length)

    text = secondPart
  }

  return text
}

console.log(decrypt(encrypt('This is a test!', 3), 3))

console.log(encrypt('Some text!', 4))

const split = (text) => (text.replace(/.(.)?/g, '$1') + ("d"+text).replace(/.(.)?/g, '$1')

The regex matches every other char. Do it twice, add any arbitrary char to the start of the string the second time to offset it.


Piling Up with Python This is my solution for a Hackerrank problem. The problem was:

  There is a horizontal row of cubes. The length of each cube is given. You need to create a new vertical pile of cubes. The new pile should follow these directions: if cube_i is on top of cube_j then sideLength_j >= sideLength_i.

  When stacking the cubes, you can only pick up either the leftmost or the rightmost cube each time. Print "Yes" if it is possible to stack the cubes. Otherwise, print "No". Do not print the quotation marks.

  Input Format

  The first line contains a single integer T, the number of test cases.
  For each test case, there are 2 lines.
  The first line of each test case contains , the number of cubes.
  The second line contains space separated integers, denoting the sideLengths of each cube in that order.

  Constraints

1 <= T <= 5
1 <= n <= 10**5
1 <= sideLength <= 2**31

  Output Format

  For each test case, output a single line containing either "Yes" or "No" without the quotes.

  Sample Input

2
6
4 3 2 1 3 4
3
1 3 2

  Sample Output

Yes
No

  Explanation

  In the first test case, pick in this order: left - 4, right - 4, left - 3, right - 3, left - 2, right - 1.
  In the second test case, no order gives an appropriate arrangement of vertical cubes. 3 will always come after either 1 or 2.

My code is actually rather short:

if __name__ == '__main__':
    num_tests = int(input().strip())

    for _ in range(num_tests):
        num_cubes = int(input().strip())
        cube_sidelengths = list(map(int, map(str.strip, input().strip().split())))

        i, j = 0, len(cube_sidelengths)-1
        pile = []

        while i != j:
            left, right = cube_sidelengths[i], cube_sidelengths[j]
            # In case there is nothing in the pile just add the bigger one
            if not pile:
                if left > right:
                    pile.append(left)
                    i += 1
                else:
                    pile.append(right)
                    j -= 1
            else:
                # Take the bigger of the left and right element and compare it
                # to the last item on the pile. If it's bigger we can't pile
                # it up, otherwise put it on the pile and continue
                if left > right:
                    if left > pile[-1]:
                        print('No')
                        break
                    else:
                        pile.append(left)
                        i += 1
                else:
                    if right > pile[-1]:
                        print('No')
                        break
                    else:
                        pile.append(right)
                        j -= 1
        else:
            print('Yes')

But I'm actually not very satisfied with the code. It passes all Hackerrank tests but I can't help feeling that it's missing something - but I can't put my finger on it.

An additional thing that has been bugging me was that it was part of the  "Python collections" problems but I haven't used anything from the collections module. It would be possible to use a deque here but that felt like overkill - because pop and popLeft are O(1) but just keeping the indices and accessing a list is also O(1) so it wouldn't even provide better asymptotic behaviour. So as additional question: Is there anything that could be improved by using a deque?

Otherwise I would be very glad about any feedback how I could improve the code.

The algorithm is actually pretty efficient, so congrats for that.

The first thing that I'd like to suggest is moving the logic outside the main check. In python, it's fairly common (and IMO more readable) to have your structure like this:

imports

def pile_algo(*args, **kwargs):
    """Docstring here"""
    # your code

def main():
    """Docstring here"""
    # process pile_algo

if __name__ == '__main__':
    main()

Of course, the main() is optional if you already have all the logic inside pile_algo().

Even if I said the algorithm is good, we can improve the way you're doing things.

You're not using num_cubes variable, so you can remove it (int(input().strip()) it's enough). Moving forward, neither use of strip() or list() or map() is necessary in your:

cube_sidelengths = list(map(int, map(str.strip, input().strip().split())))

You can rewrite that as:

cube_sidelengths = [int(x) for x in input().split()]

Apart from that, I don't have any other reasons to think about using deque. If it works, it works. And apparently works better enough to get over it.

Though, of course, I would also ask: Is it too slow right now? Most of the times, the aim of a programming challenge is both getting familiar with a specific library/module and improve your algorithms skills. You said:

  It would be possible to use a deque here but that felt like overkill -
  because pop and popLeft are O(1) but just keeping the indices and
  accessing a list is also O(1) so it wouldn't even provide better
  asymptotic behavior.

Well, it would provide a worse asymptotic behavior? Just start writing a deque-based solution and see what improvements might that bring. I can think of a few right now:

the code will be more comprehensive because of deque's already builtin methods which might do what you manually did (pop, etc..)
talking about efficiency, Python lists are much better for random-access and fixed-length operations, including slicing, while deques are much more useful for pushing and popping things off the ends, with indexing (but not slicing, interestingly) being possible but slower than with lists.

From the docs:

  Deques support thread-safe, memory efficient appends and pops from
  either side of the deque with approximately the same O(1) performance
  in either direction.

  Though list objects support similar operations, they are optimized for
  fast fixed-length operations and incur O(n) memory movement costs for
  pop(0) and insert(0, v) operations which change both the size and
  position of the underlying data representation.

This might look more of a theory answer, and might not specifically answer your question, but I just wanted to highlight a couple of things out.


Formatting floating point number as mixed fractional in Ruby I have to format numbers that are either whole numbers or mixed fractional numbers with quarters (1/4, 1/2, 3/4) or thirds (1/3, 2/3).

The numbers are given as floats, for example 1.3333333 or 1.5 and are therefore inaccurate (but always close to a quarter or a third).

The formatting method is supposed to return Unicode fractions, for example:

1.0    #=> 1
1.3333 #=> 1⅓
1.5    #=> 1½
1.6667 #=> 1⅔

My current approach is to format the number via sprintf and to apply a second formatting based on the result:

def format_fraction(number)
  fix, frac = sprintf('%0.2f', number).split('.')
  case frac
  when '00' then "#{fix}"
  when '25' then "#{fix}¼"
  when '33' then "#{fix}⅓"
  when '50' then "#{fix}½"
  when '67' then "#{fix}⅔"
  when '75' then "#{fix}¾"
  end
end

Is there a better / less string-based / more numeric way to solve this?

[Answering my own question here]

Following Marc Rohloff's suggestion, I'm using Float#rationalize with an argument of 0.01 to convert the floats to rational numbers:

1.0.rationalize(0.01)
#=> (1/1)

1.3333.rationalize(0.01)
#=> (4/3)

1.5.rationalize(0.01)
#=> (3/2)

1.6666.rationalize(0.01)
#=> (5/3)

divmod can then be used to get the fixed and fractional part:

(5/3r).divmod(1)
#=> [1, (2/3)]

Applied to my method:

def format_fraction(number)
  fix, frac = number.rationalize(0.01).divmod(1)
  case frac
  when 0/1r then "#{fix}"
  when 1/4r then "#{fix}¼"
  when 1/3r then "#{fix}⅓"
  when 1/2r then "#{fix}½"
  when 2/3r then "#{fix}⅔"
  when 3/4r then "#{fix}¾"
  end
end

format_fraction(1.0)    #=> "1"
format_fraction(1.3333) #=> "1⅓"
format_fraction(1.5)    #=> "1½"
format_fraction(1.6666) #=> "1⅔"

It's not a huge change, but the when arguments now resemble the output more closely, which makes the code easier to understand IMO.

The method could be further shortened by extracting the rational => string pairs:

FRACTIONS = { 1/4r => '¼', 1/3r => '⅓', 1/2r => '½', 2/3r => '⅔', 3/4r => '¾' }

def format_fraction(number)
  fix, frac = number.rationalize(0.01).divmod(1)
  [fix, FRACTIONS[frac]].join
end


Parsing website info with a database and web service I have to make project for school in C# which has to use database and web service. I have made a program which starts a web service and gets a function from there which is used for connecting to a website. I am using HTML Agility Pack for parsing info from that website. Website info is then written in textbox and stored in database.

Here is one function from project, and the rest is here.

    public void HtmlParser()
    {
        Encoding enc = Encoding.UTF8;

        HtmlAgilityPack.HtmlDocument htmlDoc = new HtmlAgilityPack.HtmlDocument();

        htmlDoc.Load(client.Request(), enc);

        HtmlNode root = htmlDoc.DocumentNode;
        HtmlNodeCollection jobNode = root.SelectNodes("//div[@class='jobBox']");
        HtmlNodeCollection headerNode = root.SelectNodes("//div[@class='jobBox']/h1");
        HtmlNodeCollection contentNode = root.SelectNodes("//div[@class='jobBox']/div[@class='content']");
        HtmlNodeCollection linkNode = root.SelectNodes("//div[@class='jobBox']/a");

        jobs = new List<string>();
        headers = new List<string>();
        content = new List<string>();
        links = new List<string>();
        full_links = new List<Uri>();

        foreach (HtmlNode node in jobNode)
        {
            jobs.Add(node.InnerText);
        }
        foreach (HtmlNode node in headerNode)
        {
            headers.Add(node.InnerText.Trim());
        }
        foreach (HtmlNode node in contentNode)
        {
            content.Add(node.InnerText.Trim());
        }

        foreach (HtmlNode node in linkNode)
        {
            links.Add(node.GetAttributeValue("href", null).Trim());
            Uri temp = new Uri(link + node.GetAttributeValue("href", null).Trim());
            full_links.Add(temp);
        }

        htmlDoc.Save("file.htm", enc);

        StringParser();
    }

Some variables are in Croatian, so don't get confused. I am looking for helpful reviews and tips on how I can make my code better.

One comment I'd have is that there is a LOT going on in that method... but for the most part
each section is the same...

GetNodeCollection
Loop Through it
Add it to a list

You could probably pull that out into a method to shorten your code.

public List<string> ParseSection(HtmlNode root, string nodePath, bool isLink = false)
{
    HtmlNodeCollection nodes = root.SelectNodes(nodePath);
    List<string> parsedNodes = new List<string>();
    foreach (HtmlNode node in nodes)
    {
        if(isLink)
        {
            parsedNodes.Add(node.GetAttributeValue("href", null).Trim());
        }
        else
        {
            parsedNodes.Add(node.InnerText.Trim());
        }
    }
    return parsedNodes;
}

public void HtmlParser()
{
    Encoding enc = Encoding.UTF8;
    HtmlAgilityPack.HtmlDocument htmlDoc = new HtmlAgilityPack.HtmlDocument();
    htmlDoc.Load(client.Request(), enc);
    HtmlNode root = htmlDoc.DocumentNode;
    HtmlNodeCollection linkNode = root.SelectNodes();

    List<string> jobs = ParseSection(root, "//div[@class='jobBox']");
    List<string> headers = ParseSection(root, "//div[@class='jobBox']/h1");
    List<string> content = ParseSection(root, "//div[@class='jobBox']/div[@class='content']");
    List<string> links = ParseSection(root, "//div[@class='jobBox']/a", true); //TRUE
    List<Uri> fullLinks = links.Select(l => new Uri(l));

    htmlDoc.Save("file.htm", enc);
    StringParser();
}


Find the minimum number of operations to convert 1 into n, and print the sequence of numbers Here is my solution for the following task: Given an integer n, print the minimum number of operations needed to obtain the number n starting from the number 1, and the sequence of numbers towards the number n. The allowed operations are: multiply by 2; multiply by 3; add 1:

def min_operations(x, y):
    visited = set()
    sequences = [[x]]

    while True:
        new_sequences = []
        for s in sequences:
            val = s[-1]

            if val == y:
                return s

            val_times_2 = val * 2
            if val_times_2 <= y and val_times_2 not in visited:
                visited.add(val_times_2)
                new_sequences.append(s + [val_times_2])

            val_times_3 = val * 3
            if val_times_3 <= y and val_times_3 not in visited:
                visited.add(val_times_3)
                new_sequences.append(s + [val_times_3])

            val_plus_1 = val + 1
            if val_plus_1 <= y and val_plus_1 not in visited:
                visited.add(val_plus_1)
                new_sequences.append(s + [val_plus_1])

        sequences = new_sequences

if __name__ == '__main__':
    y = int(input())
    mo = min_operations(1, y)
    print(len(mo) - 1)
    print(mo)

Examples:

> 1
0
1

> 5
3
1 2 4 5

> 96234
14
1 2 6 7 21 22 66 198 594 1782 5346 16038 16039 32078 96234

Is there a more efficient and/or maintainable way of doing that? Perhaps usual algorithms for doing this and similar tasks? Is the code alright?

Given the problem description, I'm not entirely sure why x is a parameter of min_operations. Could you not hard-code 1?

        val_times_2 = val * 2
        if val_times_2 <= y and val_times_2 not in visited:
            visited.add(val_times_2)
            new_sequences.append(s + [val_times_2])

        val_times_3 = val * 3
        if val_times_3 <= y and val_times_3 not in visited:
            visited.add(val_times_3)
            new_sequences.append(s + [val_times_3])

        val_plus_1 = val + 1
        if val_plus_1 <= y and val_plus_1 not in visited:
            visited.add(val_plus_1)
            new_sequences.append(s + [val_plus_1])

There's some common code here which could certainly be refactored. Perhaps

        for successor in [val + 1, val * 2, val * 3]
            if successor <= y and successor not in visited:
                visited.add(successor)
                new_sequences.append(s + [successor])

However, the approach of storing the full path for each visited element doesn't scale particularly well. All you really need is to find the predecessor: provided you store all of the predecessors, you can work your way back up the chain. One way to do that would be a dictionary (which could also take the place of visited).

def min_operations(y):
    predecessors = dict()
    new_elements = [1]

    while True:
        next_new_elements = []
        for e in new_elements:
            if e == y:
                return unchain(e, predecessors)

            for successor in [val + 1, val * 2, val * 3]
                if successor <= y and successor not in predecessors:
                    predecessors[successor] = y
                    next_new_elements.append(successor)

        new_elements = next_new_elements

def unchain(e, predecessors):
    chain = [e]
    while e in predecessors:
        e = predecessors[chain[-1]]
        chain.append(e)
    return list(reversed(chain))

An alternative way of doing it would be to use lru_cache to find the predecessors on the fly, handing off the caching to the library. Here it is necessary to track the depth as well as the predecessor.

@lru_cache(maxsize=None)
def predecessor(y):
    if y == 1:
        return (0, 0)

    candidates = []
    (decr, _) = predecessor(y - 1)
    candidates.append((decr + 1, y - 1))

    for divisor in [2, 3]:
        if y % divisor == 0:
            (divided, _) = predecessor(y // divisor)
            candidates.append((divided + 1, y // divisor))

    return min(candidates)

Then min_operations is essentially unchain but calling predecessor instead of doing a lookup in a map. This is arguably slightly more elegant, but does run into problems with stack overflow if y is too large.


Storing singleton instances in a generic utils class In one of my java projects I use singletons a lot and I was looking for a way to reduce repeating code. I made a SingletonUtils class that accepts a class and supplier parameter. Then stores the instance in a hash map. This way the static instance isn't stored as a static variable in the class I want to make a singleton.

My question is, is there any problems/cons with implementing the singleton pattern like this and is there an easier way to do this or is this the best way?

public class SingletonUtils {
    public static HashMap<Class, Object> instances = new HashMap<>();
    public static <T> T getInstance(final Class<T> singletonObj, final Supplier<T> creator) {
        if(!instances.containsKey(singletonObj)) {
            if(creator == null) return null;
            else instances.put(singletonObj, creator.get());
        }
        return (T) instances.get(singletonObj);
    }

    public static boolean instanceExists(final Class singletonObj) {
        return instances.containsKey(singletonObj);
    }

    public static void removeInstance(final Class singletonObj) {
        instances.remove(singletonObj);
    }
}

public final class AuthManager {
    private AuthManager() { }

    public static AuthManager getNewInstance() {
        return SingletonUtils.getInstance(AuthManager.class, AuthManager::new);
    }
}

Let me just express the first thing that came to mind when I read this question's title:

  Not another Singleton question.

Let's ignore in the following that Singletons are an anti-pattern in Object-Oriented Programming.

Look. I get it. Singletons are nice and easy.
Everything about them makes things easier to think about.
The whole point of a singleton is that only one instance of it ever exists.
It's by definition the single source of truth.

What your code does is subtly, but completely undermine that assumption.

Consider the following sequence:

AuthManager authSingleton = AuthManager.getNewInstance();
SingletonUtils.removeInstance(AuthManager.class),
AuthManager anotherSingleton = AuthManager.getNewInstance();

Those don't need to be in the same thread, the same class or the same anything. As long as they run in the same JVM, authSingleton == anotherSingleton will be false. And this is a complete and utter breakage of the semantics of a Singleton.

What you tried to do with that class is not something that you want. You're centrally controlling the lifetime of singletons. But you're not correctly enforcing the semantics of these singletons.

This code is something like an ObjectPool. Implying that it has anything to do with singletons is incorrect though.

Do not make singletons complicated:

public final class AuthManager {
    private static final AuthManager INSTANCE = new AuthManager();
    public static AuthManager get() {
        return INSTANCE;
    }

    private AuthManager() {
    }
}

^^ This is a complete, threadsafe and lazily initialized Singleton. In 99.9999% of the use-cases of a singleton, this is enough. But 90% of all Singleton uses are just flat out wrong.

There is no useful reason for an "AuthManager" to be a Singleton. Don't cargo-cult.


Utility object for setTimeout I was using setTimeouts for a project where a callback will be called after a duration, but the user has the options of extending, pausing or stopping the timeout. I felt using the default setTimeout was a bit clunky, especially since I needed to remember the timeout ids, and I decided to create a timeout utility object. I was wondering if I'm doing this properly, and whether you could give me some tips on how to refactor my code?

Object definition

//Utility object that makes it easier to control timeouts
function Timer(duration, callback) {
    this._duration = duration;
    this._callback = callback;
    this._id = -1;

    this._paused = true;
    this._lastRunTime = -1;
    this._remainingTime = this._duration;
}

//calls the current timeout; if a function is passed, run that function
//instead of default callback (but do not change the default callback)
Timer.prototype.callTimer = function(aCallback, aDuration) {
    var that = this,
        //sets callback and duration to default values if not passed in
        callback = aCallback || this._callback,
        duration = aDuration || this._duration;

    //if a previous timeout has been set, clear it
    if (this._id > -1 ) {
        clearTimeout(this._id);
    }
    //executes the callback and resets the timer
    this._id = setTimeout(function() {
        callback();
        that.resetTimer();
    }, duration);

    this._paused = false;
    //rememebers the time when the timer was run (for pauseTimer)
    this._lastRunTime = new Date();
}

Timer.prototype.resetTimer = function() {
    this._remainingTime = this._duration;
    this._paused = true;
    this._lastRunTime = -1;
    clearTimeout(this._id);
}

//changes the default callback
//if second argument is true, execute callTimer
Timer.prototype.changeCallback = function(callback, callTimer) {
    this._callback = callback;
    if (callTimer) this.callTimer();
}

Timer.prototype.pauseTimer = function() {
    if (this._lastRunTime === -1) {
        throw new Error('Timer has not been run yet');
    }

    //if currently paused, call timer with remaining time left
    if (this._paused) {
        this._lastRunTime = new Date();
        this.callTimer(this._callback, this._remainingTime);
    }
    //else pause
    else {
        clearTimeout(this._id);
        //subtract the amount of time that has elapsed
        this._remainingTime -= (new Date() - this._lastRunTime);
    }

    //toggles the _paused variable
    this._paused = !this._paused;
}

http://fiddle.jshell.net/6nhGW/

Test Code

var button = document.getElementById('extendTimeout'),
    button2 = document.getElementById('tempChangeCallback'),
    button3 = document.getElementById('changeCallback'),
    button4 = document.getElementById('resetTimer'),
    button5 = document.getElementById('pauseTimer'),
    startTime,
    timeout = -1;

var timer = new Timer(3000, function() {
    console.log('Total time elapsed: ' + (new Date() - startTime));
    startTime = null;
})

button.addEventListener('click', function() {
    if (!startTime) startTime = new Date();
    timer.callTimer();
})

button2.addEventListener('click', function() {
    if (!startTime) startTime = new Date();
    timer.callTimer(function() {
        console.log('Temporarily change callback');
        console.log('Total time elapsed: ' + (new Date() - startTime));
        startTime = null;
    });
})

button3.addEventListener('click', function() {
    timer.changeCallback(function() {
        console.log('Callback has been changed!');
        startTime = null;
    }, true);
});

button4.addEventListener('click', function() {
    timer.resetTimer();
    startTime = null;
});

button5.addEventListener('click', function() {
    if (!startTime) startTime = new Date();
    timer.pauseTimer();
});

From a once over:

I would not use _ to prefix all your variables, it would make more sense to either actually use private variables or write a comment that this variables should not be touched by callers
pauseTimer lies, since you toggle. I would call it toggleTimer or startStopTimer
Your code is well commented except for //toggles the _paused variable which is kind of obvious ;)
Besides some missing semicolons, your code looks fine on JsHint.com
I think in callTimer you could have called your paramater callback, then do callback = callback || this.callback and then use callback in your closure.

All in all, good, re-usable code.

For my own purposes I would probably make the private variables really private and move the functions out of prototype into the constructor. That would increase memory footprint, but I would never use more than a dozen of these at the same time.


Using Java stream to filter a list and create a map In List<Hotel>, I'd like to extract Hotels which has OTAProperty with a specific otaName, as Map<Long, Hotel>. The map's key is the id for the OTAProperty named given otaName, and the value for the map is the Hotel instance.

I made a method createHotelMapByOtaName working as intended. But I think it's possible to remove nested forEach block by using Java 8's stream and related operations such as map, flatMap, and Collectors.toMap. Can anyone get some advice or code snippets?

package com.example;

import com.google.common.collect.Lists;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class HotelService {

    public static Map<Long, Hotel> createHotelMapByOtaName(List<Hotel> hotels, String otaName) {
        Map<Long, Hotel> map = new HashMap<>();
        hotels.forEach(h -> {
            h.getOtaProperties().forEach(op -> {
                if (otaName.equals(op.getOtaName())) map.put(op.getId(), h);
            });
        });
        return map;
    }

    public static void main(String[] args) {
        List<Hotel> hotels = Lists.newArrayList(
                new Hotel("Hilton",
                        Lists.newArrayList(
                                new OTAProperty(1L, "expedia"),
                                new OTAProperty(2L, "agoda"))),
                new Hotel("Ramada",
                        Lists.newArrayList(
                                new OTAProperty(4L, "agoda"))),
                new Hotel("Hyatt",
                        Lists.newArrayList(
                                new OTAProperty(5L, "expedia")))
        );
        // expected: {1=Hotel{name=Hilton, otaProperties=[OTAProperty{id=1, otaName=expedia}, OTAProperty{id=2, otaName=agoda}]}, 5=Hotel{name=Hyatt, otaProperties=[OTAProperty{id=5, otaName=expedia}]}}
        System.out.println(HotelService.createHotelMapByOtaName(hotels, "expedia"));
    }
}

Followings are codes for 2 models:

Hotel.java:

package com.example;

import com.google.common.base.MoreObjects;

import java.util.List;

public class Hotel {
    private String name;
    private List<OTAProperty> otaProperties;

    public Hotel(String name, List<OTAProperty> otaProperties) {
        this.name = name;
        this.otaProperties = otaProperties;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public List<OTAProperty> getOtaProperties() {
        return otaProperties;
    }

    public void setOtaProperties(List<OTAProperty> otaProperties) {
        this.otaProperties = otaProperties;
    }

    @Override
    public String toString() {
        return MoreObjects.toStringHelper(this)
                .add("name", name)
                .add("otaProperties", otaProperties)
                .toString();
    }
}

OTAProperty.java:

package com.example;

import com.google.common.base.MoreObjects;

public class OTAProperty {
    private Long id;
    private String otaName;

    public OTAProperty(Long id, String otaName) {
        this.id = id;
        this.otaName = otaName;
    }

    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }

    public String getOtaName() {
        return otaName;
    }

    public void setOtaName(String otaName) {
        this.otaName = otaName;
    }

    @Override
    public String toString() {
        return MoreObjects.toStringHelper(this)
                .add("id", id)
                .add("otaName", otaName)
                .toString();
    }
}

This transformation is not so obvious, but still feasible without instantiating a dedicated Map directly and without using forEach.

First of all, I suggest to implement hashCode and equals in Hotel and OTAProperty classes: this is often useful, especially when storing objects in collections.

Here is how the functional sequence can look like:

public static Map<Long, Hotel> createHotelMapByOtaName2(List<Hotel> hotels, String otaName) {
  return hotels.stream()
             .map(hotel ->
                hotel.getOtaProperties().stream()
                     .filter(op -> otaName.equals(op.getOtaName())) // 1
                     .collect(Collectors.toMap(OTAProperty::getId,
                                               (h) -> hotel))) // 2
             .map(Map::entrySet) // 3
             .flatMap(Collection::stream) // 4
             .collect(Collectors.toMap(Map.Entry::getKey,
                                       Map.Entry::getValue)); // 5
}

Equivalent of the original if condition: we take only OTAProperties that match the required name.
Each hotel item is wrapped to a Map<Long, Hotel>, which is a subset of the resulting map.
Transformation of the stream of Maps into a stream of Set<Entry<Long, Hotel>>.
Concatenation of streams of Sets into a single stream of Entry<Long, Hotel>. Unfortunately, we cannot join Maps so easily, that is why there is this intermediate transformation.
Finally, the stream of entries is collected into a Map representing the result of the sequence.


Changing a matrix by multiplying indexes of min element I need to make the following C program: Given A (N, N) matrix (2D array). The program needs to find the min element in each column. Then it needs to multiply the row in which the element is, with the difference of its indexes (a[i] * i - j). Then transfer the new row on another matrix. Print the new matrix.

Here is my code:

#include<stdio.h>

void InputArray(int a[][50],int M, int *N);
void NewArray(int a[][50], int b [][50], int N);
void PrintA(int a[][50], int N);
void PrintB(int b[][50], int N);

void main(){
    int a[50][50], b[50][50], N;
    InputArray(a, 50, &N);
    system("cls");
    NewArray(a, b, N);
    PrintA(a, N);
    PrintB(b, N);
}
void InputArray(int a[][50], int M, int *N){
    do{
        printf("Enter rows and columns: ");
        scanf("%d", N);
    } while (*N < 1 || *N > M);

    for (int i = 0; i < *N; i++){
        for (int j = 0; j < *N; j++){
            printf("Enter i[%d][%d] element ", i, j);
            scanf("%d", &a[i][j]);
        }
    }
}

void NewArray(int a[][50], int b[][50], int N){
    int i, j, min, d, k, rowb = 0, rowa = 0;
    for (i = 0; i < N; i++){
        min = a[0][i];
        d = 0 - i;
        for (j = 0; j < N; j++){
            if (a[j][i] < min){
                min = a[j][i];
                d = j - i;
                rowa = j;
            }
        }

        for (k = 0; k < N; k++){
            b[rowb][k] = a[rowa][k] * d;
        }
        rowb++;
    }
}
void PrintA(int a[][50], int N){
    printf("You've entered: \n");
    for (int i = 0; i < N; i++){
        for (int j = 0; j < N; j++){
            printf("%d  ", a[i][j]);
        }
        printf("\n");
    }
    printf("\n");
}
void PrintB(int b[][50], int N){
    printf("New matrix is :\n");
    for (int i = 0; i < N; i++){
        for (int j = 0; j < N; j++){
            printf("%d  ", b[i][j]);
        }
        printf("\n");
    }
}

It does work, but I think it the "NewArray" function can be made better.

Here is an example output:

You've entered:
1 4 9
2 6 5
3 8 2
New matrix is:
0 0 0
-1 -4 -9
0 0 0

Use error detection

With non-numeric input, scanf("%d", N); and following code will not function as expected.  Handle bad input.  For now, simple whine and take your toys home.  Later you can add more sophisticated error handling.

void InputArray(int a[][50], int M, int *N){
    // scanf("%d", N);
    if (scanf("%d", N) != 1) {
      perror("Invalid size");
      exit(EXIT_FAILURE);
    }

Seek clarification or state functionality on edge cases

"program needs to find the min element in each column. Then it needs to multiply the row in which the element is, with the difference of its indexes".  What should happen if the min value occurs twice?

Fold like functions together

Only difference between PrintA() and PrintB() is the header text.  Pass that in.

void PrintArray(const char *header, const int a[][50], int N){
    fputs(header, stdout);
    for (int i = 0; i < N; i++){
        for (int j = 0; j < N; j++){
            printf("%d  ", a[i][j]);
        }
        printf("\n");
    }
    printf("\n");
}
...
PrintArray("You've entered:\n", a, N);
PrintArray("New matrix is :\n", b, N);

Avoid naked magic numbers

Rather than code 50 in many places, define it once and use that.

#defined ARRAY_DIM_MAX 50
// int a[50][50], b[50][50], N;
// InputArray(a, 50, &N);
int a[ARRAY_DIM_MAX][ARRAY_DIM_MAX], b[ARRAY_DIM_MAX][ARRAY_DIM_MAX], N;
InputArray(a, ARRAY_DIM_MAX, &N);

Further, consider using different constant names should you later want a rectangle matrix.  Even if both are 50, this better self documents the code.  Recommend to not declare many variables declares on a line.

#defined ARRAY_ROW_SZ 50
#defined ARRAY_COL_SZ 50

// int a[50][50], b[50][50], N;
// InputArray(a, 50, &N);
int a[ARRAY_ROW_SZ][ARRAY_COL_SZ];
int b[ARRAY_ROW_SZ][ARRAY_COL_SZ];
int N = 0; // Defensive coding to assign simple small objects.
InputArray(a, ARRAY_ROW_SZ, &N);

Minor stuff

No need for j==0 iteration

for (i = 0; i < N; i++){
    min = a[0][i];
    d = 0 - i;
    // for (j = 0; j < N; j++){
    for (j = 1; j < N; j++){

Use const when referenced data is unchanged

This allows for wider applications, convey to users of the function of data involatility and may allow additional optimizations.

// void PrintA(int a[][50], int N){
void PrintA(const int a[][50], int N){

Use standard signatures

void main() may be acceptable on your platform, but not others.   Better to return int.

Avoid non-portable code

system("cls"); is non-standard.

Use size_t

For array sizing and indexing, size_t is the Goldilocks type, neither not too narrow nor too wide.  As you develop, use that rather than int for indexing.  int is OK for learner code.

// void InputArray(int a[][50], int M, int *N){
void InputArray(int a[][50], size_t M, size_t *N){