Event Messenger C++ Game Programming I'm using this Game Messenger as an instant messaging tool in my game. Just wondering peoples on thoughts on it.
Thank you!
    enum class eGameMessageType
{
    AddItemGUI = 0,
    RemoveItemGUI,
    UpdateSelectionBoxGUI,
    UpdateItemQuantityGUI,
    DestroyCubeSetPosition,
    DestroyCubeReset,
    SelectedCubeSetPosition,
    SelectedCubeSetActive,
    SpawnPickup,
    PlayerDisgardPickup,
    AddToPlayerInventory,
    Max = AddToPlayerInventory
};

struct Listener : private NonCopyable
{
    Listener(const std::function<void(const void*)>& fp, const void* ownerAddress)
        : m_listener(fp),
        m_ownerAddress(ownerAddress)
    {
        assert(ownerAddress);
    }
    Listener(Listener&& rhs) noexcept
        : m_listener(rhs.m_listener),
        m_ownerAddress(rhs.m_ownerAddress)
    {
        rhs.m_listener = nullptr;
        rhs.m_ownerAddress = nullptr;
    }
    Listener& operator=(Listener&& rhs) noexcept
    {
        m_listener = rhs.m_listener;
        m_ownerAddress = rhs.m_ownerAddress;

        rhs.m_listener = nullptr;
        rhs.m_ownerAddress = nullptr;

        return *this;
    }

    std::function<void(const void*)> m_listener;
    const void* m_ownerAddress;
};

class GameMessenger : private NonCopyable, private NonMovable
{
public:
    static GameMessenger& getInstance()
    {
        static GameMessenger instance;
        return instance;
    }

    template <typename GameMessage>
    void subscribe(const std::function<void(const GameMessage&)>& gameMessage, const void* ownerAddress)
    {
        auto& listeners = m_listeners[static_cast<int>(GameMessage::getType())];
        assert(!isOwnerAlreadyRegistered(listeners, GameMessage::getType(), ownerAddress));

        listeners.emplace_back(reinterpret_cast<std::function<void(const void*)> const&>(gameMessage), ownerAddress);
    }

    template <typename GameMessage>
    void unsubscribe(const void* ownerAddress)
    {
        auto& listeners = m_listeners[static_cast<int>(GameMessage::getType())];
        assert(isOwnerAlreadyRegistered(listeners, GameMessage::getType(), ownerAddress));

        auto iter = std::find_if(listeners.begin(), listeners.end(), [ownerAddress](const auto& listener)
        {
            return listener.m_ownerAddress == ownerAddress;
        });

        assert(iter != listeners.end());
        listeners.erase(iter);
    }

    template <typename GameMessage>
    void broadcast(GameMessage gameMessage)
    {
        const auto& listeners = m_listeners[static_cast<int>(GameMessage::getType())];
        for (const auto& listener : listeners)
        {
            reinterpret_cast<std::function<void(const GameMessage&)> const&>(listener.m_listener)(gameMessage);
        }
    }

private:
    GameMessenger() {}
    std::array<std::vector<Listener>, static_cast<size_t>(eGameMessageType::Max) + 1> m_listeners;

    bool isOwnerAlreadyRegistered(const std::vector<Listener>& listeners, eGameMessageType gameMessageType, const void* ownerAddress) const
    {
        assert(ownerAddress != nullptr);

        if (!listeners.empty())
        {
            auto result = std::find_if(listeners.cbegin(), listeners.cend(), [ownerAddress](const auto& listener)
            {
                return listener.m_ownerAddress == ownerAddress;
            });

            return result != listeners.cend();
        }
        else
        {
            return false;
        }
    }
};

Move struct Listener into class GameMessenger
The struct Listener is just an implementation detail of GameMessenger, you can move it into class GameMessenger like so:
class GameMessenger: ...
{
    struct Listener: ...
    {
         ...
    };

    ...
};

Now that it is private, I would also keep Listener very simple, and just have it hold the two member variables, without adding any member functions.
Pass messages as const references
The actual message handler functions take a const reference to the message, but your GameMessenger::broadcast() takes it by value, making an unnecessary copy. Change it to:
void broadcast(const GameMessage &gameMessage) {...}

Naming things
Some names could be improved. For example, struct Listener has a member variable m_listener. Combined with the std::vector<Listener>, you get the word listener repeated a lot, like in GameMessenger::broadcast(), where there's even a listener.m_listener. Consider renaming m_listener to m_handler or m_callback.
Some names are a bit longer than necessary. I suggest these changes:
- `typename GameMessage` -> `typename Message`
- `isOwnerAlreadyRegistered()` -> `isRegistered()`

Make class GameMessenger a template
With void pointers you throw type-safety overboard, and requires you to reinterpret_cast<>() things. I can't say much about m_ownerAddress, however
the main problem here is that you have a non-templated class GameMessenger. Apart from the constructor, all the public member functions are template <typename GameMessage>. Instead of having one GameMessenger object being responsible for all possible message types, consider making the class templated itself, and only be responsible for one specific message type.
If you implement it this way, there will be one GameMessenger object per message type. That means there is no longer a need to have a std::array for each of the possible message types, and in fact enum eGameMessageType is no longer necessary.
Here is an example of how it might look:
template <typename Message>
class GameMessenger: private NonCopyable, private NonMovable
{
    struct Listener {
        const std::function<void(const Message &)> m_handler;
        const void *m_ownerAddress;
    };

public:
    static GameMessenger& getInstance()
    {
        static GameMessenger instance;
        return instance;
    }

    void subscribe(const std::function<void(const Message&)>& handler, const void* ownerAddress)
    {
        assert(!isRegistered(ownerAddress));
        listeners.emplace_back(handler, ownerAddress);
    }

    template <typename Message>
    void unsubscribe(const void* ownerAddress)
    {
        auto iter = std::find_if(m_listeners.begin(), m_listeners.end(), [ownerAddress](const auto& listener)
        {
            return listener.m_ownerAddress == ownerAddress;
        });

        assert(iter != listeners.end());
        listeners.erase(iter);
    }

    void broadcast(const Message &message)
    {
        for (const auto& listener: m_listeners)
        {
            listener.m_handler(message);
        }
    }

private:
    std::vector<Listener> m_listeners;

    bool isRegistered(const void* ownerAddress) const
    {
        assert(ownerAddress != nullptr);

        auto result = std::find_if(listeners.cbegin(), listeners.cend(), [ownerAddress](const auto& listener)
        {
            return listener.m_ownerAddress == ownerAddress;
        });

        return result != listeners.cend();
    }
};

So how do you use it? Instead of writing:
void handleSpawnPickup(const SpawnPickupMessage &message) {...}
void *ownerAddress = ...;

auto messenger = GameMessenger.getInstance();
messenger.subscribe(handleSpawnPickup, ownerAddress);

You replace the last two lines with:
auto messenger = GameMessenger<SpawnPickupMessage>.getInstance();
messenger.subscribe(handleSpawnPickup, ownerAddress);

If you want to avoid having to explicitly mention the type when getting the messenger instance, consider writing an out-of-class function that subscribes to the right instance for you:
template<typename Message>
void subscribeToMessenger(const std::function<void(const Message&)>& handler, const void* ownerAddress)
{
    GameMessenger<Message>::getInstance().subscribe(handler, ownerAddress);
}

So that you can just write:
subscribeToMessenger(handleSpawnPickup, ownerAddress);


Comparing performance between thread, task, parallel I wrote some function to compare the duration taken for 1000 loops. Just wondering if the comparison is right? For the thread, I want to set to maximum 3 thread. Thread is core, right? Task took the longest time which seems not correct.

using System;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

namespace Multithreading_Loop
{
class Program
{
    static void Main(string[] args)
    {
        //PrintLoopWithoutThread();
        //PrintLoopWithThread();
        //PrintLoopWithTaskWaitAll();
        //PrintLoopWithParallelFor();
        PrintLoopWithParallelForEach();
    }

    /// <summary>
    /// Elapsed time: 4.13 seconds
    /// </summary>
    private static void PrintLoopWithoutThread()
    {
        Stopwatch sw = new Stopwatch();
        sw.Start();

        for (var x = 1; x <= 1000; x++)
        {
            Console.WriteLine($"Worker Id: {Thread.CurrentThread.ManagedThreadId}");
            Console.WriteLine(x);
        }

        sw.Stop();
        Console.WriteLine(sw.Elapsed.TotalSeconds);
    }

    /// <summary>
    /// Elapsed time: 11.4 seconds
    /// </summary>
    private static void PrintLoopWithThread()
    {
        Stopwatch sw = new Stopwatch();
        sw.Start();

        Thread t1 = new Thread(PrintLoopWithoutThread);
        t1.Start();
        Thread t2 = new Thread(PrintLoopWithoutThread);
        t2.Start();
        Thread t3 = new Thread(PrintLoopWithoutThread);
        t3.Start();
        t1.Join();
        t2.Join();
        t3.Join();

        sw.Stop();
        Console.WriteLine(sw.Elapsed.TotalSeconds);
    }

    /// <summary>
    /// Elapsed time: 15.5 seconds
    /// </summary>
    private static void PrintLoopWithTaskWaitAll()
    {
        Task task1 = Task.Factory.StartNew(() => PrintLoopWithoutThread());
        Task task2 = Task.Factory.StartNew(() => PrintLoopWithoutThread());
        Task task3 = Task.Factory.StartNew(() => PrintLoopWithoutThread());

        Task.WaitAll(task1, task2, task3);

    }

    /// <summary>
    /// Elapsed time: 2.2 seconds
    /// </summary>
    private static void PrintLoopWithParallelFor()
    {
        Stopwatch sw = new Stopwatch();
        sw.Start();

        Parallel.For(0, 999, x =>
        {
            Console.WriteLine($"WorkerId: {Task.CurrentId}. Number: " + x);
        });

        sw.Stop();
        Console.WriteLine(sw.Elapsed.TotalSeconds);
    }

    /// <summary>
    /// Elapsed time: 2.0 seconds
    /// </summary>
    private static void PrintLoopWithParallelForEach()
    {
        Stopwatch sw = new Stopwatch();
        sw.Start();

        var data = Enumerable.Range(0, 999);

        // If computer has 4 cores and you want to use the maximum 4,
        // then put MaxDegreeOfParallelism = 4
        Parallel.ForEach(data, new ParallelOptions { MaxDegreeOfParallelism = 4 }, i =>
        {
            Console.WriteLine($"WorkerId: {Task.CurrentId}. Number: " + i);
        });

        sw.Stop();
        Console.WriteLine(sw.Elapsed.TotalSeconds);
    }

}
}

The Console uses synchronization:

  I/O operations that use these streams are synchronized, which means that multiple threads can read from, or write to, the streams.

This synchronization ensures that all sentences are printed and nothing gets lost due to a data race.

However, synchronization kills multithreading, because the threads will stop at the synchronization object. Don't use Console.WriteLine() for performance comparisons.

Another thing to consider: at the moment you have 3 tasks, each printing 1000 lines. That way, scheduling cannot be done in small pieces. What you typically want is 3000 tasks, each printing 1 line. (read "printing" == "doing work").


Finding the possible number of equal pairs in an array I got this test online last week.
here is an example:
given this array:

{ 1 , 2 , 4 , 5 , 1 , 4 , 6 , 2 , 1 , 4 }

on condition that

index(x) < index (y)

find the number of possible pairs (x,y).
where x = y.
the answer would be:

count = 7 - (0,4), (0,8), (4,8), (1,7), (2,5), (2,9), (5,9)

Edit3: added picture to clarify the pairs are elements from the same array. the picture has the first 3 pairs.

the first solution I came up with which was obviously bad:
public static int solution(int[] A)
    {
        int count = 0;
        for (int i = 0; i < A.Length; i++)
        {
            for (int j = i + 1; j < A.Length; j++)
            {
                if (A[i] == A[j])
                    count++;
            }
        }
        return count;
    }

basically, just count the number of possible pairs in each loop.
then after a bit of struggle I managed to do this:
public static int solution(int[] A)
        {
            int count = 0;
            var dict = new Dictionary<int, int>();
            for (int i = 0; i < A.Length; i++)
            {
                if (!dict.Any(x => x.Key == A[i]))
                    dict.Add(A[i], 1);
                else
                    dict[A[i]]++;
            }

            foreach (var item in dict)
            {
                count += Factorial(item.Value) / 2 * Factorial(item.Value - 2);
            }

            return count;
        }

1- count how many times the number is present.
2- calculate the possible pairs for each number which is given by this formula:

where n is the number of repetitions and r = 2 (pair, 2 items)
Factorial is just a simple implementation of the function.
I feel like there is a lot to improve on this. what do you think?
Edit: added the Factorial Function as requested:
public static int Factorial(int N)
        {
            return N == 0
                       ? 1
                       : Enumerable.Range(1, N).Aggregate((i, j) => i * j);
        }

Edit2:
I tested both and got 20s for the first method and 500ms for the second improved method.
the 100000 was the limit set by the test details.
here is the code for the test:
int[] testArray = new int[100000];
Random x = new Random();
for (int i = 0; i < testArray.Length; i++)
{
   testArray[i] = x.Next(1, 10);
}
Stopwatch sw = new Stopwatch();
sw.Start();
solution(testArray);
sw.Stop();

Edit4:
Well, it seems like I messed up!
calculating factorial for numbers bigger 19 in intger is not possible, out of range.
the worse is I didn't even have to calculate it.
in my formula since r is a constant I could simplify it to

n* (n -1) / 2

thus avoiding the entire thing.

Based on the calculation of the number of pairs -your assertion is correct.
The whole calculation method could be simplified by:
    public static List<KeyValuePair<int,int>> solution(int[] A)
    {

        //n(n-1)/2 number of pairs by a given set
        return  A.GroupBy(x => x).ToList().
            Select(y => new KeyValuePair<int, int>((y.Count() * (y.Count() - 1)) / 2, y.Key)).ToList();
    }


Parsing date strings, inferring the year such that the date is not in the future I got this program that I am working on and in it, I got a section that has a try statement and two error except statements.

try:
    ...
    if ...: raise SyntaxError
except SyntaxError:
    ...
except ValueError:
    ...

The thing is that the SyntaxErrors are technically ValueErrors, but the errors occur for different reasons, therefore need different code to solve/get around the problem. So in order to prevent the code in all the "excepts" from being executed, I had to separate/make the type of Errors different. Is it okay to do that even though the SyntaxErrors are techincally ValueErrors, that I label them as SyntaxErrors? Or is there a method in solving this problem?

Here is the code of the program:

from datetime import datetime

today = datetime(2019, 1, 1).date()
this_year = 2019
last_year = this_year - 1

text = ["Dec 31 ", "@@@@@@@@@@@@@@@@@@@@@Dec 31 "]

for line in text:
    date_str = line
    try:
        date = datetime.strptime(date_str + str(this_year), "%b %d %Y").date()
        if date > today: raise SyntaxError
        print("+", date)
    except SyntaxError:
        date = datetime.strptime(date_str + str(last_year), "%b %d %Y").date()
        print("-", date)
    except ValueError:
        print("abnormality")
        continue

Obtaining a date that is in the future is in no way a "syntax error", and it would be inappropriate to raise a SyntaxError.  Instead of abusing the exception-handling mechanism, you should just use a conditional.

for date_str in text:
    try:
        date = datetime.strptime(date_str + str(this_year), "%b %d %Y").date()
        if date <= today:
            print("+", date)
        else:
            date = datetime.strptime(date_str + str(last_year), "%b %d %Y").date()
            print("-", date)
    except ValueError:
        print("abnormality")

Note that you might be misinterpreting some ValueErrors as abnormalities.  For example, "Feb 29 2021" would cause a ValueError, but then you would never get a chance to try "Feb 29 2020", which is a valid day in a leap year.


Breaking up a PHP class between a master class and subclasses I have a huge class taking care of companies informations in my app. Arrived at 4000 lines of code (!), I finally decided to break it up. However I am not too sure if the way I am doing this is the best. For example I had shareholders functions in the company class taking care of returning one or all, editing, adding, and deleting shareholders. And I had a whole bunch of other functions built in the same way for representatives, warrantors, offices, etc...

I did a new company class with some basic functions that I want shared with all subclasses, and the idea was to call from within this class the specialized subclasses.

However I would like my shareholder constructor to be protected in a way that it can only be called through the company class, which is impossible with the structure I chose. Also, I shouldn't be allowed to instantiate a new shareholder from the shareholder class which again I can with this construction.

I think I should be using traits and abstract class to achieve this, but I am not sure how to. Sorry if this is a bit confused, but any help is welcome.

class company{
  private $id;
  protected $db;

  public function __construct($id, $db){
    $this->id = $id;
    $this->db = $db;
  }

  public function get_id(){
    return $this->id;
  }
  ...
  public function shareholder(){
    if ( !isset($this->shareholder) ){
      $this->shareholder = new adherent\shareholder($this->get_id(), $this->db);
    }
    return $this->shareholder;
  }
}

// This will be implemented for all subclasses dealing with lists
interface _list {

  public function count($incl_deleted = false);

  public function get($id);

  public function get_all($incl_deleted = false);

  public function insert($cfg);

  public function update($cfg);

  public function delete($id);

}

class link extends company implements _list{
  public function count($incl_deleted = false){
    ...
  }

  public function get($id){
    ...
  }

  public function get_all($incl_deleted = false){
    ...
  }

  public function insert($cfg){
    ...
  }

  public function update($cfg){
    ...
  }

  public function delete($id){
    ...
  }
}

class shareholder extends link{

  public function insert($cfg)
  {
    // Specific to this class
    if ( is_array($cfg) &&
      isset($cfg['parts'], $cfg['id_tiers']) &&
      \bbn\str\text::is_number($cfg['parts']) &&
      ($cfg['parts'] > 0) ){
      return parent::insert($cfg);
    }
    $this->error("Problème dans la configuration pour l'insertion");
  }

  public function update($cfg)
  {
    // Specific to this class
    if ( is_array($cfg) &&
      isset($cfg['id'], $cfg['parts'], $cfg['id_tiers']) &&
      \bbn\str\text::is_number($cfg['parts']) &&
      ($cfg['parts'] > 0) ){
      return parent::update($cfg);
    }
    $this->error("Problème dans la configuration pour l'update");
  }
  // Other functions use the link class
}

Coding standards
I'm not going into details here. But there are coding standards psr-1 tot psr-4. I would advice you to follow them.
Another quick remark: if you need something, ask for it. And if you cant do it, throw an exception. This code shows exactly what I mean:
  public function insert($cfg)
  {
    // Specific to this class
    if ( is_array($cfg) &&
      isset($cfg['parts'], $cfg['id_tiers']) &&
      \bbn\str\text::is_number($cfg['parts']) &&
      ($cfg['parts'] > 0) ){
      return parent::insert($cfg);
    }
    $this->error("Problème dans la configuration pour l'insertion");
  }

In order to be able to do something, you need parts, id_tiers. parts needs to be a number larger then 10. So ask it:
public function insert($parts, $tiers)
{
    if (!is_number($parts)) throw new IllegalArgumeInvalidArgumentExceptionntException('parts needs to be a number');

    if ($parts < 0) throw new InvalidArgumentException('parts needs to be larger then 0');

    //do the actual inserting
}

But the problem you had to solve with that nifty && stuff is that you are extending a Link that extends a Company. And that is how I come to my conclusion:
I don't think you understand the concept of 'OO'. You use words like interface, class, protected and that's it.
Encapsulate everything
What you did is create one GODObject. It knows all, does all and will eventually break all. A good thing to start is to define all your Domain Models. Domain Models are defined by state, and are nearly always stored in some persisting storage (e.g. a Database).
Domain Models
Domain Models represent state and give us a set of tools to easily access that state. A Domain model is by definition stupid:
class MyDomainModel
{
    private $attribute;

    public function getAttribute()
    {
        return $this->attribute;
    }

    public function setAttribute($value)
    {
        $this->attribute = $value;
    }
}

It gives you a set of tools to pass around state. In your case, there will be multiple Domain models:

Company
Shareholder
Representative
Warrantor
Office
...

A Shareholder and a Representative will probably be special cases of a User:
class User
{
    private $name;
    /** getter + setter for name */
}

class Shareholder extends User
{
    private $someShareholderStuff;
}

class Representative extends User
{
    private $someRepresentativeStuff;
}

That makes sense. Your code doesnt, here is why:
A link is a Company and implements the a _list. A Shareholder is a link (and thus also a Company). A Company has multiple Shareholders. Thus, a Company has multiple Companies. And just sounds strange.
What is beging done? And who is doing it?
This is a mistake a lot of people make. They add methods to the wrong class. This comes from the 'We use MVC and cram everything into either a Model, a View or a Controller'. And they end up writing code like this:
class User extends BasicModel
{
    public function sendRegistrationMail()
    {
        $this->mailer->sendEmail($this->email, 'Lorem Ipsum registration');
    }
}

and use it
$user->sendRegistrationMail();

Some might event argue that there is nothing wrong with this code. But lets look at what is going on. Somewhere down the road, the USer Object - that was a simple representation of state - learned how to use the MailerInterface. And now everytime we create a User, we also need a Mailer to pass to the constructor. The target has become the actor.
Much better code would be:
$mailer->sendEmail($email, $user);

The Email and the User care less about how the email is send. They don't even know an email is send. They shouldn't, they are but dum objects that represent state.
I want my constructors to be protected
This reminds me of the Singleton anti-pattern. If you don't want multiple instances to be created, then don't create multiple instances. Same goes for your Shareholder class. If you don't want it to be created outside a given context, then don't. Code doesn't magically appear.
And think of testing. One day you will have to stub a Shareholder, and then writing new Shareholder('name'); without the app complaining will be heaven.


Handling click events to open and close menus depending on the click target I'm building a navigation that has a breadcrumb with drop down menus. I want the menus to open up when I click the open link, then there are many instances when the menu should close:

The link is clicked a second time, with the intention of closing the menu
When another menu is open, the first menu should close
When a menu is open, and anywhere outside the menu is clicked
When a menu item is clicked

I'm anticipating that I'll have many click events to handle, and many different types of menus, pop-ups and drop downs through the entire site, so I'm trying to handle that efficiently, thus the switch statement.

$().ready(function () {
    //Click event mayhem!
    $('html').click(function (event) {
        event.stopPropagation();
        var target = $(event.target);
        switch (target.attr('class')) {
            case 'expandDropDown':
                handleBreadCrumbDropDownClick(target);
                break;
            case 'headlineTitle':
                event.preventDefault();
                handleAccordionItemClick(target);
                break;
            default:
                //Always close dropdown menus, unless they're specifically being opened
                toggleDropDown('.BC-DropDown-Open');
        }
    });
});

function handleBreadCrumbDropDownClick(target) {
    //Get the drop down list being targeted
    var dropDownList = '#' + target.attr('itemprop');
    //Offset of the expander link
    var offset = target.offset();
    if ($(dropDownList).hasClass('BC-DropDown-Open')) {
        //Dropdown is being closed with the expandDropDown link
        $(dropDownList)
                        .hide()
                        .toggleClass('BC-DropDown-Open');
    }
    else {
        //Dropdown is being opened with the expandDropDown link

        if ($('.BC-DropDown-Open').length > 0) {
            //Close existing drop down lists if any
            toggleDropDown('.BC-DropDown-Open');
        }
        //Open selected dropdown
        toggleDropDown(dropDownList, offset);
    }
}

The switch statement isn't needed.

Just set a click function for each class.

$(document).ready(function ready () {
    $('.expandDropDown').click( handleBreadCrumbDropDownClick );
    $('.headlineTitle').click( handleAccordionItemClick );
});

function handleBreadCrumbDropDownClick() {
    var target = $(this);
    var dropDownList = $('#' + target.attr('data-itemprop'));
    var offset = target.offset();
    if ( dropDownList.hasClass('BC-DropDown-Open') ) {
        dropDownList
          .hide()
          .toggleClass('BC-DropDown-Open');
    }  else {
        if ( $('.BC-DropDown-Open').length ) {
            toggleDropDown('.BC-DropDown-Open');
        }
        toggleDropDown(dropDownList, offset);
    }
}


Finding the most common character in a string with a hash map I created a method for finding the most common character in a string (using HashMap):

public static char getMaxViaHashmap ( String s) {
         HashMap<Character, Integer> map = new HashMap<Character, Integer>();
         char maxappearchar = ' ';
         for (int i = 0; i < s.length(); i++)
         {
              if ( map.containsKey(s.charAt(i)))
              {
                    map.put (s.charAt(i), map.get(s.charAt(i)) + 1 );
              }
              else
              {
                map.put (s.charAt(i), 1);
              }
         }

          int maxValueInMap=(Collections.max(map.values()));  // This will return max value in the Hashmap
            for (Entry<Character, Integer> entry : map.entrySet())
            {
                if (entry.getValue()==maxValueInMap)
                {
                    System.out.println("the max char is : " + entry.getKey() + "  and displayed  " +maxValueInMap+ "  times");     // Print the key with max value
                    maxappearchar = entry.getKey();
                }

            }
            return maxappearchar;
    }

Questions:

What is the performance cost (in terms of \$O()\$)?
What is the memory cost (in terms of \$O()\$)?

Normally operations on a hashmap should be pretty much constant (O(n)=1), so it's something like O(n) = n + 2*m (number of characters in the String plus twice the amount of different characters in the string, since you iterate twice over the map to find the max and the corresponding character).
Memory should also be pretty much linear, since you are storing one int per different character.

As Collections.max iterates once over the map, iterating again over it to get the corresponding character is pretty much wasted. You could remove the Collections.max call and start by iterating over the entries and remember the one with the highest count, thus saving the whole Collections.max iteration ( getting a O(n) = n + m runtime).

Your hashmap will probably need to resize itself once or twice (at least). You can reduce the need for that by choosing a good start size for your hashmap (aprox. the number of different characters in your string, which depends on the language), but normally that shouldn't be that much gain (depending on the length of the sentence and the amount of different characters in the sentence).


Add protocols inside the same file as controller I have a question about something maybe not so much important, but every time when I have to decide, I feel it can write in another way.
Imagine I am developing and system about authentication and then I have to receive a pin after sending my phone number.
So I am writing some UseCase and, in this case, I have an interactor and a Worker as protocol.
Let's show the code
import Foundation

protocol SendPinWorker {
    func run(onResult: ((Result<Bool, Error>)->Void)?)
}

protocol SendPinUseCaseInteractor {
    func error(e: SendPINUseCaseError)
    func sent()
    func processing()
}

enum SendPINUseCaseError {
    case invalidPhone
    case couldNotSend
}

class SendPINUseCase {

    private let worker: SendPinWorker
    private let output: SendPinUseCaseInteractor
    init(worker: SendPinWorker, output: SendPinUseCaseInteractor) {
        self.worker = worker
        self.output = output
    }

    func execute(phone: String) {
        guard !phone.isEmpty else {
            self.output.error(e: .invalidPhone)
            return
        }

        self.output.processing()

        self.worker.run { [weak self] result in

            guard let self = self else {
                return
            }

            guard let sent = try? result.get(), sent else {
                self.output.error(e: .couldNotSend)
                return
            }

            self.output.sent()
        }
    }
}

Should I put these protocols and enums in another file or should I still with these structures in the same file?

Consider nesting your types:
If you have two types that start with the same prefix, it's possible that one type is better nested inside the other.
For example: SendPINUseCase raises SendPINUseCaseError's
class SendPINUseCase {
    enum Error {
        case invalidPhone
        case couldNotSend
    }
}

Now you have a type called SendPINUseCase.Error, it's scoped to the SendPINUseCase. If you want to use throwing functions instead of returning Result<..., Error>'s, then you would make your SendPINUseCase.Error enum conform to Swift.Error
Protocols can't be nested, but in this case you could avoid them entirely by changing your design so that, for example, execute function takes, as arguments, the callback functions involved. If you eliminate the protocols, then there's no worry about which file to put them in, and protocols are not without their problems, I would say protocols are not the right abstraction in this case.
Your SendPINUseCase is a class but doesn't encapsulate any state, so consider making it a struct.
You have execute function take a String as input, and then validate that string is a 'phone'. Instead, consider making a struct Phone that has a failable initializer, so that only valid 'Phone's can be constructed. Then execute function would take a Phone struct that you know is valid, that eliminates the invalidPhone error case entirely.


Rock, Paper, Scissors, Lizard and Spock I am currently done with this little Rock, Paper, Scissors, Lizard, Spock game in JavaScript. The game is working fine, but I am not pleased with the code. Is there a way to refactor these if statements? I was thinking about ternary operators and I tried doing so but failed. This is basically my function based on which I get the results.

function result(userChoice, cpuChoice) {
    var result = '';

     if(userChoice == 'rock' ) {
        if (cpuChoice == 'rock') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'spock') {
            result = 'Spock vaporizes rock';
            loses++;
        } else if ( cpuChoice == 'lizard' ) {
            result = 'Rock crushes lizard';
            wins++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Paper covers rock';
            loses++;
        }  else if ( cpuChoice == 'scissors' ) {
            result = 'Rock crushes scissors';
            wins++;
        };
    } else if(userChoice == 'paper') {
        if (cpuChoice == 'paper') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'spock') {
            result = 'Paper disproves Spock';
            wins++;
        } else if ( cpuChoice == 'lizard' ) {
            result = 'Lizard eats paper';
            loses++;
        }  else if ( cpuChoice == 'rock' ) {
            result = 'Paper covers rock';
            wins++;
        }  else if ( cpuChoice == 'scissors' ) {
            result = 'Scissors cuts paper';
            loses++;
        };
    } else if(userChoice == 'scissors') {
        if (cpuChoice == 'scissors') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'spock') {
            result = 'Spock distroys scissors';
            loses++;
        } else if ( cpuChoice == 'lizard' ) {
            result = 'Scissors beheads lizard';
            wins++;
        }  else if ( cpuChoice == 'rock' ) {
            result = 'Rock crushes scissors';
            loses++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Scissors cuts paper';
            wins++;
        };
    } else if(userChoice == 'lizard') {
        if (cpuChoice == 'lizard') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'spock') {
            result = 'Lizard poisons Spock';
            wins++;
        } else if ( cpuChoice == 'scissors' ) {
            result = 'Scissors beheads lizard' ;
            loses++;
        }  else if ( cpuChoice == 'rock' ) {
            result = 'Rock crushes lizard';
            loses++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Lizard eats paper';
            wins++;
        };
    } else if(userChoice == 'spock') {
        if (cpuChoice == 'spock') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'lizard') {
            result = 'Lizard poisons Spock';
            loses++;
        } else if ( cpuChoice == 'scissors' ) {
            result = 'Spock distroys scissors';
            wins++;
        }  else if ( cpuChoice == 'rock' ) {
            result = 'Spock vaporizes rock';
            wins++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Paper disproves Spock';
            loses++;
        };
    } else {
        return false;
    };

    return result;
};

To review the code as it is:

It would be better not to even have result as a variable, and swap the order of operations inside the else-ifs that assign result from (result = X, gameResult++) to (gameResult++, return X)
You don't need semi-colons on the end of if statements:

} else {
    return false;
};

Rather than individually testing (userInput == X, then X == cpuInput), remove them and add the following if statement to the top.

if (userChoice === cpuChoice){
    ties++;
    return "Tie";
}
//The rest follows

Onto fixing the if-else!

So, instead of a long  if-else statement, I'd use an object, which lets you specify key and value pairs, which really cleans the process up, because you don't have to iterate over all the options before finding the one you need.

Here's what I came up with:

var results = {
    wins: 0,
    loses: 0,
    ties: 0
};

function RPSLZ(userChoice, cpuChoice) {
    var RULES = {
        rock: {
            lizard: 'Rock crushes lizard',
            scissors: 'Rock crushes scissors'
        },
        paper: {
            spock: 'Paper disproves Spock',
            rock: 'Paper covers rock'
        },
        scissors: {
            lizard: 'Scissors beheads lizard',
            paper: 'Scissors cuts paper'
        },
        lizard: {
            spock: 'Lizard poisons Spock',
            paper: 'Lizard eats paper'
        },
        spock: {
            scissors: 'Spock distroys scissors',
            rock: 'Spock vaporizes rock'
        }
    };
    if (userChoice == cpuChoice) {
        results.ties++;
        return 'Tie';
    } else if (!userChoice in RULES){
        return 'Invalid Input';
    } else {
        return (cpuChoice in RULES[userChoice]
                    ? (results.wins++, RULES[userChoice][cpuChoice])
                    : (results.loses++, RULES[cpuChoice][userChoice])
                );
    }
}

  How does it work?

That's a good question. It follows the following logic flow:

Declare the results outside the function, so it can continue recording results with more function calls (There is a way to do this inside the function, but I couldn't figure out how.)
Declaring the game RULES (Notice the all-caps, meaning it's a constant)
There's two special rules: tie and Invalid Input. They can be tested like described above, and like testing whether the input exists inside the RULES variable, because if it didn't, it would be undefined.
By using the same undefined testing rule, the cpuChoice can be tested to see whether it's inside the object of the userChoice, depending on the result, the respective counter is incremented, and the result returned.


Same function, variable parameters and types with lambda In my current project, I find myself using Dispatcher.Invoke a lot. Before each call I test if the current dispatcher is the right one. I want to write a wrapper class like so:

class SafeRunner
{
    //From the thread we always need to run on
    public static System.Windows.Threading.Dispatcher disp;

    public static void Run<T>(Action<T> f, T arg)
    {
        if (disp != System.Windows.Threading.Dispatcher.CurrentDispatcher)
        {
            disp.Invoke(f, arg);
        }
        else
        {
            f(arg);
        }
    }
    public static void Run<T1, T2>(Action<T1,T2> f, T1 arg1, T2 arg2)
    {
        if (disp != System.Windows.Threading.Dispatcher.CurrentDispatcher)
        {
            disp.Invoke(f, arg1, arg2);
        }
        else
        {
            f(arg1, arg2);
        }
    }
    public static void Run<T1, T2, T3>(Action<T1,T2,T3> f, T1 arg1, T2 arg2, T3 arg3)
        {...}
    public static void Run<T1, T2, T3, T4>(Action<T1,T2,T3,T4> f, T1 arg1, T2 arg2, T3 arg3, T4 arg4)
        {...}
}

And I can use it like this:

SafeRunner.Run<string,string>((a,b) => {
    doSomething(a,b);
    doSomethingElse(a);
    // Maybe a few more lines here
}, "Hello", "World");

This works, but doesn't feel right. Is there a better way?

A couple of things I notice, and they are both around the if statements

I would flip the if statements around so that they are checking the positive test.  I would also put the disp on the right side of the ==.  You can eliminate the else statement, all it really does is clutters up the code.

if (System.Windows.Threading.Dispatcher.CurrentDispatcher == disp)
{
    f(arg);
    return;
}

disp.Invoke(f, arg);


Visualize the runtimes of list algorithms I wrote a small script to visualize the runtimes of list algorithms.

The script works by creating a list of lists with sampled lengths from 1 to max_size. Next, I map the given algorithm over each list element and replaced the list with the resulting runtime. The runtimes of each list are plotted against the length of each list to generate a runtime plot.

My questions are:

Does my code deviate from best practices?
Could my code get a performance increase by using co-routines or some
other type of concurrency (like asyncio)?

from time import time
from random import randrange

from concurrent.futures import ThreadPoolExecutor
from functools import partial

from matplotlib import pyplot as plt

def func_timer(func, *params, verbose=False):
    """Takes in a function and some parameters to the function and returns the execution time"""

    start = time()
    func(*params)
    t = time() - start

    if verbose:
        print('function {func_name} took {time} seconds to complete given parameters'.format(
        func_name=func.__name__, time=t))
    return t

def list_generator(max_size, num_samples, n=10, upper_num=100):
    """Generates random integer lists with (sampled) lengths from range 1 to max_size.
    The difference between lengths (length spacing) of each list is delta, where delta is
    defined as the floor division of max_size and num_samples.

    max_size: int
    The max length of a generated list
    num_samples: int
    The number of lists to generate
    returns:
    lst_of_rand_lsts: list of lists
    """

    def get_rand_list(n):
        """Returns a  list of random numbers."""
        return [randrange(upper_num) for x in range(n)]

    assert max_size > num_samples
    delta = max_size // num_samples # uses the floor function to get difference between each sample.
    lst_lengths = [delta*x for x in range(1, num_samples + 1)] # Shift range by 1 to avoid making an empy list.

    return (get_rand_list(x) for x in lst_lengths)

def runtime_generator(func, lists):
    """Maps func over each list in a list of lists and replaces each list element with the function runtime."""

    partial_func_timer = partial(func_timer, func)

    with ThreadPoolExecutor(max_workers=5) as executor:
        res = executor.map(partial_func_timer, lists)

def rtviz(func, *args, max_size=1000, num_samples=500, viz=True, verbose=True):
    """Takes in a function that receives an iterable as input.
    Returns a plot of the runtimes over iterables of random integers of increasing length.

    func: a function that acts on an iterable"""

    def subplot_generator(lst_lengths, func_times_lst):
            plt.plot(lst_lengths, func_times_lst, 'bo')
            plt.xlabel('length of random lists')
            plt.ylabel('function runtime (sec)')
            plt.title('Runtime of function {}'.format(func.__name__))
            return

    lst_of_lsts = list(list_generator(max_size, num_samples))
    lsts_len = [len(elem) for elem in lst_of_lsts]

    start = time()
    runtime_gen = runtime_generator(func, lst_of_lsts)
    t = time() - start

    if verbose == True:
        print('%s took %0.3fms.' % (func.__name__, t*1000.))

    subplot_generator(lsts_len, list(runtime_gen))

    if viz == True:
        plt.show()

    return

if __name__ == "__main__":

    rtviz(sorted)

An even more vital question than does it deviate from best practices, is whether it actually does what you intend for it to do! Does it accurately time exection of the function in you are testing?

I believe not due to the following reasons:

Using time one a single run is at best inaccurate. Any code when run only once is kind of a random indicator as to how long time it'll take to do on the average. The shorter time it takes to execute, the more repeats you should have. You might also run into issues related to which current platform you are on, and imprecision in the time.time() call, as the documentation only promises a precision of a second.
You're setting up the time testing method within the actual time taking. You're timing on the outside of the runtime_generator, and do functools.partial and ThreadPoolExecutor within that function. In the best situations this only skew all of your data, in a more likely situation your method disappears in the time to set up the test run. This effect will be worse the faster your tested function are.

Lets say that test setup takes 100 ms, and your method 1 ms for 1 item, and 10 ms for 1000 ms. Your total time will vary from 101 ms to 110 ms, so you'll likely conclude that it is almost constant time, as 101 ms ~ 110 ms, but the real situation is that is has ten-doubled, 1 ms ~ 10 ms.

Best practice in Python, seems to use the timeit module for timing executions. This module can setup your runs, and execute them multiple times and eliminate some of these caveats.

Although you really gotta keep your head straight even when using this module, as timing execution depends on a lot of sub parameters like:  operating system, python interpreter, other load on testing platform, memory issues, code issues, sub effects of memoisations or other programming paradigm, and list goes on.


Emulating the pipe operator in javascript in a readable way In this article I found this quite elegant looking Elixir code:

conn
|> put_session(:current_user, user)
|> put_resp_header(location, "/")
|> put_resp_content_type("text/html")
|> send_resp(302, "You are being redirected")

It works by using the output of each of the functions as the first argument to the next function. The given args are bound to the second argument and up.

I know two different proposals for the pipe operator are being considered for a future version, but I am wondering if there is a way of writing something visually and functionally similar looking using the Javascript of today?

I am guessing I could use currying in some sense, binding everything but the first argument?

For reference, here are the original function definitions from the article. They all return a conn

assign(conn, key, value)
clear_session(conn)
put_resp_content_type(conn, content_type, charset)
put_resp_header(conn, key, value)
put_session(conn, key, value)
send_resp(conn, status, body)

I tried making some suggestions, but I would like input on how this could be improved/be more readable/look better for the casual programmer coming to modify my code.

A basic attempt using ES5.1 would look like this:

const {bind, flow} = require('lodash');
const _ = bind.placeholder;

flow(
  bind(put_session, null, _, current_user, user),
  bind(put_resp_header, null, _, location, '/'),
  bind(put_resp_content_type, null, _, 'text/html'),
  bind(send_resp, null, _, 302, 'You are being redirected')
)(conn);

It's a little bit too verbose compared to the original, so I tried compacting it down a bit:

// bind all but first argument - what should I call this?
// bindAllButFirstArgument is a tad bit long
const $$ = (...args) => bind(args[0], null, _, args.slice(1));

flow(
  $$(put_session, current_user, user),
  $$(put_resp_header, location, '/'),
  $$(put_resp_content_type, 'text/html'),
  $$(send_resp, 302, 'You are being redirected')
)(conn);

This looks a bit better (except for the mysterious $$ :) ), and with some minor modifications I could try to make the function in itself syntactically stand out by having a line like

  $$(put_session, current_user, user),

look like

  pipe.prepare(put_session)(current_user, user),

where pipe would be the equivalent of flow and pipe.prepare a utility function that "prepared" the function for the pipe flow mentioned above. Better? What do you think of this? Too dense? Needing to much reading of JSDocs? Different ways?

Would look like this

const pipe = (...args) => flow(...args);
pipe.prepare = fn => (...args) => bind(fn, null, _, args);

pipe(
  pipe.prepare(put_session)(current_user, user),
  pipe.prepare(put_resp_header)(location, '/'),
  pipe.prepare(put_resp_content_type)('text/html'),
  pipe.prepare(send_resp)(302, 'You are being redirected')
)(conn);

Runnable example: https://runkit.com/fatso83/piping-with-binding

It works by using the output of each of the functions as the first argument to the next function. The given args are bound to the second argument and up.

Just running by this description, one could easily create a piping operation with 2 helper functions:

const bind = (fn, ...boundArgs) => callArg => fn(callArg, ...boundArgs)
const pipe = (...fns) => initialArg => fns.reduce((r, fn) => fn(r), initialArg)

pipe(
  bind(put_session, current_user, user),
  bind(put_resp_header, location, '/'),
  bind(put_resp_content_type, 'text/html'),
  bind(send_resp, 302, 'You are being redirected')
)(conn)

bind accepts a function and bound arguments and returns a function that accepts a single argument. When this returned function is called, it calls the bound function with the call argument first, followed by the rest of the bound arguments.

pipe accepts a list of bound functions, and calls them one by one. The first function gets the call argument, while the remaining functions gets the return value of the previously called functions.

This approach effectively copies over the lodash signature, but does not require lodash. It also doesn't deal with this. You get the conciseness of your second example, and less of the bloat on your third example.


Stop Watch with time-records Exercise Description

Create a stop watch which has the following features:

"Start-/Stop" button
"Start-/Stop" button can be triggered by pressing the "s"-key.
"Record current time" button.
Display the recorded times since start.
"Record current time" button can be triggered by pressing the "t" key.
"Reset time" button which sets the timer counter back to 0 and deletes all time records.
"Reset time" button can be triggered by pressing the "r" key.

My Implementation

(function() {
  'use_strict';
  // -- VARIABLE DEFINITIONS ----------------------------------
  let currentTime;
  let timeRunning;
  let pastTimes = ['Past Times'];
  let intervalId;

  let startStop = document.getElementById('start-stop');
  let reset = document.getElementById('reset');
  let recordTime = document.getElementById('record-time');
  let currentTimeDisplay = document.getElementById('current-time-display');
  let pastTimesDisplay = document.getElementById('past-times-display');

  // -- FUNCTION DEFINITIONS ----------------------------------
  function init() {
    if (intervalId) {
      clearInterval(intervalId);
    }

    currentTime = 0;
    timeRunning = false;
    pastTimes.length = 1;
    currentTimeDisplay
      .textContent = currentTime;

    listPastTimes();
  }

  function triggerStartStop() {
    if (!timeRunning) {
      timeRunning = true;

      intervalId = setInterval(() => {
        currentTime += 10; // Increment

        currentTimeDisplay.textContent = // Write
          (currentTime / 1000).toFixed(2);
      }, 10);
    } else {
      clearInterval(intervalId);
      timeRunning = false;
    }
  }

  function listPastTimes() {
    var html = '
';

    pastTimes.forEach((item) => {
      html += '
' + item + '
';
    });

    pastTimesDisplay.innerHTML = html + '
';
  }

  function recordCurrentTime() {
    pastTimes.push((currentTime / 1000).toFixed(2));
    listPastTimes();
  }

  // -- ATTACHING EVENT HANDLER ------------------------
  startStop.addEventListener('click', triggerStartStop);
  reset.addEventListener('click', init);
  recordTime.addEventListener('click', recordCurrentTime);

  startStop.addEventListener('keydown', (event) => {
    if (event.keyCode === 83) { // 83 === 's' in ASCII
      triggerStartStop();
    }
  });

  reset.addEventListener('keydown', (event) => {
    if (event.keyCode === 82) { // 82 === 'r' in ASCII
      init();
    }
  });

  recordTime.addEventListener('keydown', (event) => {
    if (event.keyCode === 84) { // 84 === 't' in ASCII
      recordCurrentTime();
    }
  });

  init();
})();
body {
  font-family: Verdana;
  color: rgba(50, 50, 50, 1.0);
  background-color: rgba(250, 250, 250, 1.0);
}

.main-wrap {
  max-width: 800px;
  margin: 50px auto;
}

.navbar {
  margin: 20px 0;
}

#past-times-display li {
  list-style: none;
}

.subtitle {
  font-weight: 900;
  display: inline-block;
  margin-bottom: 10px;
}

.list-past-times li:nth-of-type(n+2) {
  margin-bottom: 6px;
}

Stopwatch Demo

    <button id="start-stop" tabindex="1">Start/Stop</button>
    <button id="reset" tabindex="2">Reset</button>
    <button id="record-time" tabindex="3">Record Time</button>

Has become quite a lot of code in the end.

Any ideas how one could accomplish the task with less code?
Are there points which could be improved or solved more elegantly?
Is everything done well?

Best Practices

Ideally you should separate your logic from your HTML as much as possible.
Building a string with += is less efficient than using a buffer. In other words, push each part of the string into an array and then join() the array. When you use += you're loading the entire string into memory every time you add to the string. Here's a re-write of your listPastTimes function.

function listPastTimes() {
  pastTimesDisplay.innerHTML =
    '

' +
    pastTimes.join('

') +
    '

';
}

Stuff that's just my opinion

I personally would not use the curlies for your if if you're only calling a function. I would put the function on the same line and lose the curlies. Some people are religious about the curlies and it's ok if you are, but in my opinion it's just adding unnecessary lines to the code. I'll defend that to the death. The Apple SSL bug is not a valid argument for curlies because that developer did not put the code on the same line as the if.
This is only gonna work on relatively new browsers (maybe that was inended) - I would have used more widely supported methods.
I've really don't like the idea of assigning booleans in an if/else. To me, this feels redundant. I would have done triggerStartStop like this:

function triggerStartStop() {
  intervalId = timeRunning ?
    clearInterval(intervalId) :
    setInterval(increment, 10) ;
  timeRunning = !timeRunning;
}

function increment() {
  currentTime += 10; // Increment
  currentTimeDisplay.textContent = // Write
    (currentTime / 1000).toFixed(2);
}


Validate parentheses "()" and "[]" are balanced Firstly I was using regex to get if the number of parentheses in a string is balanced or not, but the performance was quite slow when any large string was passed to the regex. So I created this custom method, which returns whether a string contains balanced parentheses or not. Please review this code and point out any mistakes and improvements.

checkBalancedBrackets(str) {
    var b1 = [];
    var b2 = [];
    for ( var i=0; i < str.length; i++ ) {
        var ch = str.charAt(i);
        if (ch == "(" ) b1.push(ch);
        else if ( ch == "[" ) b2.push(ch);
        else if ( ch == ")" ) {
            if (b1.length < 1) {
                return false;
            } else {
                b1.pop();
            }
        }
        else if ( ch == "]" ) {
            if (b2.length < 1) {
                return false;
            } else {
                b2.pop();
            }
        }
    }
    return b1.length == 0 && b2.length == 0;
}

A short review;

checkBalancedBrackets("([)]") returns true, not sure that was the intention
b1 and b2 are not great names, what does the b stand for?
Using arrays is not needed, all the code does is counting numbers, you can just use numbers
The inconsistent use of curly braces in your if and else if clauses is not good


Read numbers between 1 and 5, then print the minimum and maximum Just looking for some feedback regarding how to optimize my code.  It works, but just want some feedback on how to improve the solution.

Requirements:

Prompt for and read a number between 1 and 5. Repeat this step until the input is 1..5.
Repeat the following multiple times according to the number read in step 1.

a. Read in a list of integers ending with a 0. The 0 marks the end of
the input and is not considered part of the list
b. Print the largest and smallest integers in the list.
c. If only a    zero appears in the list, print an error message

CODE:

import java.util.ArrayList;
import java.util.Collections;
import java.util.Scanner;

public class SIMPLE_NUMBER_PROGRAM {

    public static void main(String[] args) {

        int input = prompt();
        ArrayList<Integer> inputList;
        inputList = new ArrayList();
        Integer min;
        Integer max;

        if (input == 0 && inputList.isEmpty()) {
            System.out.println("error: empty list!");
        } else {
            while (input < 1 && input != 0 || input > 5 && input != 0) {
                input = prompt();
            }
            while (input >= 1 && input <= 5) {
                inputList.add(input);
                input = prompt();
            }
            min = Collections.min(inputList);
            max = Collections.max(inputList);
            System.out.println("Min: " + min + "\n" + "Max: " + max);
        }

    }

    private static int prompt() {
        Scanner sc = new Scanner(System.in);
        System.out.println("Enter a number: ");
        return sc.nextInt();
    }

}

If you make the Scanner object outside of the prompt method and use that object in the prompt method when you want to get an input then you won't be creating a new instance of Scanner every time you call the prompt method.


OVH email manager script using docopt This is what I coded to realize a command line program to make it easier to manage emails on ovh shared hostings. It's working ok but it seems ugly.

I'm a beginner with Python and docopt.  What would you suggest?

#!/usr/bin/env python
# -*- coding: utf-8 -*-
''' OVH Email Manager (ovhEmailMan)
A small script that helps to add and remove one or more email addresses on the OVH shared domains

    Usage:
        ovh_mails.py list [--ugly]
        ovh_mails.py add (<address> [--pswd=<password>][--description=<description>] | --file <filename> [--notify])
        ovh_mails.py remove (<address> | --file <filename>)
        ovh_mails.py (-h | --help)

    Arguments:
        <password>                        Password to access the mailbox (if not provided it's random generated)
        <filename>                        Name of the files to process (csv). Check README to see how to format it

    Options:
        -h, --help                        Show this help message
        -u, --ugly                        Print without nice tables
        -p, --pswd=<password>             Set the password to the one provided
        -n, --notify                      If set, notification mail is sent using smtp credentials in ovh.conf

    Commands:
        list                              list all the email addresses currently configured
        add                               add one or more (configured in <filename>) email addresses
        remove                            remove one ore more (configured in <filename>) email addresses
    '''

import ovh
from docopt import docopt
from ovhem import EmailManager
from ovhem import fileprocesser as fp

if __name__ == '__main__':
    args = docopt(__doc__)
    #Validate args ---- TODO
    eman = EmailManager()

    # 'List' command parsing
    if args['list']:
        if args['--ugly']:
            eman.niceoutput = False

        eman.list_emails()
    # 'Add' command parsing
    elif args['add']:
        if args['<address>']:

            emails = (
                      {
                       'address': args['<address>'],
                       'password': None,
                       'description': None,
                       },
                      )
            if args['--description']:
                emails[0]['description'] = args['<description>']
            if args['--pswd']:
                emails[0]['password'] = args['<password>']
        if args['--file']:
            emails = fp.process_file(args['<filename>'])

        # Getting back the emails dict
        emails=eman.add_emails(emails)
        if args['--notify']:
            fp.send_notifications(emails)

    # 'remove' command parsing
    elif args['remove']:
        if args['<address>']:

            emails = (
                      {
                       'address': args['<address>'],
                       },
                      )
        if args['--file']:
            emails = fp.process_file(args['<filename>'])
        eman.remove_emails(emails)

Overall, it looks quite fine, with some smaller and bigger issues.

Parsing the add command

It's not so great to use the '<address>' string twice here:

    if args['<address>']:

        emails = (
                  {
                   'address': args['<address>'],
        # ...

You might mistype one of them. Or you might change something later and forget to update it everywhere. It would be better to put the value of args['<address>'] into a variable, and use that variable in evaluations and assignments.

This part is also awkward:

        emails = ( ... )
        if args['--description']:
            emails[0]['description'] = args['<description>']
        if args['--pswd']:
            emails[0]['password'] = args['<password>']

It would be much easier to use description and password variables to store defaults, then update these variables if --description or --pswd were given, and finally create the emails tuple.

Like this:

    address = args['<address>']
    if address:
        password = None
        description = None
        if args['--description']:
            description = args['<description>']
        if args['--pswd']:
            password = args['<password>']
        emails = (
            {
                'address': address,
                'password': password,
                'description': description,
            },
        )

Parsing the remove command

Here, if the second if statement is true,
it will overwrite the effect of the first:

    if args['<address>']:
        emails = (
            {
                'address': args['<address>'],
            },
        )
    if args['--file']:
        emails = fp.process_file(args['<filename>'])

That suggest that you should use an elif, and switch the order of the statements:

    if args['--file']:
        emails = fp.process_file(args['<filename>'])
    elif args['<address>']:
        emails = (
            {
                'address': args['<address>'],
            },
        )

emails might be undefined

Both the parsing of add and remove have a problem:
As their last step they do something with emails,
for example eman.add_emails(emails) and eman.remove_emails(emails),
but at that point the emails variable might be undefined.

This can be fixed by adding validation for these commands.
It looks that either of these must be true, checked in this order:

--file was specified and <filename> parameter was given
<address> parameter was given

For example:

if args['--file']:
    filename = args['<filename>']
    if filename:
        emails = fp.process_file(filename)
    else:
        raise Exception("Filename parameter missing")
else:
    address = args['<address>']
    if address:
        emails = ( ... )
    else:
        raise Exception("Address parameter missing")

# main action


iOS delegation design pattern I am working on a project that has a Model-View-Presenter structure and use the delegation design pattern that has the following structure/implementation. I would love feedback.

The ViewController will set up the MVP triad:

class BookViewController: UIViewController {

    private var bookModel: BookModel!
    private var bookView: BookView!
    private var bookPresenter: bookPresenter!

    override func loadView() {
        super.loadView()

        bookModel = BookModel()
        bookView = BookView()
        bookPresenter = BookPresenter(model: bookModel, view: bookView)

        view.addSubview(bookView)
    }
}

The Presenter will set itself up as the observer to both the View and Model:

class BookPresenter: BookModelObserver, BookViewObserver {

    let bookModel: BookModel
    var bookView: BookView

    init(model: BookModel, view: BookView) {
        self.bookModel = bookModel
        self.bookView = bookView

        self.bookModel.observer = self
        self.bookView.observer = self

        let book = self.model.currentBook
        self.bookView.setData(book)
    }
}

The BookView will have a protocol defined:

protocol BookViewObserver: class {

    func evt_bookView(bookView: BookView, didSelectItem: Book)
}

A user action on the View layer will emit an event to the Presenter:

class BookView: UIView, UITableViewDataSource, UITableViewDelegate {

    weak var observer: BookViewObserver?

    func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {
        observer?.evt_bookView(self, didSelectItem: books[indexPath.row])
    }
}

The Presenter will implement the BookViewObserver protocol, and handle that emitted event:

class BookPresenter: BookModelObserver, BookViewObserver {

    func evt_bookView(bookView: BookView, didSelectItem book: Book) {
        model.updateCurrentBook(book)
    }
}

The BookModel will have a protocol defined:

protocol BookModelObserver: class {

    func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook currentBook:     Book)
}

The Model will update itself, and emit an event back up to the Presenter:

class BookModel {

    private(set) var currentBook: Book!
    weak var observer: BookModelObserver?

    func updateCurrentBook(book: Book) {
        currentBook = book
        observer?.evt_bookModel(self, didUpdateCurrentBook: currentBook
    }
}

The Presenter will implement the BookModelObserver protocol, and handle that emitted event. In some cases, it has to update the View again, and thus will call a function on the BookView.

class BookPresenter: BookModelObserver, BookViewObserver {

    func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook book: Book) {
        var index: Int?
        for (key, value) in DataStore.defaultInstance.books.enumerate() {
            if value.id == book.id {
                index = key
                break
            }
        }

        if let _ = index {
            bookView.updateBook(book, atIndex: index!)
        }
    }
}

Finally the View will update itself, bringing everything full circle:

class BookView: UIView, UITableViewDataSource, UITableViewDelegate {

    func updateData(book: Book, atIndex index: Int) {
        books[index] = book
        tableView.reloadRowsAtIndexPaths([NSIndexPath(forRow: index, inSection: 0)], withRowAnimation: UITableViewRowAnimation.None)
    }
}

func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook book: Book) {
    var index: Int?
    for (key, value) in DataStore.defaultInstance.books.enumerate() {
        if value.id == book.id {
            index = key
            break
        }
    }

    if let _ = index {
        bookView.updateBook(book, atIndex: index!)
    }
}

I see a few problems in this block of code alone.

Ultimately, this could be written far more simply as:

func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook book: Book) {
    for (key, value) in DataStore.defaultInstance.books.enumerate() where value.id == book.id {
        bookView.updateBook(book, atIndex: index)
        break
    }
}

This eliminates a variable, and better yet, eliminates a force unwrap (that isn't even necessary if you just used the if let correctly).

class BookModel {

    private(set) var currentBook: Book!
    weak var observer: BookModelObserver?

    func updateCurrentBook(book: Book) {
        currentBook = book
        observer?.evt_bookModel(self, didUpdateCurrentBook: currentBook)
    }
}

We've got more unnecessary optional problems in this class.  There's not a particularly good reason for currentBook to be an implicitly unwrapped optional in this case.  It can cause nothing but problems.

There's currently nothing else to this class, so it's unclear why we need to keep a reference to the book at all, but our updateCurrentBook can be changed slightly to do the same thing, not need any unwrapping code, and not need the implicitly unwrapped optional (which again is only going to lend itself to a future Stack Overflow "fatal error found nil while unwrapping optional" question).

class BookModel {
    private(set) var currentBook: Book?
    weak var observer: BookModelObserver?

    func updateCurrentBook(book: Book) {
        currentBook = book
        observer?.evt_bookModel(self, didUpdateCurrentBook: book)
    }
}

Of course, this might be even better if we just used the didSet property observer method on the property itself:

class BookModel {
    private(set) var currentBook: Book? {
        didSet {
            if let newBook = currentBook {
                observer?.evt_bookModel(self, didUpdateCurrentBook: newBook)
            }
        }
    }

    weak var observer: BookModelObserver?
}

But now it seems weird that currentBook could be set to nil and the property observers not notified of that.  So here's what makes most sense to me:

class BookModel {
    var currentBook: Book {
        // a willSet probably also makes sense
        didSet {
            observer?.evt_bookModel(self, didUpdateCurrentBook: newBook)
        }
    }

    weak var observer: BookModelObserver?

    init(book: Book, observer: BookModelObserver? = nil) {
        self.book = book
        self.observer = observer
    }
}

In this case, the book property always has a good value.

Or, alternatively, we make it fully optional.  It doesn't matter if it's public or private.

Here's the biggest problem with your original implementation:

let fooBookModel = BookModel()
let fooBook: Book = fooBookModel.currentBook // crash, book is nil, it was never set

This is perhaps not how the class is intended to be used, but nothing at all about your code prevents this usage.

My sample prevents the empty initializer and forces this:

let barBook = Book()
let fooBookModel = BookModel(barBook)
let fooBook = fooBookModel.currentBook

Here, the currentBook property can never be nil.  And the property observer code is handled in a far more Swift-standard way versus the sort of Swift-weird setter approach in the original implementation.


You should investigate whether GNU Make (or whichever Make implementation your users are supposed to use) has a special variable for all dependencies of the current rule, so that you don't need to list the source files twice.
The command line for gcc is missing the -Wall -Wextra -O2 options to catch common mistakes and to make the program run faster.
If you want the Makefile to be portable to systems other than GNU Linux on x86_64, you should switch to GNU autoconf, but that's a lot of work. If your project is just a toy project, it's fine as it is.


Bootstrap Comment post HTML markup I am creating styles for the comment posts for a projects using BootStrap. I am wondering if this is good HTML markup and the extending of BootStrap's CSS.

Using HTML5 markup recommendations the HTML for the comment posts is as follows.

Comments

username

 That Guy

                <time class="comment-date" datetime="16-12-2014 01:05"> Dec 16, 2014</time>

                  Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

 reply

I tried to work off BootStrap's CSS and add my style to it.

The CSS is as follows

/*Comment List styles*/
.comment-list .row {
margin-bottom: 0px;
}
.comment-list .panel .panel-heading {
 padding: 4px 15px;
 position: absolute;
 border:none;
/*Panel-heading border radius*/
border-top-right-radius:0px;
top: 1px;
}
.comment-list .panel .panel-heading.right {
border-right-width: 0px;
/*Panel-heading border radius*/
border-top-left-radius:0px;
right: 16px;
}
.comment-list .panel .panel-heading .panel-body {
padding-top: 6px;
}
.comment-list figcaption {
/*For wrap text is thumbnail*/
word-wrap: break-word;
}
/* Portrait tablets and medium desktops */
@media (min-width: 768px) {
.comment-list .arrow:after, .comment-list .arrow:before {
content: "";
position: absolute;
width: 0;
height: 0;
border-style: solid;
border-color: transparent;
}
.comment-list .panel.arrow.left:after, .comment-list .panel.arrow.left:befor {
border-left: 0;
}
/*****Left Arrow*****/
/*Outline effect style*/
.comment-list .panel.arrow.left:before {
left: 0px;
top: 30px;
/*Use boarder color of panel*/
border-right-color: inherit;
border-width: 16px;
}
/*Background color effect*/
.comment-list .panel.arrow.left:after {
left: 1px;
top: 31px;
/*Change for different outline color*/
border-right-color: #FFFFFF;
border-width: 15px;
}
/*****Right Arrow*****/
/*Outline effect style*/
.comment-list .panel.arrow.right:before {
right: -16px;
top: 30px;
/*Use boarder color of panel*/
border-left-color: inherit;
border-width: 16px;
}
/*Background color effect*/
.comment-list .panel.arrow.right:after {
right: -14px;
top: 31px;
/*Change for different outline color*/
border-left-color: #FFFFFF;
border-width: 15px;
}
}
.comment-list .comment-post {
margin-top: 6px;
}

I have included a Bootply link to the example.

comment post example on Bootply

About your HTML

Removing all the div elements that seem to be needed only for presentation, you have this markup:

Comments

username

 That Guy

  <time class="comment-date" datetime="16-12-2014 01:05"> Dec 16, 2014</time>

Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

 reply

Element for all comments

The class name comment-list suggests that the section groups all comments (which would be the appropriate element for a list of comments). If that’s the case, the heading ("Comments") should be a child of this section.

Comments

Element for a single comment

For each comment, you should use the article element.

Comments

Element for CSS icons

Using the i element for CSS/font icons is not appropriate. Use span instead.

datetime format

Your time element’s datetime value is not in the correct format.

In your case it probably should be

<time datetime="2014-12-16T01:05">Dec 16, 2014</time>

Username / avatar

I don’t understand why you seem to reference two usernames ("username" in figure, "That Guy" in header), but using figure for the avatar doesn’t seem to be the best choice to me (not saying that it would necessarily be wrong).
Is the username really the caption of the avatar image? I’d rather think that these two, the avatar and the username, stand on their own. But if you think it make sense, keep it like that.


Sum of multiples of 3 or 5 below N How do I improve speed of this code? The problem definition is "Find the sum of all the multiples of 3 or 5 below N".

{-# LANGUAGE BangPatterns #-}
{-# OPTIONS_GHC -O2 -optc-O2 #-}
import qualified Data.ByteString.Lazy as L
import Control.Monad (forM_)

readInt :: L.ByteString -> Int
readInt !s = L.foldl' (\x c -> 10 * x + fromIntegral c - 48) 0 s

main :: IO ()
main = do
    -- don't need number of inputs, since it is read lazily.
    (_:ls) <- L.split 10 `fmap` L.getContents
    -- length ls <= 10^5
    forM_ ls $ \ l ->
        print $ f . readInt $ l

-- n <= 10^9
f :: Int -> Int
f n = go 0 0
  where
    go i !a | i == n = a
            | i `mod` 3 == 0 || i `mod` 5 == 0 = go (i+1) (a+i)
            | otherwise         = go (i+1) a

Specification

There is  some possible ambiguity in the specification: Are multiples of 15 counted once as in FizzBuzz or twice as in a more mathematical specification?

Mathematics

The general problem of summing a sequence of integers is well understood

The pseudo code for the FizzBuzz version of the problem:

   threes   = (n/3).toInteger
   fives    = (n/5).toInteger
   fifteens = (n/15).toInteger

   sigmaThrees = [threes * (threes + 1) / 2] * 3
   sigmaFives = ...
   sigmaFifteens = ...

   return sigmaThrees + sigmaFives - sigmaFifteens

Implementation

The implementation should use Haskell's quot to perform integer division.

Performance

Blazingly fast. Not just because there are few calculations but also quot usually runs as a hardware integer operation.


Receiving a user's registration submission and inserting row into database #2 I asked this question before and mickmackusa and Your Common Sense gave me some good answers. I went over them and made as much changes as I could because some of the code didn't work properly for me. So again i want to know if this is a good way to have my registration code for people to sign up.
//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) {

    // looks like a hack, send to index.php
    header('Location: ../../index.php');
    die();
}

require '../../config/connect.php';
$con = new mysqli(...$dbCredentials);

$first_name = $_POST['first_name'] ?? '';
$last_name = $_POST['last_name'] ?? '';
$username = $_POST['username'] ?? '';
$email = $_POST['email'] ?? '';
$pw = $_POST['pw'] ?? '';
$pw2 = $_POST['pw2'] ?? '';

$errors = [];

if (!trim($first_name)) {

    $errors[] = "Fill in first name to sign up";
}
if (!ctype_alnum($first_name)) {
    $errors[] = "Invalid first name, it only may contain letters or digits";
}

if (!trim($last_name)) {

    $errors[] = "Fill in last name to sign up";
}
if (!ctype_alnum($last_name)) {
    $errors[] = "Invalid last name, only letters and/or digits.";
}

if (!trim($username)) {

    $errors[] = "Fill in last name to sign up";
}
if (!ctype_alnum($username)) {
    $errors[] = "Invalid last name, only letters and/or digits.";
}

if (!trim($pw)) {
    $errors[] = "Fill in password to sign up";
}
if (!trim($pw2)) {
    $errors[] = "Confirm password to sign up";
}

if (!trim($email)) {
    $errors[] = "Fill in email to sign up";
}
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
    $errors[] = "Invalid email";
}

if ($pw !== $pw2) {

    echo "Your passwords do not match";
}

if (!$errors) {

    $check_email = $con->prepare("SELECT username FROM users WHERE username=?");
    $check_email->bind_param("s", $username);
    $check_email->execute();
    $row = $check_email->get_result()->fetch_assoc();

    if ($row) {

        echo "That username is already in use";
        die();
    }
}

if (!$errors) {

    $check_username = $con->prepare("SELECT email FROM users WHERE email=?");
    $check_username->bind_param("s", $email);
    $check_username->execute();
    $row = $check_username->get_result()->fetch_assoc();

    if ($row) {
        echo "That email is already in use";
        die();
    }
}

if (!$errors) {

    $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, array('cost' => 14));

    $stmt = $con->prepare("INSERT INTO users (first_name, last_name, username, email, pw)
        VALUES (?, ?, ?, ?, ?)");
    $stmt->bind_param("sssss", $first_name, $last_name, $username, $email, $pw);
    $stmt->execute();

    $_SESSION["id"] = $_POST['username'];
    header("Location: ../../index.php");
    exit();

} else {
    // The foreach construct provides an easy way to iterate over arrays.
    foreach ($errors as $error) {
        $errors[] = 'An error occurred.';
    }

}

While it is sensible to some people to precisely specify the cause of the submission failure, I prefer to return compound messages rather than a length error message and a quality error message.  I would probably save on screen space (versus piling upto a dozen separate email messages on the user), and merge some responses like:

First name is a required field and may only contain letters and/or digits.

Also, you are repeating yourself a few times with these error messages and you can easily implement a looped battery of checks -- and it is simpler to do that within the $_POST array.  Not only does this avoid some code bloat, you are assured to have consistent error message and reduce your chances of copy-pasting typos.  (Maybe you didn't realize that you wrote last name in the username errors  -- this is the kind of thing that really perplexes users!)
Note: ctype_alnum() will return false on a zero-length string so the !trim() can be omitted.
$alnums = ['first_name', 'last_name', 'username'];  // whitelist of required alphanumeric fields
foreach ($alnums as $field) {
    if (!isset($_POST[$field]) || !ctype_alnum($_POST[$field])) {
        $errors[] = "$field is a required field and may only contain letters and/or digits.";
    }
}

if (!isset($_POST['email']) || !filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
    $errors[] = "Email is a required field and must be valid.";
}

if (!isset($_POST['pw'], $_POST['pw2']) || $pw !== $pw2) {
    $errors[] = "Password and Password 2 are required fields and must be identical.";
}

Ultimately, all of these server-side checks are to protect you and your database -- NOT to help your users.  For the best possible user experience (UX), you need to duplicate all of these validations with javascript onsubmit of the form.  This way you inform the user as quickly as possible AND avoid making a fruitless trip to your server.
You can check for unique usernames and email addresses in a single trip to the database.  Because the earlier validation assures no leading or trailing whitespace, the posted data will match the respective result set values (iow, no trimming is needed).
if (!$errors) {
    $stmt = $con->prepare("SELECT username, email FROM users WHERE username=? OR email=?");
    $stmt->bind_param("ss", $_POST['username'], $_POST['email']);
    $stmt->execute();
    foreach ($stmt->get_result() as $row) {
        foreach (['username', 'email'] as $field) {
            if ($row[$field] == $_POST[$field]) {
                $errors[] = "Sorry, the submitted $field is already in use.";
            }
        }
    }
}

Once you have fully validated all incoming data as valid, THEN you can happily insert the new user into your database and save the SESSION data.  By the way, I don't recommend changing the naming convention from username to id.
If any of the checkpoints are failed then present your $errors.  As already pointed out, you are not actually displaying your error messages.  In simplest terms, you could create
 tags as you loop.
if (!$errors) {
    $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, ['cost' => 14]);
    $stmt = $con->prepare("INSERT INTO users (first_name, last_name, username, email, pw) VALUES (?, ?, ?, ?, ?)");
    $stmt->bind_param(
        "sssss",
        $_POST['first_name'],
        $_POST['last_name'],
        $_POST['username'],
        $_POST['email'],
        $_POST['pw']
    );
    $stmt->execute();

    $_SESSION["username"] = $_POST['username'];  // I always expect an id to be an integer
    header("Location: ../../index.php");
    exit();
}

foreach ($errors as $error) {
    echo "
$error
";
}

None of the above snippets have been tested; I make no guarantees that they will work "out of the box".


Forming a dictionary where all values are only conditionally set I have lots of dictionary keys with conditional values, ternary in this case:

# cover page
data_coverPage.append({'reportCalendarOrQuarter': coverPage.reportcalendarorquarter.string if coverPage.reportcalendarorquarter else "N/A",
            'filingManager_name': coverPage.filingmanager.find_all('name')[0].text if coverPage.filingmanager.find_all('name') else "N/A",
            'filingManager_street1': coverPage.find('com:street1').string if coverPage.find('com:street1') else "N/A",
            'filingManager_city': coverPage.filingmanager.find('com:city').text if coverPage.filingmanager.find('com:city') else "N/A",
            'filingManager_stateOrCountry': coverPage.filingmanager.find('com:stateorcountry').text if coverPage.filingmanager.find('com:stateorcountry') else "N/A",
            'filingManager_zipCode': coverPage.filingmanager.find('com:zipcode').text if coverPage.filingmanager.find('com:zipcode') else "N/A",
            'reportType': coverPage.reporttype.string if coverPage.reporttype else "N/A",
            'form13FFileNumber': coverPage.form13ffilenumber.string if coverPage.form13ffilenumber else "N/A",
            'provideInfoForInstruction5': coverPage.provideinfoforinstruction5.text if coverPage.provideinfoforinstruction5 else "N/A"
            })

data.append(data_coverPage)
# print data

# info tables
for it in bf.find_all('infotable'):
    # print it.nameofissuer.text
    data_infoTable.append({'nameOfIssuer': it.nameofissuer.text if it.nameofissuer else "N/A",
                 'titleOfClass': it.titleofclass.text if it.titleofclass else "N/A",
                 'cusip': it.cusip.text if it.cusip else "N/A",
                 'value': it.value.text if it.value else "N/A",
                 'sshPrnamt': it.shrsorprnamt.sshprnamt.text if it.shrsorprnamt else "N/A",
                 'sshPrnamtType': it.shrsorprnamt.sshprnamttype.text if it.shrsorprnamt.sshprnamttype else "N/A",
                 'investmentDiscretion': it.investmentdiscretion.text if it.investmentdiscretion else "N/A",
                 'putCall': it.putCall.string if it.putCall else "N/A",
                 'otherManager': it.otherManager.string if it.otherManager else "N/A",
                 'sole': it.votingauthority.sole.text if it.votingauthority.sole else "N/A",
                 'shared': it.votingauthority.shared.text if it.votingauthority.shared else "N/A",
                 'none': it.votingauthority.none.text if it.votingauthority.none else "N/A"})

Is there a much shorter way to represent so many conditional values? I'm repeating "else 'N/A'" throughout that block of code. I could store "N/A" with a variable and refer to that variable instead of the raw string. Other than that, how would I keep things essentially DRY in this situation?

The easiest thing to do, seeing how similar most of these are, would be to have a lambda for the ternary if.

gettext = lambda var: var.text if var else "N/A"
'titleOfClass' : gettext(it.titleofclass)

If you do one for .string as well, then you only have a few (3?) cases where your if cannot be shortened to a lambda call.  You could pass in another lambda to pull the value you want to store out of var, but for just three cases the added indirection doesn't seem useful.


Reduce repetition when sharing data across an application I have a CodeIgniter 3 application. There is some data every page across the application needs to have.

This is the system I currently have.

Create a model function that will get the data I need

Class General_model extends CI_Model
{
    function data_init()
    {
        $data['session'] = $this->session->userdata('session');
        $data['foobar'] = $this->general_model->get_foobar();
        return $data;
    }
}

In every controller, I call this data in the __construct()

function __construct()
{
    parent::__construct();
    $this->load->model('general_model', '', TRUE);
    $this->data = $this->general_model->data_init();
}

I begin every controller function by getting the data in the scope of the function, which then gets passed to the view

public function index()
{
    $data = $this->data;
    $data['page_title'] = 'example';
    $this->load->view('templates/header', $data);
    $this->load->view('page/example', $data);
    $this->load->view('templates/footer', $data);
}

I feel this is not ideal. It feels repetitive and not dry to have $this->data = $this->general_model->data_init(); in every constructor and $data = $this->data; in every function.

It feels like it'd be better if I could declare $this->data = $this->general_model->data_init(); once in the application and have $this->data available to every function in every controllers. I'd also like to avoid $data = $this->data; at the start of every function, but I'd rather do that than have the additional 7 characters $this-> hundreds of times across the codebase (perhaps that is me being lazy?).

I'm not sure how to improve either of those without breaking OOP and going global, or touching CodeIgniter core classes such as CI_Model.

Is there a way to improve how I'm sharing and moving this data?

I would not get all worked up about having to type a few extra letters when referencing a variable.  I could get worked up over possibly doubling the storage space required for your data depending on it's size. Let's walk though what PHP is doing behind the scenes here to understand:

// PHP references $this->data to $data by reference
$data = $this->data;

// since you modify $data here, PHP now copies the entirety of $this->data
// to $data and breaks reference
// you are now taking up double (plus some) storage for your data
$data['page_title'] = 'example';

As opposed to directly modifying $this->data before making assignment to $data like shown below , which would not double memory utilization:

$this->data['page_title'] = 'example';
$data = $this->data;

Just remember that as soon as you modify an array assigned in this manner, you remove the ability for PHP to perform it's behind the scenes memory optimization. But really you should not be relying on this anyway, as IMO it is bad coding form to be relying on some hidden, undocumented and possibly subject to change optimization for your code to be working in the best manner.

Alternately, instead of associative array for data, why not use an object (even stdClass), then you are always working with reference to the object (unless you specifically clone it)? For example:

// assuming $this->data holds object
// $data is just reference to $this->data here
$data = $this->data;

// here modifying $data would modify both $data and $this->data
$data->page_title = 'example';

This also gives you the possible side benefit of being able to type hint against an object in method signatures as you are passing this data around or injecting it as dependency.


Two simple sorting algorithms in Python For a personal programming challenge, I wanted to try writing simple bubble sort and insertion sort functions in Python 3. I haven't looked at the standard pseudo-code, I just read about how they worked. Are these good implementations?

def bubble_sort(array):
    """
    Sorts array using a bubble sort.

    >>> bubble_sort([43, 10, 100, 24, 1, 6, 10, 3])
    [1, 3, 6, 10, 10, 24, 43, 100]
    """
    array2 = array[:] # Save a copy, so that original is not mutated
    last_index = len(array) - 1 # Iterate up to this position
    while last_index > 0:
        for i in range(last_index):
            a, b = array2[i], array2[i + 1] # Consecutive numbers in array
            if a > b:
                array2[i], array2[i + 1] = b, a # Swap positions
        last_index -= 1 # A new number has bubbled up, no need to inspect it again
    return array2

def insertion_sort(array):
    """
    Sorts array using an insertion sort.

    >>> insertion_sort([43, 10, 100, 24, 1, 6, 10, 3])
    [1, 3, 6, 10, 10, 24, 43, 100]
    """
    sorted_array = []
    for a in array:
        # Loop backwards through sorted_array
        for i, b in reversed(list(enumerate(sorted_array))):
            if a > b:
                sorted_array.insert(i + 1, a) # Insert a to the right of b
                break
        else:
            # a is less than all numbers in sorted_array
            sorted_array.insert(0, a) # Add a to beginning of list
    return sorted_array

The docstrings and doctests are nice.  I would prefer a more explicit description of the behaviour: "Return a copy of the array, sorted using the bubble sort algorithm."  Typically, if you're implementing these sorting algorithms as an exercise, you would perform the sorting in place.

In bubble_sort(), the while last_index > 0 loop would be written more idiomatically as:

for last_index in range(len(array) - 1, 0, -1):
    …

In insertion_sort(), the reversed(list(enumerate(sorted_array))) would be making temporary copies of sorted_array.  I would therefore consider it an improper implementation of the algorithm.


Sass code for displaying two simple search forms Sass code for creating a basic search box. Resulting CSS used for two that are different widths.

The goals is to define sass that generates CSS that is easily re-usable as well as concise and efficient and to keep the initial definitions as DRY as possible.

Review:

Coding style
Re-usability
DRY principles

SASS:

$container-width:940px;
$search-container-padding:54px;
$form-width:$container-width - ($search-container-padding * 2);
$form-height:47px;
$button-width:118px;

$search-container-background:#e9e9e9;
$form-background:#fff;
$input-border:1px solid #dadadc;
$focus-border:1px solid #052546;
$input-br:4px;
$input-inset:15px;

/*
    input width, sets the width of an imput and adds polyfill placeholder correctly
*/
@mixin input-width($width)
{
    .search-input
    {
        width:$width;
    }
    .search-placeholder
    {
        left:$form-width - $width - $button-width + $input-inset;
    }
}

.search-container
{
    box-sizing:border-box;

    width:$container-width;

    background-color:$search-container-background;

    padding:30px $search-container-padding 0;
}
.case-search-container
{
    padding-bottom:20px;
}
.book-search-container
{
    height:124px;
}

.search-form
{
    box-sizing:padding-box;

    width:$form-width;
    height:$form-height;

    position:relative;

    background-color:$form-background;

    border:5px solid $form-background;
    border-radius:6px;

    input[type=submit],
    button[type=submit]
    {
        width:$button-width;
        height:$form-height;

        position:absolute;
        top:0;
        right:0;

        background:image-url("structure/button-sprite.png") 0px 0px no-repeat;
        cursor:pointer;

        border:0px;

        color:#fff;
        font-weight:bold;
        text-transform:uppercase;

        &:hover{
            background-position:0px 0 - $form-height;
        }
    }

    .text
    {
        input
        {
            box-sizing:border-box;

            height:$form-height;

            position:absolute;
            top:0;
            right:$button-width;

            border-top:$input-border;
            border-bottom:$input-border;
            border-left:$input-border;
            border-radius:$input-br 0 0 $input-br;
            margin:0;
            padding:10px $input-inset;

            &:focus
            {
                outline:none;
                border:$focus-border;
            }

        }
    }

    .search-placeholder
    {
        position:absolute;
        top:13px;
        z-index:10;

        font-size:15px;
    }
}

/* specific width over-rides and label placement relative to this */
.book-search-form
{
    @include input-width(714px);
}
.case-search-form
{
    @include input-width(455px);
}

HTML:

    <form class='search-form'>

            <input class='search-input' type='text' ... />

            <input type='submit' ... />

    </form>

    <form class='search-form'>

            <input class='search-input' type='text' ... />

            <input class='submit' ... />

    </form>

Sass

If this is meant to be a reusable Sass library, you'll want to make use of default variables so that they're easier to override:

$container-width: 940px !default;
$search-container-padding: 54px !default;

You'll want to do something similar with your mixins so that the values can be changed for one-off instances:

@mixin input-width($width, $form-width: $default-form-width, $button-width: $default-button-width, $input-inset: $default-input-inset)
{
    .search-input
    {
        width:$width;
    }
    .search-placeholder
    {
        left:$form-width - $width - $button-width + $input-inset;
    }
}

You'll probably want to avoid generating styles by default, since the user might not want them.  It is a common request for people who use libraries like Bootstrap and Foundation:  how do I make it stop generating default styles?  Instead, you might want to consider having a separate file that can be imported that generates these styles.

The $input-br variable has an ambiguous name.  When I initially saw it in the source, I thought it was for the br element.  It wasn't until later I realized br was being used as an abbreviation for border-radius (none of your other variables are abbreviated).

Sass has a bit of a shortcut you can use for some properties:

border: {
  top: $input-border;
  bottom:$input-border;
  left:$input-border;
  radius: $input-br 0 0 $input-br;
}

CSS

If the background image for your button fails to load, the button is completely invisible because it is white text on a white background.

The use of px units here is sub-optimal.  When your containers contain text, you should be using a unit that's relative to the font-size, like em or ex, to specify its measurements.  If the user changes their browser's font settings in such a way that they want their font size to be 20px at the bare minimum, then your content is going to spill out of these containers.

Using archaic widths of yesteryear (900px~1024px) are very unfriendly towards mobile devices.  Set a max-width if you must, but avoid setting hard widths that are likely to cause horizontal scrolling.  Better yet, look into responsive design.

HTML

Placeholder text is not meant to be a replacement for labels, please don't treat it as such.  It should be thought of as example content only.

Labels must have a closing tag.  If they're not containing the input element that they're for, they need to have a for attribute and the associated input element needs to have an id with the matching value.

There's a new input type of search that I recommend using instead of text for your search input fields.  Browsers that don't support search will fallback and use text anyway.

Your 2nd submit button has a class of submit rather than a type.