Best approach for the Design Pattern for multiple schedulers using interface and abstract class I have a requirement to fetch data for different types of Users (Customers, Publishers etc) for three different timelines (retro, current, and past), all have different URLs. There is a scheduler which will run at the different interval (for example 2 minute for retro data, 5 minutes for current data and 10 minutes for Future Dated Data). I have attached an image for better understanding.

 interface IDataPublisher {
   void GetUrl();
   void Publish();
 }

  interface ITimeLineGrabber {
     TimeSpan GetTimeSpanForNextRun();
  }

 abstract class BasePublisher : IDataPublisher, ITimeLineGrabber, BackgroundService
 {
    private readonly ILogger<BasePublisher> _logger;
    public BasePublisher (ILogger<BasePublisher> logger)
    {
        _logger = logger;
    }

    public abstract string GetUserType();
    public abstract string GetDataType();
    public abstract TimeSpan GetTimeSpanForNextRun();
    public abstract string GetUrl();

    public override async Task StartAsync(CancellationToken cancellationToken)
    {
      await base.StartAsync(cancellationToken);
    }

    public override async Task StopAsync(CancellationToken cancellationToken)
    {
      await base.StopAsync(cancellationToken);
    }

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
      var userType = GetUserType();
      var dataType = GetDataType();
      TimeSpan timeSpan = GetTimeSpanForNextRun();

      while (!stoppingToken.IsCancellationRequested)
      {
            try
            {
                // Do Something with userType and dataType
                await Task.Delay(timeSpan , stoppingToken);
            }
            catch (Exception ex)
            {
                _logger.LogError($"Error - {ex.Message}", DateTimeOffset.Now);
            }
        }
   }

  public sealed class CustomerRetroPublisher: BasePublisher
  {
    public override string GetUserType()
    {
       return "Customer";
    }

    public override string GetDataType()
    {
      return "Retro";
    }

    public override Timespan GetTimeSpanForNextRun()
    {
      return Timespan.FromMinutes(2); // read from config file..
    }

    public override string GetUrl()
    {
       return "CustomerRetroUrl";
    }
  }

but this will end up with 12 different classes and loads of code will be duplicated/ over complicated. I have a feeling there must be a simple approach to all this. Just wondering what could be the best approach for my requirement.

It seems not a good idea to hard code the different constants. I would inject them through the constructor (I am using properties instead of Get-methods):
public sealed class Publisher : interfaces
{
    public void Publisher (string userType, string dataType, Timespan interval, string url)
    {
        UserType = userType;
        DataType = dataType;
        Interval = interval;
        Url = url;
    }

    public string UserType { get; }
    public override string DataType { get; }
    public override Timespan Interval { get; }
    public override string Url { get; }
}

Then create a factory
public static class PublisherFactory
{
    public static Publisher CreateCustomerRetroPublisher()
    {
        return new Publisher("Customer", "Retro", Timespan.FromMinutes(2), "CustRetroUrl");
    }

    ...
}

If you need different behaviors for different publishers, you can inject them this way as well. For instance, inject services for timeline-specific behavior (through a ITimelineService) and user type-specific behavior (through a IUserService). Like this you can combine the different behavior types in different ways. 3 timeline services + 4 user services = 7 services is better than 12 different classes.
The constructor would then have additional parameters
public void Publisher (string userType, string dataType, Timespan interval, string url,
    ITimelineService timeLineService, IUserService userService)

The services can be assigned to private fields, as they do not need to be visible publicly.
See also: Composition over inheritance (Wikipedia)


A game that is going somewhere I'm working on a clicker game in Tkinter that is very different from Bubble Blaster:

from tkinter import *
from tkinter.messagebox import *
from time import sleep
import math

extraCashCost = 50

class AppUI(Frame):

    def __init__(self, master=None):
        self.saves = open("self.saves.log", "r")
        self.savesEdit = open("self.saves.log", "a")
        self.saveSring = ''.join(self.saves.read())

        Frame.__init__(self, master, relief=SUNKEN, bd=2)

        self.menubar = Menu(self)

        menu = Menu(self.menubar, tearoff=0)
        self.menubar.add_cascade(label="Game", menu=menu)
        menu.add_command(label="Open Store", command=lambda:storewin())
        menu.add_command(label="Browse Saves", command=lambda:showinfo("WIP", "This is a work in progress game"))

        menu = Menu(self.menubar, tearoff=0)
        self.menubar.add_cascade(label="Edit", menu=menu)
        menu.add_command(label="Open the developer window", command=lambda:devwin())

        try:
            self.master.config(menu=self.menubar)
        except AttributeError:
            # master is a toplevel window (Python 1.4/Tkinter 1.63)
            self.master.tk.call(master, "config", "-menu", self.menubar)

        self.money = 0
        self.cashGet = 1
        self.moneyShow = Label(root, text="$" + str(self.money))
        self.moneyGet = Button(root, text="Get Money!", command=lambda:cashin(self.cashGet))
        self.moneyGet.pack(side=BOTTOM)
        self.moneyShow.pack()

def cashin(cash):
    sleep(0.125)
    app.money += int(cash)
    app.moneyShow.config(text="$" + str(app.money))
def devcashadd(moneyz):
    app.money += int(moneyz)
    app.moneyShow.config(text="$" + str(app.money))
def devwin():
    dev = Tk()
    dl = Label(dev, text="Welcome to the developer window. \n Here you will have access to developer tools. \n This is available only in developer versions.").pack()
    mcl = Label(dev, text="Enter a number below and press the button under it to get free money!")
    mc = Entry(dev)
    mca = Button(dev, text="Get Money", command=lambda:devcashadd(mc.get())).pack()
    mc.pack()
    dev.mainloop()
def ec(cs):
    global extraCashCost
    if app.money >= extraCashCost:
        extraCashCost = math.ceil(extraCashCost * 2.2)
        app.money -= extraCashCost
        app.cashGet *= 1.6
        app.moneyShow.config(text="$" + str(app.money))
        cs.config(text="$" + str(extraCashCost))

def storewin():
    global extraCashCost
    store = Tk()
    extraCashShow = Label(store, text="$" + str(extraCashCost))
    extraCash = Button(store, text="More Money!", command=lambda:ec(extraCashShow))
    extraCashShow.pack()
    extraCash.pack()
    store.mainloop()

root = Tk()

app = AppUI(root)
app.pack()

root.mainloop()

It is going to be on GitHub soon. Anything that could be changed to make it run faster?
And also, how can I put custom styles on the widgets?
Like background gradients, images, etc...

Don't use wildcard imports

You had this same advice on your prior post. You apparently decided to ignore it. It doesn't make you look smarter.

Use import tkinter as tk if you must. But put some kind of prefix on Tk functions so that later readers of your code - including yourself months from now - will know when a function call or class reference is to an outside library:

class AppUI(Frame):   # ?

class AppUI(tk.Frame): # Aha!

Use super in __init__

Instead of calling Frame.__init__ you should call super like so:

super().__init__(...)

See this article by Raymond Hettinger for more details.

Don't hold open files you don't use

You open files in __init__ that you don't use and don't close. Avoid this. Instead, open/read/close or open/write/close them when you actually need to. The open operations are cheap compared to writing or reading from a file, and file handles are a scarce resource.

Also, doing your open/read or open/write at the same time allows you to use the preferred Python idiom, with, for your file I/O:

with open('savegame.txt', 'r') as save:
    savetext = save.read()
    ...

The with model handles errors well and automatically closes the resource when you're done, so you don't have to worry about it.

Move your GUI code out of __init__

Do what you need to do to set up member data, then delegate your GUI setup to another method. This makes init easier to understand, and you will likely find some code reuse once you have the "create a method" approach in mind.

Use bound methods rather than global variables

You have a callback like so:

def cashin(cash):
    ...
    app.money += int(cash)

But app is a global variable that points to your AppUI. How do you call it? With this line, set up in __init__:

    self.moneyGet = Button(root, text="Get Money!", command=lambda:cashin(self.cashGet))

So you're inside self.__init__, passing a function as a command to a button. But inside the function, you have to look up a global variable to figure out the value of self?

Try passing a bound method, or using a closure, for this kind of thing.

command=self.cashin  # no parens, it's a reference not a call

Wrap your top-level code in a function

There's a standard idiom for top-level code in Python. Use it!

def main():
    root = Tk()
    app = AppUI(root)
    app.pack()
    root.mainloop()

if __name__ == '__main__':
    main()

Encapsulate!

You pass root into AppUI() as a parameter. So why are you calling root.mainloop()? Why don't you encapsulate this in your AppUI?

app = AppUI()  # Let __init__ figure out to call tk.Tk()
app.run()      # app.run can call self.master.mainloop()


Setters dependent on other instance variables in Java I am relatively new to java and I am trying to set up a class with non-trivial setters, but I wonder if setting an instance variable via another one is allowed/good practice?
public class Pdf {

    private byte[] bytearray;
    private String name;
    private PDDocument pddocument;
    private int lastPage;

    public Pdf() {
        super();
    }

    public Pdf(String name, byte[] bytearray) {
        super();
        this.name = name;
        this.bytearray = bytearray;
    }

    public String getName(String name) {
        return name;
    }

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

    public byte[] getBytearray() {
        return bytearray;
    }

    public void setBytearray(byte[] bytearray) {
        this.bytearray = bytearray;
    }

    public PDDocument getPddocument() {
        return pddocument;
    }

    public void setPddocument() {

        PDDocument pddocument = null;
        try {
            pddocument = org.apache.pdfbox.pdmodel.PDDocument.load(this.bytearray);
        } catch (IOException e) {
            e.printStackTrace();
        }
        this.pddocument = pddocument;
    }

    public int getLastPage() {
        return lastPage;
    }

    public void setLastPage() {

        this.lastPage = this.pddocument.getNumberOfPages()-1;
    }
}

Allowed?
Yes. There seems to be nothing technically wrong with your code.
Good practice?
No.
Specifically, in your example code setPddocument is not actually a setter.
It is a load / initialize function, that does not set a value in the object, but instead performs an operation under the assumption a certain value (bytearray) has already been set.
It would be better off named init or load, and you probably should set up some code to check if there is a valid array, throwing a different exception in that case. (Like InvalidStateException).
setLastPage is also more of an initializer then a setter.
There, you have no error checking at all, so if it is called before setPddocument your program will crash.
While it is possible to have non-trivial setters that modify several instance variables, the general rule of thumb is: if it does not receive a value from outside to do its job, it is not a setter.


Advanced love calculator that generates a result based on the similarities between the two names My love calculator gives a result from 0 to 100% based on these factors:

If the first letters of the names are the same.
If the number of vowels is the same.
If the length of the name is the same.
If the number of consonants is the same.

Also, there is an extra boost of 10-50%, so the result is also partially as a result of luck.
Can the coding below be simplified or improved to function more smoothly?:

(Edit 1: By the way, I'm very new at Python - I joined the community only yesterday.)

(Edit 2: I am using Python 3.7.3 for Mac OSX.)

name1 = input("Please type Name 1.\n")
name2 = input("Please type Name 2.\n")

total_vowel1 =0
for i in ['a','e','i','o','u']:
     total_vowel1 += name1.count(i)
total_vowel2 =0
for i in ['a','e','i','o','u']:
     total_vowel2 += name2.count(i)

love = 0
if(total_vowel1 == total_vowel2):
    import random
    love +=random.randint(10,30)

consonants1 = 0
consonants2 = 0
CONSONANTS = 'bcdfghjklmnprstvwxyz'
consonants1 = len([letter for letter in name1 if letter.lower() in
CONSONANTS])
consonants2 = len([letter for letter in name2 if letter.lower() in
CONSONANTS])

if(consonants1 == consonants2):
    import random
    love +=random.randint(20,40)

line1 = name1
line2 = name2
split1 = line1.split()
split2 = line2.split()
fl1 = [word[0] for word in split1]
fl2 = [word[0] for word in split2]

if (fl1 == fl2):
    import random
    love +=random.randint(10,30)

if (len(name1) == len(name2)):
    import random
    love+=random.randint(1,10)

import random
love +=random.randint(10,50)
if (love>100):
    love = 100

print("Calculating...")
import time
import random
time.sleep(random.randint(1,3))

print("",name1,"and",name2,"have a",love,"% relationship.")
if ((love>90) or (love == 90)):
    print("They have an unbreakable relationship that will last
    forever.")
if ((love<89) or (love == 89)) and ((love>70) or (love == 70)):
    print("They have a strong relationship that will most likely
    lead to a marriage.")
if ((love<69) or (love == 69)) and ((love>50) or (love == 50)):
    print("They have a good relationship that can lead to a
    honeymoon to Paris.")
if ((love<49) or (love == 49)):
    print("They have a weak relationship that could have been a
    'match made in heaven'.")

First, your definition of consonants can be simplified a bit using set operations:

from string import ascii_lowercase

# Storing vowels in a set instead of a list
vowels = {'a', 'e', 'i', 'o', 'u'}

# Read as "Take the set of lowercase letters, and remove the vowels"
consonants = set(ascii_lowercase) ^ vowels

The vowel counting code below that is repetitious. You have the same counting loop twice. This is a good place to make use of a function:

def count_vowels(name):
    count = 0
    for i in vowels:
        count += name.count(i)

    return count

total_vowel1 = count_vowels(name1)
total_vowel2 = count_vowels(name2)

Instead of importing random a few times throughout the script, just import it at the top once. Unless you have a good reason to hold off on importing something until you really need it, just import any modules right at the top for clarity.

Your spacing is kind of a mess. At different points you have:

total_vowel1 =0
consonants1 = 0
love +=random.randint(10,50)
((love<49) or (love == 49))
love +=random.randint(10,30)
love+=random.randint(1,10)

See the problem? You're either being wildly inconsistent in your spacing, or you aren't caring to make sure your code readable and nice looking. Pick a spacing style and stick to it. I'd recommend putting spaces around infix operators (love < 49 or love == 49).

You're using two different counting and naming methods for vowels and consonants: a full loop, and a comprehension:

total_vowel1 =0
for i in ['a','e','i','o','u']:
     total_vowel1 += name1.count(i)

. . .

consonants1 = len([letter for letter in name1 if letter.lower() in consonants])

Again, try to be consistent. You or someone else may need to read your code later, and consistency greatly helps readability.

((love>90) or (love == 90)) is needlessly complicated. Just write love >= 90.

In that same area, you have a bunch of ifs. They're all exclusive of each other though, so really, all the ifs after the first should be elifs.

if love <= 89 and love >= 70 (which I fixed up) could be simply written as 70 <= love <= 89. Python, unlike most languages, allows for comparison chaining.

The last case can just be an else. By elimination, if the other cases weren't picked, the last one must be.

After taking all that into consideration and fixing some more spacing, I ended up with:

from string import ascii_lowercase
import random
import time

name1 = input("Please type Name 1.\n")
name2 = input("Please type Name 2.\n")

vowels = {'a', 'e', 'i', 'o', 'u'}
consonants = set(ascii_lowercase) ^ vowels

def count_vowels(name):
    count = 0
    for i in vowels:
        count += name2.count(i)

    return count

total_vowel1 = count_vowels(name1)
total_vowel2 = count_vowels(name2)

love = 0
if(total_vowel1 == total_vowel2):
    love += random.randint(10, 30)

consonants1 = len([letter for letter in name1 if letter.lower() in consonants])
consonants2 = len([letter for letter in name2 if letter.lower() in consonants])

if(consonants1 == consonants2):
    love += random.randint(20, 40)

line1 = name1
line2 = name2
split1 = line1.split()
split2 = line2.split()
fl1 = [word[0] for word in split1]
fl2 = [word[0] for word in split2]

if (fl1 == fl2):
    love += random.randint(10,30)

if (len(name1) == len(name2)):
    love += random.randint(1,10)

love += random.randint(10,50)

if (love > 100):
    love = 100

print("Calculating...")

print(name1, "and", name2, "have a", love, "% relationship.")

if love >= 90:
    print("They have an unbreakable relationship that will last forever.")

elif 70 <= love <= 89:
    print("They have a strong relationship that will most likely lead to a marriage.")

elif 50 <= love <= 69:
    print("They have a good relationship that can lead to a honeymoon to Paris.")

else:
    print("They have a weak relationship that could have been a 'match made in heaven'.")


How to observe nested objects By having two model classes Conversation and Message, what are the best practices to handle the next situation: A conversation listening for its messages PropertyChanged events and so being able to update itself.

What are the best practices?
How can I improve this model design?
Is it going to generate memory leaks?

code

using SoftConsept.Collections;

public class Conversation
{
    readonly SortedObservableCollection<Message> messages;

    public Conversation ()
    {
        messages = new SortedObservableCollection<Message> ();
    }

    public void Add (Message message)
    {
        messages.Add (message);
        message.PropertyChanged += HandleMessagePropertyChanged;
    }

    public void Remove (Message message)
    {
        message.PropertyChanged -= HandleMessagePropertyChanged;
        messages.Remove (message);
    }

    public IList<Message> Messages ()
    {
        return messages.ToList ();
    }

    void HandleMessagePropertyChanged (object sender, PropertyChangedEventArgs e)
    {
        // Uptade omitted conversations properties using data from the updated message.
    }
}

public class Message : INotifyPropertyChanged
{
}

First of all, the Conversation class does not provide any events for chances, so nobody has a chance to get to know if something changes. This is already be implemented within an (Sorted)Observable collection, but unfortunately you publish this private field as IList<Message> by the Message method. So the notification abilities of this collection can not be used. Furthermore methods should always be connected with a action, so GetMessages() would be a more suitable name. But the best way is to make it a property.

using SoftConsept.Collections;

public class Conversation : INotifyPropertyChanged
{
    readonly SortedObservableCollection<Message> messages;

    public event PropertyChangedEventHandler PropertyChanged;

    public Conversation ()
    {
        messages = new SortedObservableCollection<Message> ();
    }

    private DateTime _updateTime = DateTime.Now;
    public DateTime UpdateTime{
       get{ return _updateTime;}
       private set{
           UpdateTime = value;
           OnPropertyChanged("UpdateTime");
       }
    }

    private void OnPropertyChanged(string propertyName){
       var handler = PropertyChanged;
       if(handler==null) return;
       handler(this, new PropertyChangedEventArgs(propertyName));
    }

    public void Add (Message message)
    {
        messages.Add (message);
        UpdateTime = DateTime.Now;
        message.PropertyChanged += OnMessagePropertyChanged;
    }

    private void OnMessagePropertyChanged(object sender, PropertyChangedEventArgs args)
    {
        UpdateTime = DateTime.Now;
    }

    public void Remove (Message message)
    {
        messages.Remove (message);
        UpdateTime = DateTime.Now;
        message.PropertyChanged -= OnMessagePropertyChanged;
    }

    public ObservableCollection<Message> Messages
    {
        get{
            //I assume that SortedObservableCollection is subtype of ObservableCollection
            return messages;
        }
    }
}

public class Message : INotifyPropertyChanged {}

So now some observer might register for changes in Messages (add, delete) and also for changes in the single Message instances.

For more info, look at Microsoft documentation:


Output in one window Can anyone give me feedback please? I used what I have learnt so far, mainly methods, loops and arrays. I would like you view based on these topics, however, comments on how to improve are welcome.

/*ask the user for 10 numbers
store them in an array
print them "your numbers are:"
print them "Average"
print them "Max"
print them "Min"*/

import javax.swing.*;

public class homeworkArrayWFunctionstest5
{
    public static void main(String[] args) throws Exception
    {
        String listOfNums = "";
        int sum = 0;
        int population = 0;
        int average = 0;
        int maxValue = 0;
        int minValue = 0;
        String [] numsStringInput = new String[10];
        population = numsStringInput.length;
        int [] arrayNumInt = new int[population];

        numsStringInput     = UserInputs(numsStringInput,population);
        arrayNumInt         = StringToInt(numsStringInput,population);
        listOfNums          = OneLineList(numsStringInput, population);
        sum                 = TotalCalc(arrayNumInt,population);
        average             = AvarageCalc(sum, population);
        maxValue            = MaxValueCalc(arrayNumInt,population);
        minValue            = MinValueCalc(arrayNumInt,population);

        Output(listOfNums,average,minValue,maxValue);
    }

    public static String [] UserInputs(String []numsStringInput,int population)
    {
        for (int i = 0; i<population; i++)
        {
            numsStringInput [i] = JOptionPane.showInputDialog(null,"Please Input number " + (i+1) + " of " + population + " numbers","Input Table",JOptionPane.QUESTION_MESSAGE);
        }

        return numsStringInput;
    }

    public static int [] StringToInt(String[] numsStringInput,int population)
    {
        int [] arrayNumInt = new int[population];

        for (int i = 0; i<population; i++)
        {
            arrayNumInt [i] = Integer.parseInt(numsStringInput [i]);
        }
        return arrayNumInt;
    }

    public static String OneLineList(String[] numsStringInput,int population)
    {
        String listOfNums = "";
        for (int i = 0; i<population; i++)
            {
                listOfNums = listOfNums + numsStringInput [i] + " ";
            }

        return listOfNums;
    }

    public static int TotalCalc(int []arrayNumInt,int population)
    {
        int total = 0;
        for(int i=0;i < population;i++)
        {
            total = total + arrayNumInt[i];
        }
        return total;
    }

    public static int AvarageCalc(int sum,int population)
    {
        int avarage = sum / population;
        return avarage;
    }

    public static int MaxValueCalc(int []arrayNumInt,int population)
    {
        int maxValue = arrayNumInt[0];
        for(int i=1;i < population;i++)
        {
            if(arrayNumInt[i] > maxValue)
            {
              maxValue = arrayNumInt[i];
            }
        }
        return maxValue;
    }

    public static int MinValueCalc(int []arrayNumInt,int population)
    {
        int minValue = arrayNumInt[0];
        for(int i=1;i < population;i++)
        {
            if(arrayNumInt[i] < minValue)
            {
              minValue = arrayNumInt[i];
            }
        }
        return minValue;
    }

    public static void Output(String listOfNums, int average, int minValue,int maxValue )
    {
        String text = "";

        String messageText[] = new String [] {"The number you have entered are:","Average is: ","Minimum number is: ","Maximum number is: "};
        int [] textNum = new int[] {0,average,minValue,maxValue};

        text = messageText[0]+ "\n" + listOfNums;

        for (int i = 1; i<messageText.length; i++)
        {
            text = text + "\n" + messageText[i] + textNum[i];
        }
        JOptionPane.showMessageDialog(null,text,"Output Table" ,JOptionPane.INFORMATION_MESSAGE);
    }
}

The Java standard dictates the use of the "Egyptian braces" style:

if (something) {
    // something...
}

In AvarageCalc():

int avarage = sum / population;
return avarage;

you can just return the calculation:

return sum / population;

Also, it should be spelled average. ;-)
In Output(), you don't need to declare text and then assign it later.  Instead, initialize it where it's first used.  Always try to keep variables as close in scope as possible, which can help keep your code cleaner and easier to maintain.

String text = messageText[0]+ "\n" + listOfNums;


Generically convert bytes to integers I wanted to know If there was a way to make it more versatile or shorter/simpler. Here's the code:
// Take only std::byte parameter and return an unsigned integral
constexpr auto bytes_to_uint(std::same_as<std::byte> auto... bytes) -> std::unsigned_integral auto {
    constexpr auto N = sizeof...(bytes);

    // Integral types large enough to hold N bytes
    using types = std::tuple<
        std::uint8_t,
        std::uint16_t,
        std::uint32_t,
        std::uint32_t,
        std::uint64_t,
        std::uint64_t,
        std::uint64_t,
        std::uint64_t
    >;

    using result = std::tuple_element_t<N, types>;

    return [&]<std::size_t... S>(std::index_sequence<S...>) {
        // Accumulate the part of the number using the bitwise or operator for each bytes
        return ((static_cast<result>(bytes) << CHAR_BIT * (N - S - 1)) | ... );
    }(std::make_index_sequence<N>{});
}

It is meant to be used like this:
bytes_to_uint(std::byte{0xaa}, std::byte{0xbb}); // std::uint16_t: 0xaabb

bytes_to_uint(
    std::byte{0x11},
    std::byte{0x22},
    std::byte{0x33},
    std::byte{0x44}
); // std::uint32_t: 0x11223344

The function signature strikes me as difficult to read, thanks to the constraint std::same_as<std::byte> auto... and the trailing "return type" std::unsigned_integral auto. I might rather write something like
constexpr auto bytes_to_uint(std::initializer_list<std::byte> bytes) {

...Ah, but then you couldn't use bytes.size() as a constant expression; I see.
So I'd think about writing an overload set, like this:
constexpr std::uint8_t bytes_to_uint(std::byte a) {
    return a;
}
constexpr std::uint16_t bytes_to_uint(std::byte a, std::byte b) {
    return (a << 8) | b;
}
constexpr std::uint32_t bytes_to_uint(std::byte a, std::byte b, std::byte c, std::byte d) {
    return (a << 24) | (b << 16) | (c << 8) | d;
}

But I guess this is messy because you need 16 different overloads. You can't even use default function arguments, because you want bytes_to_uint(a,b,c) to be equal to bytes_to_uint(0,a,b,c) and not bytes_to_uint(a,b,c,0). Of course you could still write
#define B std::byte
constexpr std::uint8_t bytes_to_uint(B a)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,a); }
constexpr std::uint16_t bytes_to_uint(B a, B b)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,0,0,a,b); }
constexpr std::uint32_t bytes_to_uint(B a, B b, B c)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,0,a,b,c); }
constexpr std::uint32_t bytes_to_uint(B a, B b, B c, B d)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,a,b,c,d); }
constexpr std::uint64_t bytes_to_uint(B a, B b, B c, B d, B e)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,a,b,c,d,e); }
[...22 more lines...]
#undef B

but I bet you don't want to do that. Okay, let's stick with the templatey thing you did.

using result = std::tuple_element_t<N, types>;

I'd rather see this dependent typedef use CamelCase (like a template parameter) or suffixedwith_type (like an STL member typedef). Calling it result makes it look too much like a variable, and makes it hard to pick out the one place where you use it.
Instead of spending 13 lines and a <tuple> dependency, I'd rather just do
using ResultType = std::conditional_t<
    (N == 1), std::uint8_t, std::conditional_t<
    (N == 2), std::uint16_t, std::conditional_t<
    (N <= 4), std::uint32_t, std::uint64_t>>>;

Which reminds me, you need something like
static_assert(N <= 16);

to stop you from trying to handle an argument list of 17 bytes or more.
And I didn't even notice until I tried it in Godbolt, but, you've got an off-by-one bug here! You meant tuple_element_t<N-1, types>. Remember that indexing always starts at zero (everywhere except <regex>).

If you don't like conditional_t, another option is to use plain old ifs. Factor out the computation while it's still parameterized on ResultType, and then use an if-else chain to decide on the right type to plug in for ResultType later:
auto do_it = [&]<class ResultType, std::size_t... S>(ResultType, std::index_sequence<S...>) {
    return ((static_cast<ResultType>(bytes) << CHAR_BIT * (N - S - 1)) | ... );
};
if constexpr (N == 1) {
    return do_it(std::uint8_t{}, std::make_index_sequence<N>{});
} else if constexpr (N == 2) {
    return do_it(std::uint16_t{}, std::make_index_sequence<N>{});
} else if constexpr (N <= 4) {
    return do_it(std::uint32_t{}, std::make_index_sequence<N>{});
} else if constexpr (N <= 8) {
    return do_it(std::uint64_t{}, std::make_index_sequence<N>{});
}

Even better, permit the compiler to do the math at its preferred bit-width (which is 64 bits on all the desktop platforms I care about) and then truncate it at the end. This produces similar codegen and reads nicer yet:
std::uint64_t result = [&]<std::size_t... S>( std::index_sequence<S...>) {
    return ((static_cast<std::uint64_t>(bytes) << CHAR_BIT * (N - S - 1)) | ... );
}(std::make_index_sequence<N>{});
if constexpr (N == 1) {
    return std::uint8_t(result);
} else if constexpr (N == 2) {
    return std::uint16_t(result);
} else if constexpr (N <= 4) {
    return std::uint32_t(result);
} else {
    return std::uint64_t(result);
}

You have another bug when N == 1 (besides the off-by-one bug). When N == 1, the fold-expression has no | operations at all, and so it's just a uint8_t shifted by zero. That shift expression has type int. Which is not an unsigned integral type. So your return-type-constraint fails!
This is just another reason to do all the math first in uint64_t, and then cast down to uint8_t right before you return, as shown in my last example above.
Writing any test cases would have caught both this bug and the off-by-one bug. Test cases are always important! Especially when you're planning to put the code up for public review. (Or for review by coworkers, for that matter.)

Finally, I recommend parentheses to clarify the precedence of x << CHAR_BIT * y. In context it's obvious what you expected the precedence to be; but as a reader, I'm not sure you're right. Put the parentheses so that I don't have to think about it even for a second.
However, in this context, that's a very minor point, because you clearly aren't expecting anybody to read the expression ((static_cast<result>(bytes) << CHAR_BIT * (N - S - 1)) | ... ). It's a "trust me" line of code.
It's also silly to pretend that CHAR_BIT is relevant here. This code explodes spectacularly if CHAR_BIT is anything other than 8. So just write 8; and if you are compelled to work in a reference to CHAR_BIT, do so by writing
static_assert(CHAR_BIT == 8);

at the top of the function.


Takes in two sorted lists and outputs a sorted list that is their union Write a function that takes in two sorted lists and outputs a sorted
  list that is their union

Could anyone help me to improve this code and make it more user-friendly? We are not supposed to use any built-in Python functions, but rather develop our own efficient algorithms.

def answer(list1, list2):

    len1 = len(list1)
    len2 = len(list2)

    final_list = []
    j = 0
    k = 0

    for i in range(len1+len2):

        if k == len1:
            final_list.append(list2[j])
            j += 1
            continue

        if j == len2:
            final_list.append(list1[k])
            k += 1
            continue

        if list1[k] < list2[j]:
            final_list.append(list1[k])
            k += 1

        else:
            final_list.append(list2[j])
            j += 1

    return final_list

print answer([1, 2 , 17, 18, 100], [3, 3, 4, 5, 15, 19, 20, 101])

I think your solution is ok.

Instead of using continue, consider using if..elif..elif..else:

if k >= len1:
  ...
elif j >= len2:
  ...
elif ...:
  ...
else:
  ...

Also, the test k >= len1 is an example of "defensive programming". It protects against k skipping over len1 when being incremented in the loop. Also, the expression list1[k] doesn't make sense unless k < len1, and the negation of that is k >= len1 which is another good reason to use it in the if statement.

If you've learned about xrange() then I would use that instead of range() -- see these SO answers:

https://stackoverflow.com/questions/135041/should-you-always-favor-xrange-over-range

Otherwise it looks good!


Hacker rank 30 days of code Maximum Sum of Hourglass I came up with this code as a solution to a contest question. The original question can be viewed here for attribution sake: Hackerrank 2d array problem

Sample input is like this:

1 1 1 0 0 0
0 1 0 0 0 0
1 1 1 0 0 0
0 0 2 4 4 0
0 0 0 2 0 0
0 0 1 2 4 0

Sample output is like this:

19

My actual code input from the submission website is as follows:

import math
import os
import random
import re
import sys

if __name__ == "__main__":
    arr = []

    for _ in range(6):
        arr.append(list(map(int, input().rstrip().split())))

total = 0
max_total = -1073741824

for i in range(len(arr)):
    for j in range(len(arr[i])):
        if (j+2 < 6) and (i+2 < 6):
            total = arr[i][j] + arr[i][j+1] + arr[i][j+2]+arr[i+1][j+1]+arr[i+2][j]+arr[i+2][j+1]+arr[i+2][j+2]
            if max_total < total:

                max_total = total
print(max_total)

There were several code listings on a different StackOverflow posting C++/Ruby solutions to 2d array problem. Being an absolute beginner in Python, I am still coming up to speed with advanced concepts in Python so my solution presented here is "un-Pythonic" and am sure not at all times optimized. How can I rewrite this code to be more "Pythonic"? I understand list comprehensions, but how can they be applied to multi-dimensional arrays like this case? Also, would rewriting the code to be Pythonic actually optimize the solution?

Reading the input data in a loop

arr = []
for _ in range(6):
    arr.append(list(map(int, input().rstrip().split())))

can be done with list comprehension, also rstrip() is not needed here because split() without arguments ignores trailing whitespace:

arr = [ list(map(int, input().split())) for _ in range(6)]

In

for i in range(len(arr)):
  for j in range(len(arr[i])):

I would assign the dimensions to separate variables, making the code
self-documenting:

num_rows = len(arr)
num_cols = len(arr[0])
for i in range(num_rows - 2):
    for j in range(num_cols - 2):

The initial value

max_total = -1073741824

looks arbitrary without an explaining comment. What about

max_total = -7 * 9 - 1  # An hourglass sum is at least 7 * (-9)

instead? And (even though @janos advised against it :) you can get rid
of both the total and the max_total variable if you use the
built-in max()
function on a generator of hourglass sums:

def hourglass_sums(arr):
    """Generate all sums of hourglasses in the array."""
    num_rows = len(arr)
    num_cols = len(arr[0])
    for i in range(num_rows - 2):
        for j in range(num_cols - 2):
            yield (arr[i][j] + arr[i][j + 1] + arr[i][j + 2] +
                   arr[i + 1][j + 1] +
                   arr[i + 2][j] + arr[i + 2][j + 1] + arr[i + 2][j + 2])

def max_hourglass_sum(arr):
    """Maximal hour glass sum in the array."""
    return max(hourglass_sums(arr))


JavaScript function that toggles element visibility according checked radio input This function has a set of radio inputs, for each radio there is a hidden text input. When a radio input is checked it shows its chained text input.

I've done it with a function constructor, and I could have done this with closures or with simpler invocation patterns. My idea is to have some robust function which I could extend.

I haven't tested browser compatibility, which is somehow important for my market (IE8 lovers).

jsFiddle

<form>
<input type="radio" name="color" value="yellow" checked />Yellow
<input type="radio" name="color" value="brown" />Brown
<input type="radio" name="color" value="orange" />Orange
<input type="radio" name="color" value="purple" />Purple

<input type="text" id="value-yellow" class="radio-condition" placeholder="Which yellow?" />
<input type="text" id="value-brown" class="radio-condition" placeholder="Which brown?" />
<input type="text" id="value-orange" class="radio-condition" placeholder="Which Orange?" />
<input type="text" id="value-purple" class="radio-condition" placeholder="Which Purple?" />

</form>

// INSTRUCTIONS
// There is a hidden text input for each radio input.
// Checked radio will show its chained text input.
// They're chained by the radio value and the text id
// the radio value makes a string that is used as the id of the text

// Constructor Function
var JavaScript_form = function() {
  this.radios = document.querySelectorAll('input[type="radio"]');
}

// Public method of instance
JavaScript_form.prototype.radio_hide_element = function() {

  // inputs that will be hidden
  var els_hidden = document.getElementsByClassName('radio-condition'),
      els_hidden_length = els_hidden.length;

  // loop each radio input
  [].forEach.call(this.radios, function (el) {
      var i = 0;
      for (; i < els_hidden_length; i++) {
        els_hidden[i].style.display = 'none';
      }
      els_hidden[0].style.display = 'block';

      // click radio will toggle visibility of text input
      el.addEventListener('click', function() {
        var i = 0;
        for (; i < els_hidden_length; i++) {
          els_hidden[i].style.display = 'none';
          els_hidden[i].value = '';
        }
        document.getElementById('value-' + this.value).style.display = 'block';
      });
  });
}

// Make instance
var JS_radios = new JavaScript_form();
JS_radios.radio_hide_element();

I'd use data attributes on the radio buttons to point to the IDs of the text inputs. Then you don't have to worry about constructing the ID strings in code. For IE8, use getAttribute()

I'd also use external CSS to default all the text fields to hidden -- to avoid that first nested loop in your JavaScript.

I'd then use a CSS class to toggle the visibility of the text fields (if the class is present, the field is visible, if the class is not present, the field is hidden). This makes it easier to locate the currently visible input (document.getElementsByClass), instead of having to iterate through all the text fields every time a radio button is clicked.

If you use data attributes and CSS classes, the event handler becomes pretty generic. You could use "event delegation" and attach the handler once to the parent element (avoiding that forEach loop -- which is not supported natively in IE8).

FYI, the <label> tag is significantly less usable without it either encompassing both the radio button and label text, or having its for attributed defined.


Finding an element in a list I'm kind of new to Python. So, I wonder which method is better to use in a function to find an element in a list.

First:

def search_binary(xs,target):
    count = 0
    for i in xs:
        if i == target:
            return count
            count = count +1
        else:
            count = count +1
            continue
    return -1

Second:

def search_binary(xs, target):
    lb = 0
    ub = len(xs)
    while True:
        if lb == ub:
           return -1

        mid_index = (lb + ub) // 2

        item_at_mid = xs[mid_index]

        if item_at_mid == target:
            return mid_index
        if item_at_mid < target:
            lb = mid_index + 1
        else:
            ub = mid_index

If your list is sorted you can use binary search (your second code), and it will be faster for large lists, but if it's unsorted you'll have to keep to linear search. The Python library provides fast implementation of both methods, so you really don't need to roll your own:

>>> a = range(10)

>>> a.index(4) # linear search
4

>>> import bisect
>>> idx = bisect.bisect(a, 4) # binary search
>>> if a[idx-1] == 4:
...     print idx - 1
... else:
...     raise ValueError('item not in list')
...
4


RenderBatch for game engine Recently, I implemented an RenderBatch class, which renders any objects, using a specific shader (with its configuration). I'd like to improve on the way my render process works.

RenderBatch.h:

#pragma once
#include "alpha/Shader.h"
#include "alpha/RenderComponent.h"
#include <glm/glm.hpp>
#include "alpha/Mesh.h"
#include "alpha/Transform.h"
#include "alpha/Model.h"

class ShaderConfiguration
{
public:
    ShaderConfiguration();
    virtual ~ShaderConfiguration();
    virtual void Update(Shader& shader);
    void SetProjectionView(const glm::mat4& projectionView){
        m_projectionView = projectionView;
    }
    glm::mat4 GetProjectionView() const{
        return m_projectionView;
    }
    void SetTransform(const Transform& transform){
            m_transform = transform;
    }
    Transform GetTransform(){
        return m_transform;
    }
protected:
    glm::mat4 m_projectionView;
    Transform m_transform;
};
typedef ShaderConfiguration DefaultShaderConfiguration;

///Render Batch is really low lovel... It is only advised to use it
///in case you want to experiment with GLSL shaders : for more basic uses,
///ModelBatch is actually integrated in the GameObject system...
class RenderBatch : RenderComponent
{
public:
    RenderBatch(){
        m_shader.loadFromFile(GL_VERTEX_SHADER, "res/basicShader.glslv");
        m_shader.loadFromFile(GL_FRAGMENT_SHADER, "res/basicShader.glslf");
        m_shader.CreateAndLink();
    }
    RenderBatch(Shader shader)
        : m_shader(shader){}
    virtual ~RenderBatch(){}
    void SetShader(Shader shader){this->m_shader = shader;}
    Shader GetShader()const {return m_shader;}

    ///Genereal way of updating -----------------> using polymorphism
    void UpdateShaderConfiguration(ShaderConfiguration* config){
        m_shader.Bind();
            config->Update(m_shader);
        m_shader.UnBind();
    }
    ///Update for simple rendering --------------> ProjectionView + Transform ()
    ///Equivalent of :
    ///DefaultShaderConfiguration defaultShaderConfiguration
    ///UpdateShaderConfiguration(&defaultShaderConfiguration);
    void Update(glm::mat4 projectionView, Transform transform){
        m_shader.Bind();
            glUniformMatrix4fv(
                m_shader.GetUniformLocation("MVP"),
                1,
                GL_FALSE,
                glm::value_ptr(projectionView * transform.GetModelView())
                );
        m_shader.UnBind();
    }
    void Render(Mesh& mesh){
        m_shader.Bind();
            mesh.Draw();
        m_shader.UnBind();
    }
    glm::mat4 GetProjectionViewMatrix(){
        return m_projectionView;
    }
    void SetProjectionViewMatrix(glm::mat4 projectionView){
        this->m_projectionView = projectionView;
    }
protected:
    Shader m_shader;
    glm::mat4 m_projectionView;
};

RenderBatch.cpp

#include "alpha/RenderBatch.h"

ShaderConfiguration::ShaderConfiguration()
    : m_projectionView(glm::mat4(1))
{}
ShaderConfiguration::~ShaderConfiguration(){}

void DefaultShaderConfiguration::Update(Shader& shader){
    glUniformMatrix4fv(
    shader.GetUniformLocation("MVP"),
    1,
    GL_FALSE,
    glm::value_ptr(m_projectionView * m_transform.GetModelView())
    );
}

Main.cpp

 ///example 1 ---->using defaults
 RenderBatch batch = RenderBatch();
 Mesh mesh("res/Test.obj");
 Camera camera;

 while(isRunning)
 {
     /*Update */
     batch.Update(camera.Combined(), transform)
     /*some more update code...*/

     /*Render*/
     batch.Render(mesh);
 }

 ///example 2 ---->using custom configuration
 RenderBatch batch = RenderBatch();
 Mesh mesh("res/Test.obj");
 Camera camera;
 ShaderConfiguration config;

 while(isRunning)
 {
     config.SetProjectionView(camera.combined());
     config.SetTransform(transform)
     /*Update */
     batch.Update(config);
     /*some more update code...*/

     /*Render*/
     batch.Render(mesh);
 }

There is a bit of misnomer with RenderBatch. The term "batch rendering" is normally associated with grouping several geometries that share the same rendering attributes (textures/materials/etc) to render then together and avoid expensive state changes. Your class is just a wrapper around a Shader, so I don't see much reason for its existence, when all that could be part of Shader instead.

There is also an excessive amount of shader binding/unbinding going on inside RenderBatch. You definitely don't want to do that, changing shader programs is a very expensive operation.

That's one of the pitfalls of attempting to integrate OpenGL with OOP. The library was just not made to be used like that; it relies heavily on global state. This will change in the future with the new "bindless" OpenGL, but that's for version 4.5 and above.

Another potential issue that I see here is that RenderBatch is taking its Shader parameter by value, thus making a copy of it. How are you dealing with the internal GL shader program handle in this case? Even though you can copy the integer handle given to you by OpenGL, you can't copy the underlaying shader object itself. So I'm wondering if you are not winding up with duplicate shader handles through your program that will get deleted multiple times by calls to glDeleteProgram or even leak. Yet another problem of mixing a very C-ish API with C++. You have to be extra careful to avoid these sorts of mistakes. Be very aware that you are mixing two distinct programming languages in the same project.


Demonstrating the speed advantage of parallel computation I am teaching a class on parallel computation and am looking for a simple example that illustrate the speed gains from using "CompletableFuture"s in Java. Currently my example problem is very simple: calculate the sum of an array. Each CompletableFuture calculates the sum of about 1/4 of the array, and when all of them finish, the main program calculates the total sum.

The line sum += Math.pow(Math.pow(Math.pow(array[i],0.3),0.4),0.5); was initially just sum += array[i]; I added all the "pow"s just to make the serial computation appear longer.

Currently, on my machine with 4 cores, when I put NUM_OF_THREADS = 1 the sum is calculated in 4 seconds, while with NUM_OF_THREADS = 4 the sum is calculated in 5.5 seconds, so this is not a good example - it does not show that parallel computation is more efficient.

what should I change in the example, to demonstrate the advantage of parallel computation?

public class ConcurrencyTest {

    static final int SIZE = 10000000;
    static final int NUM_OF_THREADS = 1;

    static int iThread;
    @SuppressWarnings("unchecked")
    public static void main(String[] args) throws InterruptedException, ExecutionException {
        int[] array = new int[NUM_OF_THREADS*SIZE];
        Arrays.fill(array, 1);

        ExecutorService executor = Executors.newCachedThreadPool();
        Instant startTime = Instant.now();

        CompletableFuture<Integer>[] futures = new CompletableFuture[NUM_OF_THREADS];
        for (iThread=0; iThread<NUM_OF_THREADS; ++iThread) {
            final int iThreadLocal = iThread;
            futures[iThread] = CompletableFuture.supplyAsync( () -> {
                int sum=0;
                int start = iThreadLocal*SIZE;
                int end = (iThreadLocal+1)*SIZE;
                for (int i=start; i<end; ++i) {
                    Thread.yield();
                    sum += Math.pow(Math.pow(Math.pow(array[i],0.3),0.4),0.5);
                }
                System.out.println(iThreadLocal+": "+sum +
                        "   "+Duration.between(startTime, Instant.now()).toMillis()+" [ms]");
                return sum;
            }, executor);
        }

        CompletableFuture.allOf(futures).thenRun(
                () -> {
                    try {
                        Arrays.stream(new int[] {1,2,3}).sum();
                        int totalSum = 0;
                        for (iThread=0; iThread<NUM_OF_THREADS; ++iThread)
                            totalSum += futures[iThread].get();
                        System.out.println("Total: "+totalSum+
                                "   "+Duration.between(startTime, Instant.now()).toMillis()+" [ms]");
                    } catch (InterruptedException | ExecutionException e) {
                        e.printStackTrace();
                    }
                }
        );

        executor.shutdown();
    }
}

Currently each future processes SIZE elements (increasing the NUM_OF_THREADS increases the total workload (NUM_OF_THREADS * SIZE)). Instead you want to divide the workload between each of the futures:

int start = SIZE / NUM_OF_THREADS * iThreadLocal;
int end = SIZE / NUM_OF_THREADS * (iThreadLocal + 1);

Besides that iThread could (and should) be a local variable. The spacing between operators is inconsistent, you should add whitespace around the operators (except for unary operators) to improve readability.


Array is MinMax Equal or Not An array is defined to be maxmin equal if it contains at least two different elements and the number of times the maximum value occur is the same as the number of times the minimum value occur. So {11, 4, 9, 11, 8, 5 , 4, 10} is maxmin equal, because the max value 11 and min value 4 both appear two times in the array.

if the input array is {} , isMaxMinEqual should return false (array must have at least two different elements) status = false;
if the input array is {2}, isMaxMinEqual should return false (array must have at least two different elements).
if the input array is {1,1,1,1,1,1}, isMaxMinEqual should return false (array must have at least two different elements).
if the input array is {2,4,6,8,11}, isMaxMinEqual should return true (Both max value(11) and min value 2 appear exactly one time).
if the input array is {-2,-4,-6,-8,-11}, isMaxMinEqual should return true(Both max value(-2) and min value -11 appear exaclty one time).

I wrote the class MaxMin which satisfied the above condition.

public class MaxMin {

public static void main(String args[]) {
    System.out.println("The result is: " + isMaxMinEqual(new int[]{2, 2}));
}

public static boolean isMaxMinEqual(int[] a) {
    boolean status = false;
    int count = 0;
    if (a.length < 2) {
        status = false;
        return status;
    }
    for (int i = 1; i < a.length; i++) {
        if (a[i - 1] != a[i]) {
            count++;
        }
    }

    int max = a[0];
    int min = a[0];
    for (int i = 0; i < a.length; i++) {
        if (a[i] > max) {
            max = a[i];
        } else if (a[i] < min) {
            min = a[i];
        }
    }

    int maxCount = 0;
    for (int j = 0; j < a.length; j++) {
        if (a[j] == max) {
            maxCount++;
        }
    }

    int minCount = 0;
    for (int j = 0; j < a.length; j++) {
        if (a[j] == min) {
            minCount++;
        }
    }
    System.out.println(count);
    System.out.println("max is: " + max);
    System.out.println("min is: " + min);
    System.out.println("Max Count is: " + maxCount);
    System.out.println("Min Count is: " + minCount);

    if (maxCount == minCount && count >= 1) {
        status = true;
    }

    return status;
}
}

Varags

Since Java 1.5, you can use varargs in your method so that you can pass in a sequence of elements, instead of an explicit array:

public static boolean isMaxMinEqual(int... values) {
    // use values like a normal int[] array
}

Variables assignment

You have declared status right at the start, but it's actually redundant as you can simply return from the method directly in the places where you use them:

if (a.length < 2) {
    // status = false;
    // return status;
    return false; // shorter and clearer
}
// ...
/*
if (maxCount == minCount && count >= 1) {
    status = true;
}
return status;
*/
return maxCount == minCount && count >= 1; // no need to set status = true

'Debug' statements

System.out.println(count);
System.out.println("max is: " + max);
System.out.println("min is: " + min);
System.out.println("Max Count is: " + maxCount);
System.out.println("Min Count is: " + minCount);

These look like 'debug' statements where the developer (or user) is manually checking that the counting is done correctly. Usually, this should be done through a logging framework (so that the verbosity can be adjusted), or be eliminated entirely. It does not serve a direct purpose as to the method's usage.

Iterations

Your approach has to iterate through the array three times:

Find the min/max.
Count the max.
Count the min.

In fact, these can be done in one pass:

int min = Integer.MAX_VALUE;
int max = Integer.MIN_VALUE;
int minCounter = 0;
int maxCounter = 0;
for (int i : values) {
    if (i < min) {
        min = i;
        minCounter = 1;
    } else if (i == min) {
        minCounter++;
    }
    if (i > max) {
        max = i;
        maxCounter = 1;
    } else if (i == max) {
        maxCounter++;
    }
}
return minCounter == maxCounter && min != max;

The idea here is that when you need to reset your min/max values, you can just reset your counters to 1 and proceed, instead of having to iterate again.

If you do not mind extra ternary operators, you can sort of combine both if branches per min/max too:

int min = Integer.MAX_VALUE;
int max = Integer.MIN_VALUE;
int minCounter = 0;
int maxCounter = 0;
for (int i : values) {
    if (i <= min) {
        minCounter = 1 + (i == min ? minCounter : 0);
        min = i;
    }
    if (i >= max) {
        maxCounter = 1 + (i == max ? maxCounter : 0);
        max = i;
    }
}
return minCounter == maxCounter && min != max;


Finding consecutive occurrences of a value in an array Given an array, return True if the array contains consecutive values:

has22([1, 2, 2]) - True
has22([1, 2, 1, 2]) -False
has22([2, 1, 2]) - False

I do aware of a quick solution by iterating the list in a for loop and comparing current and next items for equality until it reaches the end, also using modules like itertools as pointed out by @syb0rg. However, I am asking this to learn algorithmic approach

Current Solution:

def has22(nums):
    total_occurrences = nums.count(2)

    if total_occurrences >= 2:

        forward_start = 0
        backward_start = 0
        reversed_nums = nums[::-1]
        last_item_index = len(nums) - 1

        for _ in xrange(total_occurrences/2 ):

            next_forward_occurrence = nums.index(2,forward_start)
            next_backward_occurrence=  last_item_index - reversed_nums.index(2, backward_start)

            if nums[next_forward_occurrence] == nums[next_forward_occurrence+1]:
                return True
            elif nums[next_backward_occurrence] == nums[next_backward_occurrence - 1]:
                return True

            forward_start = next_forward_occurrence
            backward_start = next_backward_occurrence

    return False

I would like to know if there is any other efficient algorithm (using only built-in methods, no modules/lib please)

First off your code doesn't work as you want it too.
>>> has22([2, 1, 2, 2, 1, 2, 1])
False
>>> has22([2, 1, 2, 1, 2, 1, 2])
True

Your code also doesn't follow PEP8, and the name of your variables are, long, really long.
This code, as you've written it, intends to look forwards and backwards, consequently, this causes the code to use more memory than needed. It also adds to the complexity of the code, thus making it harder to read.
Instead if you make the code travel only one way, and change forward_start to prev_index and next_forward_occurrence to index, you can obtain:
def has22(nums):
    total_occurrences = nums.count(2)
    if total_occurrences >= 2:
        prev_index = 0
        for _ in xrange(total_occurrences - 1):
            index = nums.index(2, prev_index)
            if nums[index] == nums[index + 1]:
                return True
            prev_index = index + 1
    return False

I don't like the use of nums.count(2) as it causes the algorithm to become \$O(2n)\$.
Instead I'd break upon index becoming -1.
def has22(nums):
    prev_index = 0
    while True:
        index = nums.index(2, prev_index)
        if index == -1:
            break
        if nums[index] == nums[index + 1]:
            return True
        prev_index = index + 1
    return False

But both of these are still, harder to read compared to the algorithm that you stated in the question.

I do aware of a quick solution by iterating the list in a for loop and comparing current and next items for equality until it reaches the end

Here is the algorithm you're talking about:
def has22(nums):
    nums = iter(nums)
    prev = next(nums, StopIteration)
    if prev is not StopIteration:
        for num in nums:
            if num == prev and num == 2:
                return True
            prev = num
    return False

Where a simpler approach would use any and zip, and a smarter approach would also use itertools.
def has22(nums):
    return any(num1 == num2 and num1 == 2 for num1, num2 in zip(nums, nums[1:]))

from itertools import tee, izip

def has22(nums):
    a, b = tee(nums)
    next(b, None)
    return any(num1 == num2 and num1 == 2 for num1, num2 in izip(a, b))


Laravel - lots of dependency injection I have the following UserServiceProvider and I'm wondering whether so much dependency injection is overkill:

use
Illuminate\Support\ServiceProvider,
App\Repositories\Eloquent\User\EloquentUserRepository,
App\Repositories\Eloquent\Profile\EloquentProfileRepository,
App\Repositories\Eloquent\Seller\EloquentSellerRepository,
App\Repositories\Eloquent\AppResponse\EloquentAppResponseRepository,
App\Repositories\Eloquent\AppRequest\EloquentAppRequestRepository,
App\Repositories\Eloquent\Model\EloquentModelRepository,
App\Repositories\Eloquent\Maker\EloquentMakerRepository,
Cartalyst\Sentry\Sentry,
App\Services\Cache\Decorators\UserCacheDecorator,
App\Services\Cache\Laravel\LaravelCache,
App\Entities\Entities\Profile\ProfileEntity,
App\Entities\Entities\Seller\SellerEntity,
App\Entities\Entities\AppRequest\AppRequestEntity,
App\Entities\Entities\AppResponse\AppResponseEntity,
App\Entities\Entities\Model\ModelEntity,
App\Entities\Entities\Maker\MakerEntity,
Profile,
Seller,
AppResponse,
AppRequest,
Model,
Maker;

class UserServiceProvider extends ServiceProvider
{
    public function register()
    {
        $this->app->bind('App\Repositories\Interfaces\UserRepositoryInterface', function($app)
        {
            $user = new EloquentUserRepository(
                new Sentry(),
                new ProfileEntity(
                    new EloquentProfileRepository(
                        new Profile()
                        )
                    ),
                new SellerEntity(
                    new EloquentSellerRepository(
                        new Seller()
                        )
                    ),
                new AppRequestEntity(
                    new EloquentAppRequestRepository(
                        new AppRequest()
                        )
                    ),
                new AppResponseEntity(
                    new EloquentAppResponseRepository(
                        new AppResponse()
                        )
                    ),
                new ModelEntity(
                    new EloquentModelRepository(
                        new Model()
                        )
                    ),
                new MakerEntity(
                    new EloquentMakerRepository(
                        new Maker()
                        )
                    )
                );
            return new UserCacheDecorator(
                $user,
                new LaravelCache($app['cache'], 'user')
                );
        });
    }
}

I do need all these entities in my UserEntity. Is this normal or should I start reducing the number of injections?

You should reconsider the responsibilities of class UserServiceProvider, too many responsibilities may lead to a lot of dependencies in many cases.
It is quite possible that this class is doing a lot of work that it should not, for example populating User object with sales reports, earnings reports, or activating user, checking activation status etc.

It is not clear in your question but looking at the names of dependencies it looks like it does a lot of checking and population, you should consider moving respective codes to respective repositories and also refactor the service object into separate classes. This will create a spreaded hierarchy like this:

                           DashboardController
                           /                \
            UserProfileService              UserService
                /        \                    /         \
UserProfileRepository  OtherRepository  UserRepository   SomeOtherDependency

Now, instead of injecting UserProfileRepository, OtherRepository, UserRepository, SomeOtherDependency into UserService and then injecting UserService into controller you can inject UserProfileService and UserService into controller with all of their respective dependencies.

A couple of them looks like they can be defined as a relation. You can, for example, define UserProfile as a relation on User which in a typical case should be a one-to-one relationship. This should sort out the dependencies quite a bit.

Main point to take home is that you should re-evaluate the responsibilities of classes and determine which object should be responsible for what action. as a basic rule of thumb, all data fetching and storing should be a responsibility of repository and all data manipulation and logical decisions should be a responsibility of a service object. If a service object requires more than one repository as a dependency then step back and check if the manipulation of data from second repo should really be a responsibility of service object ?


Print dict with varying-length lists Is there a shorter way of taking a dict with varying length lists and printing it to screen? Shortcomings of this method also include that values which aren't lists are printed as letters separated by spaces, and values which are numbers produce an error.  Would it be better to write a class which could accept any type of dict value, or is it best to have different classes for different types of dict?

class PrintDict(object):
    """prints dict keys and values including lists
    except if values are numbers"""

    def print_dict_to_screen(self, data):

        self.data = data
        print ""
        for key in sorted(self.data):
            print "\n\n%s" % str(key)
            items = len(self.data[key])
            iterate = -1
            for i in range(items):
                iterate += 1
                print self.data[key][iterate],

crops = {'trees' : ['hazelnut'],
'potatoes': ['early', 'main'],
'legumes': ['broad beans', 'peas', 'runner beans'],
'brassicas': ['cabbages', 'sprouts', 'kale', 'swede', 'turnip'],
'onions and roots': ['carrots',   'parsnips',
'beetroot', 'onions', 'shallots'],
'other': ['lettuce', 'sweet corn', 'flowers'],
'fruit': ['blackberry', 'blackcurrant', 'raspberry']}

print_ = PrintDict()

print_.print_dict_to_screen(crops)

I don't see any reason why the PrintDict class should exist.  Furthermore, there's no point in keeping self.data as part of the object state.  A standalone function print_dict_to_screen(data) would work just fine.

Much of the code is concerned with printing out the value lists in the right format.      This whole chunk of code…

items = len(self.data[key])
iterate = -1
for i in range(items):
    iterate += 1
    print self.data[key][iterate],

… could be replaced with

print ' '.join(self.data[key])

It's usually more manageable if you consistently put your newlines at the end rather than at the beginning of your output.

There's no need to cast the key as a str, since the %s format specification always interprets its input as a string.

So, the code can be simplified to:

def print_dict_to_screen(self, data):
    for key in sorted(data):
        print "%s\n%s\n" % (key, ' '.join(data[key]))

It would be even cleaner if the output had no excessive leading or trailing newlines.  To accomplish that, I would use join() again:

def dict_to_string(data):
    return "\n\n".join(
        "%s\n%s" % (key, ' '.join(data[key])) for key in sorted(data)
    )

Here, dict_to_string() returns the string to be printed rather than printing it directly.  Then, the caller is free to print the resulting string, log it to a file, or whatever.


Angular Models as Property or Injectable? I have an application that looks like such:

var app = angular.module('app', []);

app.Person = function(firstName, lastName) {
    this.firstName = firstName;
    this.lastName = lastName;
}

app.controller = function('myController', function($scope) {

    $scope.pageData = [];
    $scope.pageData.firstName = ['John'];
    $scope.pageData.lastName = ['Smith'];

    $scope.pageData.people = [];

    // just a demo. The point of this is just to show the controller creating Person objects
    // assume that first name and last name will always be the same length;
    for (var i = 0; i < firstName.length; i++) {
        $scope.pageData.people.push(new Person($scope.pageData.firstName[i], $scope.pageData.lastName[i]);
    }
});

Is there a better way of structuring Person? Should it be in a factory or service?

Because Javascript allows you to easily create objects without an explicit class, I usually find it unnecessary to define a class that doesn't have any functions.  That is, there's no benefit to using new Person(x,y) over {firstName: x, lastName: y}.  In fact, I would back up a step and have that kind of data in my model instead of separate arrays of first and last names:

$scope.pageData = [{firstName: 'John', lastName: 'Smith'} /* etc */];


Authentication using PDO I'm new to PDO, so I was wondering if you guys could check over my PDO code for efficiency (and if the PHP can be improved)

public function ValidateUser($username = '', $password = '')
{
    $prefix = "SELECT ";
    $type = 'COUNT(id)';
    $suffix = " FROM users WHERE username = :username LIMIT 1";
    if($grabUser = $this->db->prepare($prefix.$type.$suffix))
    {
        $grabUser->bindParam(":username", $username, PDO::PARAM_STR);
        $grabUser->execute();
        if(count($grabUser->fetchColumn()) <= 0)
        {
            return array(0,0);
        }
    }
    else {
        return array(0,0);
    }

    $type = 'salt';
    if($grabUser = $this->db->prepare($prefix.$type.$suffix))
    {
        $grabUser->bindParam(":username", $username, PDO::PARAM_STR);
        $grabUser->execute();
        $salt = $grabUser->fetch()['salt'];
        $password = $this->core->blueHash($password, $salt);
    }
    else {
        return array(0,0);
    }

    $stmt = "SELECT COUNT(id) FROM users WHERE username = :username AND password = :password LIMIT 1";
    if($checkFinal = $this->db->prepare($stmt))
    {
        $checkFinal->bindParam(":username", $username, PDO::PARAM_STR);
        $checkFinal->bindParam(":password", $password, PDO::PARAM_STR);
        $checkFinal->execute();
        if(!count($checkFinal->fetchColumn()) > 0)
        {
            return array(0,0);
        }
        return array(count($checkFinal->fetchColumn()),$password);
    }
    return array(0,0);
}

This code checks that the supplied username and password are correct and if so, an array is returned.

EDIT: AJAX file where ValidateUser() is used:

<?php
require_once "../../_global.php";
$core->ajaxCheck();

header("Content-Type: application/json");
$json = array();

if(USER_ID <= 0 && !LOGGED_IN)
{
    // DO ISSET CHECK :((
    $username = $core->input($_POST['name']);
    $password = $core->input($_POST['pword']);

    $valid = $users->ValidateUser($username, $password);
    if($valid[0] == 0)
    {
        $json[0] = "error";
        $json[1] = "ajax";
        $json[2] = "These details are incorrect.";
    }
    elseif($valid[0] > 0)
    {
        $_SESSION['login']['username'] = $users->GrabUserVar($users->Name2id($username),"username");
        $_SESSION['login']['hash'] = $valid[1];
        $_SESSION['login']['social'] = false;
        $json[0] = "success";
    }
}
else {
    $json[0] = "error";
    $json[1] = "ajax";
    $json[2] = "You are already signed in.";
}
echo json_encode($json);
exit;
?>

EDIT2: Code that is run on each page to check whether the session username and password hash match that of the database:

session_start();
if(isset($_SESSION['login']['username']) && isset($_SESSION['login']['hash']))
{
    $userN = $_SESSION['login']['username'];
    $userH = $_SESSION['login']['hash'];
    if($users->CheckUser($userN, $userH) > 0)
    {
        define('LOGGED_IN',true);
        define('USER_ID',$users->Name2id($userN));
        $cUser = $users->createIndividual(USER_ID);
        define('USER_NAME',$cUser->getUsername());
        define('USER_HASH',$userH);
    }
    else {
        define('LOGGED_IN',false);
        define('USER_ID',0);
        define('USER_NAME','Guest');
        define('USER_HASH',0);
        session_destroy();
    }
}
else {
    define('LOGGED_IN',false);
    define('USER_ID',0);
    define('USER_NAME','Guest');
    define('USER_HASH',0);
}

Efficiency

You currently perform three queries:

check if a user with that name exists
get salt for that username
check if user with that username and password exist

You can perform all these actions with one query:

SELECT salt, password FROM users WHERE username = :username LIMIT 1;

Then just hash the supplied password with the salt from the db, and compare the result to the password from the db. It might look something like this:

if (hash_equals($grabUser->fetch()['password'], $this->core->blueHash($password, $grabUser->fetch()['salt'])) {
    echo 'valid password';
}

Return Early

If you negate your if clauses, you can safe one level of nesting, which I think results in more readable code, because nesting decreases readability, and because it's clearer to what if clause the else clause belongs. Eg:

if(!($grabUser = $this->db->prepare($prefix.$type.$suffix)))
{
    return array(0,0);
}

$grabUser->bindParam(":username", $username, PDO::PARAM_STR);
$grabUser->execute();
if(count($grabUser->fetchColumn()) <= 0)
{
    return array(0,0);
}

Misc

it's unclear why you are returning an array. It seems to always contain [1, $userPassword]. Why do you need to return the password? And wouldn't it be easier to just return true or false instead of 1 or 0 inside an array? At least I would add a PHPDoc style comment explaining what is returned.
I would extract some of the code to its own function to increase readability and avoid duplication. eg you could have fetchFromUser($columnName, $username).
I don't know what password hashing you currently use, but I would think about switching to bcrypt. It's easier to use (you don't have to manage salts, etc), and is currently considered secure.


Reversing a number in Ruby I'm writing a function to reverse a number in Ruby. IE, 314159.reverse should return 951413.

Here's what I have, which works:

class Numeric
  def reverse
    str = self.to_s.reverse

    if self.is_a?(Float)
      return str.to_f
    elsif self.is_a?(Integer)
      return str.to_i
    end
  end
end

I don't like the enumeration of different number types. Is there a way to dynamically pass in the type?

Something like:

return str.to_type(self.type)

Any other thoughts on simplifying this?

See also: My related Stack Overflow question on some other approaches I tried.

I am going to refactor this in small steps so that you can follow my thought process and see where you agree and you disagree.

First, let's start with some style. In Ruby, self is the implied receiver of a message send if you don't supply one, it is generally not necessary to state it explicitly:

class Numeric
  def reverse
    str = to_s.reverse

    if is_a?(Float)
      return str.to_f
    elsif is_a?(Integer)
      return str.to_i
    end
  end
end

Conditionals are expressions in Ruby, not statements. In fact, everything is an expression in Ruby. So, conditionals evaluate to a value, which means that instead of returning a value from each of the branches, we can return the value of the entire conditional expression instead:

class Numeric
  def reverse
    str = to_s.reverse

    return if is_a?(Float)
      str.to_f
    elsif is_a?(Integer)
      str.to_i
    end
  end
end

Being an expression-oriented language, the value of a method body (and also a block body, module body, class body, any block of code, really) is the value of the last expression evaluated inside that body. There is no need to explicitly return (or next in case of a block) it:

class Numeric
  def reverse
    str = to_s.reverse

    if is_a?(Float)
      str.to_f
    elsif is_a?(Integer)
      str.to_i
    end
  end
end

For multi-way conditionals like this, it is usually more legible to use a case expression, instead of a series of if and elsifs:

class Numeric
  def reverse
    str = to_s.reverse

    case
    when is_a?(Float)
      str.to_f
    when is_a?(Integer)
      str.to_i
    end
  end
end

Now we can start doing some real interesting changes. Module#=== checks whether its argument is an instance of self. Object#is_a? checks whether self is an instance of its argument. IOW: the two methods are mirror images of each other, you can usually exchange o.is_a?(m) with m === o and vice-versa. This allows us to simplify our case expression even more:

class Numeric
  def reverse
    str = to_s.reverse

    case self
    when Float
      str.to_f
    when Integer
      str.to_i
    end
  end
end

Now, on to the big thing!

In an object-oriented language, it is always possible to replace a conditional with runtime polymorphic message dispatch. Message dispatch is more powerful than conditionals. Smalltalk is the existence proof of that, it doesn't even have conditionals built into the language, its conditionals are implemented in the library, using message dispatch, kind of like this:

class TrueClass
  def if_then_else(then_block, else_block)
    then_block.()
  end
end

class FalseClass
  def if_then_else(then_block, else_block)
    else_block.()
  end
end

(2 > 3).if_then_else(-> { puts 'True' }, -> { puts 'False' })
# False

This gives rise to the Replace Conditional with Polymorphism Refactoring. (Here is an example of it in Ruby.)

In your case, you are not just switching behavior based on some abstract notion of "type" of the object, you are literally switching behavior based on the class. Even worse: you are switching based on the class of self.

That is a big honking red flag: self always knows what class it is. It should never ever have to check for its own class!

What you are basically doing is re-implementing message dispatch:

foo.bar

will run different code, depending on the class of foo. That's already built into Ruby, you don't have to re-implement it yourself.

So, we let Ruby take care of making the decision whether to invoke the Float or the Integer version of our code:

class Integer
  def reverse
    str = to_s.reverse

    str.to_i
  end
end

class Float
  def reverse
    str = to_s.reverse

    str.to_f
  end
end

Okay. So, the last bit we have here is a little bit of code duplication, which we can get rid of by using the Extract Method Refactoring followed by the Pull Up Method Refactoring.

Extract:

class Integer
  def reverse
    reverse_to_s.to_i
  end

  private def reverse_to_s
    to_s.reverse
  end
end

class Float
  def reverse
    reverse_to_s.to_f
  end

  private def reverse_to_s
    to_s.reverse
  end
end

Pull Up:

class Numeric
  private def reverse_to_s
    to_s.reverse
  end
end

class Integer
  def reverse
    reverse_to_s.to_i
  end
end

class Float
  def reverse
    reverse_to_s.to_f
  end
end

Now we're done. Our code is nice and clean, all our methods do exactly one thing, there are no IFs or other conditionals.

The only slightly smelly bit left is the monkey-patching we are doing. Refinements to the rescue!

module ReversibleNumeric
  refine Numeric do
    private def reverse_to_s
      to_s.reverse
    end
  end

  refine Integer do
    def reverse
      reverse_to_s.to_i
    end
  end

  refine Float do
    def reverse
      reverse_to_s.to_f
    end
  end
end

314159.reverse
# NoMethodError

using ReversibleNumeric

314159.reverse
# => 951413

[Well, actually, Refinements are still in a state of flux, and the above code may or may not work, depending on your exact version and implementation of Ruby.]


A matrix-vector multiply parallelized using boost::MPI This program performs a matrix-vector multiply using MPI to split the computation up. It is not extremely robust (for example, it doesn't handle the case where the number of MPI processes does not divide evenly into the dimensions of the matrix), but I mainly wanted to get some practice using MPI. Is my use of the functions correct, and what are some ways I can improve this code?

#include <iostream>
#include <string>
#include <boost/mpi.hpp>
#include <numeric>

void matVecMult( double ** mat, double * vec, double * result, int beg, int end, unsigned N) {
    for (auto i = beg; i < end; i++) {
        result[i] = std::inner_product(mat[i], mat[i] + N, vec, 0.0);
    }
}

int main(int argc, char ** argv) {
    boost::mpi::environment env;
    boost::mpi::communicator world;

    int dim = (argc > 1) ? std::stoi(argv[1]) : 4;
    auto N = static_cast<unsigned>(dim);

    auto **matrix = new double *[N]();
    auto  *vector = new double[N]();
    auto  *result = new double[N]();

    for(unsigned i = 0; i < N; i++){
        matrix[i] = new double[N]();
        for(unsigned j = 0; j < N; j++){
            if(i==j) {matrix[i][j] = i+j;}
        }
        vector[i] = i + i;
    }

    int beg = world.rank() * (N/world.size());
    int end = beg + (N/world.size());

    matVecMult(matrix, vector, result, beg, end, N);

    world.isend(0, world.rank(), result + beg, (N/world.size()));
    if(world.rank() == 0){
        for(int i = 0; i < world.size(); i++) {
            world.recv(i,i, result+(i*N/world.size()), (N/world.size()));
        }
    }

    for(unsigned i = 0; i < N; i++){
        delete[] matrix[i];
    }

    delete[] matrix;
    delete[] vector;
    delete[] result;

    return 0;
}

An obvious way to improve the code is to use standard containers to manage memory instead of raw pointers.

For this code, I would choose std::vector<double> for vector and result, and probably std::vector<std::vector<double>> for matrix (though note that this isn't the most cache-friendly choice for a 2-d matrix).  Remember, we can refer to a std::vector's elements as a plain array using the data() member if needed (here, the iterators should be sufficient).

That allows us to eliminate all the delete[] operations, as destructors will take care of that for us - including the non-local exits where new[] throws std::bad_alloc.

This loop looks wasteful:

    for(unsigned j = 0; j < N; j++){
        if(i==j) {matrix[i][j] = i+j;}
    }

As the body is conditional on i==j, that's simply equivalent to

        matrix[i][i] = i+i;

I don't have experience with MPI, so not commenting on its use.  It's probably worth making N be const, and possibly also defining a useful variable:

auto const chunk_size = N/world.size();

Is the variable env needed?  If constructing a boost::mpi::environment has some useful side-effect, it may be worth an explanatory comment, as it currently looks like an unused variable (if this is something obvious and expected in MPI, then disregard this comment).


Implementation of LRU cache What do you think of this implementation of an LRU cache? I'm trying to migrate to modern C++, so it would be fantastic if you could give me any tips here. I've heard it's not desired to use raw pointers, right? What about only internally, as in the example?

Also, since I can't return a nullptr value in the get() method, what is the best alternative for this? Default value as in the example?

#ifndef __LRU_HPP__
#define __LRU_HPP__
#include <map>
#include<iostream>
#include<assert.h>

template<class T>
struct Default {
  T value;
  Default():value(){}
};

template<class K, class V>
struct node {
  K key;
  V value;
  node<K,V>* next;
  node<K,V>* prev;
  explicit node(K key, V value);
};

template<class K, class V>
class lru {
private:
  node<K,V>* head;
  node<K,V>* tail;
  std::map<K,node<K,V>*> map;
  std::size_t capacity;
  void replace(node<K,V>*);
public:
  explicit lru(std::size_t);
  void put(K key, V value);
  V get(K key);
};

template<class K, class V>
inline node<K,V>::node(K key, V value) {
  this->key = key;
  this->value = value;
}

template<class K, class V>
inline lru<K,V>::lru(std::size_t capacity) {
  assert(capacity > 1);
  this->capacity = capacity;
  this->head = tail = nullptr;
}

template<class K, class V>
void lru<K,V>::put(K key, V value) {
  auto it = map.find(key);
  if (it != map.end()) {
    node<K,V>* temp = it->second;
    temp->value = value;
    replace(temp);
    return;
  }
  if(capacity == map.size()) {
    tail->prev->next = tail->next;
    node<K,V>* target = tail;
    map.erase(target->key);
    tail = tail->prev;
    delete tail;
  }
  node<K,V>* temp = new node<K,V>(key, value);
  temp->prev = nullptr;
  temp->next = head;
  if(head != nullptr)
    head->prev = temp;
  else
    tail = temp;
  head = temp;
  map.insert(std::pair<K,node<K,V>*> {key, temp});
}

template<class K, class V>
V lru<K,V>::get(K key) {
  auto it = map.find(key);
  if(it != map.end()) {
    replace(it->second);
    return it->second->value;
  }
  return Default<V>().value;
}

template<class K, class V>
void lru<K,V>::replace(node<K,V>* temp) {
  if (temp->prev != nullptr)
    temp->prev->next = temp->next;
  if (temp->next != nullptr)
    temp->next->prev = temp->prev;
  if (tail == temp)
    tail = temp->prev != nullptr ? temp->prev : temp;
  if (head != temp) {
    head->prev = temp;
    temp->next = head;
    head = temp;
  }
}
#endif

Namespace Usage

I'd put all of this into some namespace, so it won't accidentally collide with other usage of the names it defines.

I'd probably also work even a little harder to do something to hide your node and Default templates. For example, node doesn't need to be visible to the outside world, so it would probably be better off defined inside of lru.

Prefer Initialization to Assignment

A constructor like this:

template<class K, class V>
inline node<K, V>::node(K key, V value) {
    this->key = key;
    this->value = value;
}

...is typically better written something like this instead:

template <class K, class V>
inline node<K, V>::node(K key, V value)
    : key(key)
    , value(value)
{}

In some cases, you must us initialization (like this) instead of assignment. In this case, it's not mandatory, but I still consider it preferable.

Consider passing by reference

Passing your keys and values by value generally requires copying. You might want to consider whether it's worth passing by reference instead. This generally accomplishes little (or nothing) if the type being passed is "small", but can improve speed substantially if the type is large.

Encapsulate knowledge of a type's internals in that type

Right now, your lru class "knows" (depends upon) the internals of the node class. I'd generally prefer if only node knew about its internals. For one possibility, you might pass a next and prev to the ctor, and let it put those values into the node itself:

template <class K, class V>
inline node<K, V>::node(K key, V value, node *prev = nullptr, node *next = nullptr)
    : key(key)
    , value(value)
    , prev(prev)
    , next(next)
{}

In this case, your code like this:

node<K, V>* temp = new node<K, V>(key, value);
temp->prev = nullptr;
temp->next = head;

...would become something like:

node<K, V> *temp = new node<K, V>(key, value, nullptr, head);

Consider other Data Structures

Although a doubly-linked list does work for the task at hand, it's often fairly wasteful, using a couple of pointers per node in addition to the data you actually care about storing.

I'd consider using a singly linked list instead. Although it may not immediately be obviously how you can do this, I'm reasonably certain you can.

Consider pre-allocating your storage

Given that the total maximum number of objects is fixed when you create the cache, I'd also consider pre-allocating storage for both the objects and the linked-list nodes when you create the cache. An allocator for a fixed-size set of fixed-size blocks can be really trivial, which can improve speed considerably over using a couple of allocations on the free store for every item you insert in the cache.


Comparing SKU numbers Sonar complains that I have too many return statements. How can I reduce the return statement count nicely?

public int compare(Sku o1, Sku o2) {
    if (o1 == null) {
        return (o2 == null) ? 0 : 1;
    } else if (o2 == null) {
        return -1;
    }

    if (o1.getSkuNumber() == null) {
        return o2.getSkuNumber() == null ? 0 : 1;
    } else if (o2.getSkuNumber() == null) {
        return -1;
    }

    return o1.getSkuNumber().compareTo(o2.getSkuNumber());
}

Since you state that you're using Java 8, you should use the variety of new Comparator static methods to extract the relevant fields for comparison:

public int compare(Sku o1, Sku o2) {
    Comparator<Sku> c1 = Comparator.nullsLast(null);
    return c1.thenComparing(Sku::getSkuNumber,
            Comparator.nullsLast(Comparator.naturalOrder())).compare(o1, o2);
}

When Comparator.nullsLast(Comparator) is given a null argument, it considers two non-null argument to be equal. This is the basis of our first comparator.
We then need to compare by getSkuNumber(), so we pass the method reference Sku::getSkuNumber to Comparator.thenComparing(Function, Comparator) as our second comparator. However, since SKU numbers may be null, we use a nested (or tertiary?) null-friendly comparator, Comparator.nullsLast(Comparator) again, as the second argument. The final comparator in use is just Comparator.naturalOrder(), which performs the final straightforward integer comparison if both SKU numbers are non-null.

edit: @Simon mentioned a good point that since your existing compare(Sku, Sku) method is likely to be the implementation for implementing Comparable<Sku>, you should consider making the Comparator I suggested above a public static final field, so that it doesn't have to be created every time, e.g.:

public static final Comparator<Sku> COMPARATOR =
                        Comparator.nullsLast((Comparator<Sku>) null)
                            .thenComparing(Sku::getSkuNumber,
                                Comparator.nullsLast(Comparator.naturalOrder()));


Template matching program to find an image within an image The program I've made is a template matching program which finds an image within an image - where's Wally/Waldo! I've complete the program and all works fine despite being very slow (around a minute). I would appreciate some advice on how I could improve the following code. I'm happy to share the rest of the source code if anyone is interested.

        #include "Matrix.h"

    Matrix::Matrix(int sizeR, int sizeC, double val){
    M = sizeR; // Set number of rows.
    N = sizeC; // Set number of cols.
    data = new double[sizeR * sizeC];
    }

    Matrix::Matrix(int sizeR, int sizeC, double* inputData){
    M = sizeR;
    N = sizeC;

    data = new double[sizeR * sizeC]; // Set data sixe
    for (int i = 0; i < sizeR * sizeC; i++) //  input data store
    {
        data[i] = inputData[i];
    }
    }

    Matrix::~Matrix(){
    delete[] data;
    data = 0;
    }

    Matrix::Matrix(const Matrix& existingMatrix){
    M = existingMatrix.M;
    N = existingMatrix.N;

    data = new double[M * N]; // Allocate memory
    for (int i = 0; i < M * N; i++)
    {
        data[i] = existingMatrix.data[i];
    }
    }

         Matrix Matrix::getBlock(int width, int height, int r, int c, int yMove,     int xMove) const{
    int startCol = r * width;
    int startRow = c * height;
    int cols = width;
    int rows = height;
    Matrix R(rows, cols, 0.0);

    for (int i = 0; i < rows; i++) {

        for (int ii = 0; ii < cols; ii++) {

            R.setElmt(i, ii, getElmt(i + startRow + yMove, ii + startCol + xMove));
        }
        }return R;
     }

    void Matrix::setBlock(int r, int c, int yMove, int xMove, const Matrix& blockValues){
    int rows = blockValues.M;
    int cols = blockValues.N;
    int startCol = c * blockValues.N;
    int startRow = r * blockValues.M;

    for (int i = 0; i < rows; i++) // Rows
    {
        for (int ii = 0; ii < cols; ii++) //Cols
        {
            this->setElmt(i + startRow + yMove, ii + startCol +     xMove, blockValues.getElmt(i, ii));
        }
    }
}

double Matrix::getElmt(int i, int j) const{
    return data[i * N + j];
}

void Matrix::setElmt(int i, int j, double val)
{
    data[i * N + j] = val;
}

int Matrix::getCol() const{
    return N;
}

int Matrix::getRow() const{
    return M;
}

Matrix Matrix::operator-(double value){
    double val = 0;
    Matrix C(M, N, val);

    for (int i = 0; i < M; i++) // Rows
    {
        for (int j = 0; j < N; j++) // Cols
        {
            val = data[i * N + j] - value;
            C.setElmt(i, j, val);
        }
    }return C;
}

Matrix Matrix::operator*(const Matrix& B){
    double val = 0;
    Matrix C(M, N, val);

    for (int i = 0; i < M; i++) // Rows
    {
        for (int j = 0; j < N; j++) // Cols
        {
            val = data[i * N + j] * B.getElmt(i, j);
            C.setElmt(i, j, val);
        }
    }
    return C;
}

Matrix& Matrix::operator=(const Matrix& B){
    if (this == &B) {
        return *this;
    }
    else {
        delete[] data;

        M = B.getRow();
        N = B.getCol();
        data = new double[M * N];

        for (int i = 0; i < M; i++) // Rows
        {
            for (int j = 0; j < N; j++) // Cols
            {
                setElmt(i, j, B.getElmt(i, j));
            }
        }
    }
}

double Matrix::getSum() const{
    double result = 0.0;

    for (int i = 0; i < M * N; i++) {
        result += data[i];
    }
    return result;
}

This code seems very straightforward and easy to read, making it relatively easy to maintain. Here are some ways you could improve it further.
Naming
I would suggest working a bit on your member variable and local variable names. While in math we often use M and N as matrix width and height, it can be confusing which is which, and it can be difficult to communicate which you're talking about when speaking because they sound so similar. I would recommend using width and height as the variable names instead.
Using i and j for loop counters is a well-understood convention and just fine. I would caution against using ii (or iii and iiii as you had in an earlier version of the code) because it's too easy to confuse it with i or make a typo and end up with a subtle bug in your code. (I've done that before!)
Using const
The arguments to almost all of your methods remain unchanged in the method. When that happens, they should be marked as const to let a reader know they won't change. It also helps the compiler in a number of ways.
Performance
You mentioned that it's taking a long time to run. The best way to figure out why is to profile it and see where the slowdown is. I have a guess that it's probably due to the large numbers of copies that the code makes, particularly in getBlock(). I can see 2 ways to improve them:

Don't make a function-call per element you need to copy. Instead use a block copy function like the standard-C library memcpy() function for each row or sub-row you need to copy. This will be much faster than what you're currently doing.
Don't copy anything at all. You could modify the Matrix class to keep track of the width of a row separate from the number of elements in the row. This would allow you to create a new Matrix that just indexed into a current one. You would also need to make sure not to delete the array from that type of matrix, so you'd need another member variable - a bool that says whether it owns the array or not. It's slightly more housekeeping, but the result is that your copies now become a pointer assignment and a little bit of housekeeping instead of actual copies. They almost disappear!