Django class-based generic views - am I doing it right? I am working on a simple "to do list" app in Django (two models: List and Item).  I'm trying to learn and make use of class-based generic views. I have the following three display views working, but am requesting a quick code review to see if there's anything I can do to improve my usage/understanding of Django's ListView and DetailView before I move on to creating, updating and deleting.

# class-based generic views for my lists, a list's items, and item detail...
class MyListsView(ListView):
    """Display the current user's Lists"""
    template_name = 'cmv_app/my_lists.html'
    context_object_name = 'my_lists'

    @method_decorator(login_required)
    def dispatch(self, *args, **kwargs):
        return super(MyListsView, self).dispatch(*args, **kwargs)

    def get_queryset(self):
        return List.objects.filter(user=self.request.user).order_by('-last_modified')

class ListItemsView(ListView):
    """Display a list's items"""
    template_name = 'cmv_app/list_items.html'
    context_object_name = 'list_items'

    @method_decorator(login_required)
    def dispatch(self, request, *args, **kwargs):
        self.list = get_object_or_404(List, pk=kwargs['list_id'])
        request = list_valid_for_user(request, self.list) # security check
        return super(ListItemsView, self).dispatch(request, *args, **kwargs)

    def get_queryset(self):
        return Item.objects.filter(list=self.list).order_by('-created_date')

    def get_context_data(self, **kwargs):
        context = super(ListItemsView, self).get_context_data(**kwargs)
        context['list'] = self.list
        return context

class ItemDetailView(DetailView):
    """Display detail for one list item"""
    template_name = 'cmv_app/item_detail.html'

    @method_decorator(login_required)
    def dispatch(self, request, *args, **kwargs):
        self.list = get_object_or_404(List, pk=kwargs['list_id'])
        self.item = get_object_or_404(Item, pk=kwargs['item_id'])
        request = list_valid_for_user(request, self.list)  # security check
        request = item_valid_for_list(request, self.item, self.list)  # security check
        return super(ItemDetailView, self).dispatch(request, *args, **kwargs)

    def get_object(self):
        return self.item

    def get_context_data(self, **kwargs):
        context = super(ItemDetailView, self).get_context_data(**kwargs)
        context['list'] = self.list
        return context

This looks pretty good -- views should generally be pretty minimal. These seem pretty short, and most of what you've done so far seems appropriate.

One pattern you might think about is using a mixin to dry things out a touch more. Eg, you have all your dispatch methods decorated login_required. If you created a LoginMixin, with and just decorated the dispatch method there, you could loose a few repetitions. Mixins are also nice to have, as you can stash other repetitive elements in them as needed.


VBA loop optimization Hi folks:  I posted a SO (https://stackoverflow.com/questions/57541246/optimizing-vba-function-loop) and was told to ask here.

I am starting to think that rather than relay on excel as my data repository, I should create a separate class that holds a variant array that I can then query must faster??

Anyway... here's my question. I hope someone can help.   I agree that searching arrays may be faster but I need this data available across all functions of this sheet.

I am in need to optimizing some VBA which currently works functionally.

Given columns of sequential Dates (column B) and Times (column C), and
Given a time window (T1 and T2), return a range of rows in which dates and times fall within T1 and T2.  For example, I want MIN and MAX price between those two times.

The goal is to build Open/High/Low/Close charts for Excel candlestick charts and the data source has over 260,000 rows of data.

I currently have the following code that

Dim priceRange As Range
startRowNum = GetFirstRow(StartTime)     << THIS TAKE 10 SECONDS
endRowNum = GetLastRow(endTime)         << THIS TAKE 10 SECONDS
Set priceRange = Range(Cells(startRowNum, 4), Cells(endRowNum, 4))
targetRange.Offset(0, 2).Value = Application.WorksheetFunction.Max(priceRange)
targetRange.Offset(0, 3).Value = Application.WorksheetFunction.Min(priceRange)

To find the first row...

Function GetFirstRow(T As Date) As Long

'Starts at FirstRow and returns the first row where the time is greater than T1.

Dim currentRow As Long
Dim CompareTime As Date
Dim CompareDate As Date

currentRow = 4 'Start at row4 due to headers.

Do While (IsDate(Cells(currentRow, 2)))
    CompareDate = Cells(currentRow, 2)
    CompareTime = Cells(currentRow, 3)
    marketTime = CompareDate + CompareTime
  If (marketTime >= T) Then Exit Do
  currentRow = currentRow + 1
Loop

GetFirstRow = currentRow

End Function

GetLastRow is very similar.

My issue is that the GetFirstRow function has to process 49,000 (yes, forty nine thousand) rows, and it takes about 10 seconds.... so it takes "minutes" to complete this run.

Can someone help me optimize this?

Note I Need the date since market data starts the night before.   If this is what is slowing me down, I can filter that as I import the data?

You are doing twice the work by having a function to get the starting row and a second function get the last row.  Passing the starting row into the GetLastRow() function would be more efficient.

I prefer to have a single function return the range object.  Using the WorkshetFunction.Match() is far more efficient then iterating over the cells.

Results

getDateRange:Function

'Enumerations to clarify column data content
Public Enum DataColumns
    dcStocks = 1
    dcDates
    dcTimes
    dcValues
End Enum

' https://docs.microsoft.com/en-us/office/vba/api/excel.worksheetfunction.match
Function getDateRange(ByVal StartDateTime As Date, ByVal EndDateTime As Date) As Range
    Const LargestValueGreaterThanOrEqualTo As Long = -1
    Const FirstExactMatch As Long = 0
    Const LagestValueLessThanOrEqualTo As Long = 1

    Dim Target As Range
    With ThisWorkbook.Worksheets(1)
        Set Target = .Range("A4:Z4", .Cells(.Rows.Count, dcDates).End(xlUp))
    End With

    Dim dates
    Dim RangeStart As Long, RangeEnd As Long
    Dim SearchValue As Double
    SearchValue = StartDateTime - 1
    On Error Resume Next
    RangeStart = WorksheetFunction.Match(SearchValue, Target.Columns(dcDates), LagestValueLessThanOrEqualTo)
    On Error GoTo 0

    If RangeStart = 0 Then Exit Function
    Dim r As Long
    Dim StartFlag As Boolean
    Dim DateTime As Date

    With Target
        For r = RangeStart To .Rows.Count
            DateTime = .Cells(r, dcDates).Value + .Cells(r, dcTimes).Value
            If DateTime >= StartDateTime And Not StartFlag Then
                RangeStart = r
                StartFlag = True
            End If

            If DateTime > EndDateTime Then
                RangeEnd = r - 1
                Exit For
            End If
        Next

        If r > .Rows.Count Then RangeEnd = .Rows.Count

        Set getDateRange = .Rows(RangeStart & ":" & RangeEnd)
    End With
End Function

Worksheet Test Preparation

Sub Prep()
    Const RowCount As Long = 260000
    'https://codereview.stackexchange.com/questions/226360/vba-loop-optimization
    Dim codes, dates, stocks, times, Values
    Dim d As Date, t As Date
    codes = Array("ACB", "AYI", "A2B", "ABP", "ABL", "AEG", "ABT", "AJC", "AKG", "AX8", "AX1", "ACS", "ACQ", "ACF", "ACR", "ACW", "AIV")

    ReDim stocks(1 To RowCount, 1 To 1)
    ReDim dates(1 To RowCount, 1 To 1)
    ReDim times(1 To RowCount, 1 To 1)
    ReDim Values(1 To RowCount, 1 To 1)
    Dim r As Long, r2 As Long
    d = #1/1/2010#
    For r = 1 To RowCount - 48
        d = d + 1
        For r2 = 0 To 47
            t = TimeSerial(0, r2 * 30, 0)
            stocks(r + r2, 1) = codes(WorksheetFunction.RandBetween(0, UBound(codes)))
            dates(r + r2, 1) = d
            times(r + r2, 1) = t
            Values(r + r2, 1) = Int((Rnd * 100) + 1) + Rnd
        Next
        r = r + r2 - 1
    Next
    Range("A4").Resize(RowCount) = stocks
    Range("B4").Resize(RowCount) = dates
    Range("C4").Resize(RowCount) = times
    Range("D4").Resize(RowCount) = Values
End Sub

Test

Sub Main()
    Dim Results(5) As String * 25
    Const TestCount As Long = 10
    Dim n As Long
    Results(0) = "Date Range"
    Results(1) = "StartDateTime"
    Results(2) = "EndDateTime"
    Results(3) = "MinPrice"
    Results(4) = "MaxPrice"
    Results(5) = "Time"
    Debug.Print Results(0), Results(1), Results(2), Results(3), Results(4), Results(5)
    For n = 1 To TestCount
        Test
    Next
End Sub

Sub Test()
    Dim Results(5) As String * 25

    Dim t As Double: t = Timer
    Dim Target As Range
    Dim d As Date, StartDateTime As Date, EndDateTime As Date

    StartDateTime = WorksheetFunction.RandBetween(#1/2/2010#, #8/30/2024#)
    EndDateTime = StartDateTime + TimeSerial(WorksheetFunction.RandBetween(1, 24) - 1, WorksheetFunction.RandBetween(1, 2) * 60, 0) + WorksheetFunction.RandBetween(1, 60) - 1

    Set Target = getDateRange(StartDateTime, EndDateTime)

    Dim MinPrice As Double, MaxPrice As Double
    MinPrice = WorksheetFunction.Min(Target.Columns(4))
    MaxPrice = WorksheetFunction.Min(Target.Columns(4))

    Results(0) = Target.Address
    Results(1) = StartDateTime
    Results(2) = EndDateTime
    Results(3) = MinPrice
    Results(4) = MaxPrice
    Results(5) = Round(Timer - t, 2)
    Debug.Print Results(0), Results(1), Results(2), Results(3), Results(4), Results(5)
    Target.Select
End Sub

CandleStick Chart

The dataset in the image shows that you need to know the Open, High, Low, and Close for each day to create the Chart.  Considering there are over 200 K rows, I presume that you will also need to filter by stock.  If this is true then I would take a different approach.

I would have a dictionary that stores a sub-dictionary for each stock that stores a dictionary for each day that stores an arraylist to store the values.

Create Array From Data Structure and Write it To New Sheet

Dim CandleStickData

ReDim CandleStickData(1 To RowCount, 1 To 6)
r = 0
For Each StockKey In StockMap
    Set DateMap = StockMap(StockKey)
    For Each DateKey In DateMap
        Set ValueList = DateMap(DateKey)
        r = r + 1
        rowData = ValueList.ToArray
        CandleStickData(r, 1) = StockKey
        CandleStickData(r, 2) = DateKey
        CandleStickData(r, 3) = rowData(0)
        CandleStickData(r, 4) = WorksheetFunction.Max(rowData)
        CandleStickData(r, 5) = WorksheetFunction.Min(rowData)
        CandleStickData(r, 6) = rowData(UBound(rowData))
    Next
Next

Worksheets.Add
Range("A1:F1").Value = Array("Stock", "Date", "Open", "High", "Low", "Close")
Range("A2").Resize(RowCount, 6).Value = CandleStickData
Debug.Print Round(Timer - t)

I did a quick mock up and it took 21 seconds to load 259,967 rows of data into the dictionaries and ArrayList and just 11 seconds to build a new Array and write it to a worksheet.  After the data has been processed, it would be a simply matter of getting the date range and updating the chart table.  Changing the stocks or chart data should take no more than 1 tenth of a second.


DSP-Quantizer class in C++ I have written a class to quantize values to arbitrary bit depths within a specified amplitude.
It is meant to process one value at a time, and has two ways of quantizing values.

I wrote the class so that i could create objects of it as a property for another class, so that the quantization process can easily be included into other, more complex operations. This class I wrote, however, has some things I am quite sure about whether they were solved optimally.

My Header file, Quantizer.h, looks like this:

#pragma once
#include <cmath>
class CQuantizer
{
public:
    ~CQuantizer();

    enum qType {
        lin_midrise,
        lin_midtread,
        numberOfTypes
    };

    CQuantizer(CQuantizer::qType type = CQuantizer::lin_midtread,
        int nBits=4, float amplitude = 1.f);

    float processOneSample(float in);
    void setNBits(int bits);
    void setAmplitude(float amplitude);
    void setType(CQuantizer::qType type);

private:
    int m_nBits;

    float m_amplitude;

    CQuantizer::qType m_type;

    float processOneSampleLinMidtread(float in);
    float processOneSampleLinMidrise(float in);

    float (CQuantizer::*processSampleFunc)(float);
};

The first thing I am wondering about is the enum I used for the types: is it a good idea to include numberOfTypes as last element for later loops through the enum etc.?

Also, I saw somewhere else for a similar class that as some kind of switch, function pointers were used, which I did for the processOneSample-method.
In this scenario, is it appropriate to do so?

Also, below I've included my implementation:

#include "Quantizer.h"

CQuantizer::~CQuantizer()
{
}

CQuantizer::CQuantizer(CQuantizer::qType type, int nBits, float amplitude)
{
    setType(type);
    setNBits(nBits);
    setAmplitude(amplitude);

}

float CQuantizer::processOneSample(float in)
{
    if (m_amplitude == 0.f) {
        // watch out for zero-amplitude
        return 0.f;
    }else{
        // use function specified by type
    return (this->*processSampleFunc)(in);
    }
}

void CQuantizer::setNBits(int bits)
{
    m_nBits = bits;
}

void CQuantizer::setAmplitude(float amplitude)
{
    m_amplitude = amplitude;
}

void CQuantizer::setType(CQuantizer::qType type)
{
    m_type = type;

    switch (m_type) {
    case CQuantizer::lin_midtread:
        processSampleFunc = &CQuantizer::processOneSampleLinMidtread;
        break;
    case CQuantizer::lin_midrise:
        processSampleFunc = &CQuantizer::processOneSampleLinMidrise;
        break;
    }

}

float CQuantizer::processOneSampleLinMidtread(float in)
{
    // upscaling
    float out = std::roundf( (in / m_amplitude) * powf(2.f, m_nBits - 1.f));

    // check for upper boundary
    if (out > powf(2.f, m_nBits - 1.f) - 1.f) {
        out = powf(2.f, m_nBits - 1.f) - 1.f;
    }

    // check for lower boundary
    if (out < -powf(2.f, m_nBits - 1.f)) {
        out = -powf(2.f, m_nBits - 1.f);
    }
    // downscaling
    return m_amplitude * out / powf(2.f, m_nBits - 1.f);
}

float CQuantizer::processOneSampleLinMidrise(float in)
{
    // upscaling
    float out = std::round((in / m_amplitude) * powf(2.f, m_nBits - 1.f) + 0.5f) - 0.5f;

    // check for upper boundary
    if (out > powf(2.f, m_nBits - 1.f) - 0.5f) {
        out = powf(2.f, m_nBits - 1.f) - 0.5f;
    }
    // check for lower boundary
    if (out < -powf(2.f, m_nBits - 1.f) + 0.5f) {
        out = -powf(2.f, m_nBits - 1.f) + 0.5f;
    }
    //downscaling
    return m_amplitude * out / powf(2.f, m_nBits - 1.f);
}

In my processing methods, i feel like there is too much going on: is there a smarter way for me to round these values and ensure they are within a set range?

(The effect I'd like to achieve with this class is that for a given amplitude, there can only be \$2^{\text{nBits}}\$ possible output values within a certain range.)

I see a number of things that may help you improve your program.

Omit empty destructor

The compiler will automatically generate a destructor that would, in this case be identical to the empty one your wrote.  It's better to simply omit it and let the compiler do its thing.

Cleanly separate interface and implementation

In all, the files are fairly neatly divided, but I'd make a couple of changes.  First, I'd move the #include <cmath> to the .cpp file because it's an implementation detail (and as you'll see a bit later, might not be necessary).  Second, I'd remove the processOneSampleLinMidtread() and processOneSampleLinMidrise() from the .h file and make them static free-standing functions in the .cpp file.  More on that in a moment, too.

Avoid comparing floating point numbers for equality

It's generally a good idea to avoid comparing floating point numbers for equality, even if it's only comparing with zero.  See the oft-cited and still excellent What Every Computer Scientist Should Know About Floating-Point Arithmetic, by David Goldberg for a readable technical discussion for why this is so.

Reconsider the interface

At the moment, there's no way to read back out how many bits are in effect and no way to find out what the digital encoding would have been for a particular input float.  I'd suggest a couple of changes to address that.  First, it is probably useful to provide functions for reading back the publically settable parameters.  Second, I'd suggest splitting the current processOneSample into two pieces: int encode(float n) which would return the binary encoding of the input n and float decode(int x) which would return the corresponding floating point value for the encoding x.

Reduce repeated calculations where practical

The expression powf(2.f, m_nBits - 1.f) is used very many times within the class, but it only depends on m_nBits and not on any other value.  For that reason, it would make more sense to calculate it once and store the result.

Avoid floating point where practical

It's often faster to do integer mathematics rather than floating point.  For that reason, and because we know that the result of powf(2.f, m_nBits - 1.f) is always going to be an integer, I'd suggest instead to calculate using the expression 1 << (bits - 1).

Use standard library features

Instead of the current code checking for upper and lower boundaries, it's much more succinctly expressed using std::clamp if you are using a C++17 compiler.  If not, it's not hard to write your own version.

Use const where practical

The processOneSample() routine does not (and should not!) alter the underlying object, so it should be declared const.

float processOneSample(float in) const;

Think about future uses

Many people agree that the YAGNI principle is a sound one, and I agree with that.  But there is a balance between only coding what's needed right now and anticipating at least the possibility that there may be future uses.  An example in this case is using other kinds of digitizing algorithms such as A-law or \$\mu\$-law for audio digitizing.  An alternative approach might be to have a base class (possibly virtual) and then derived classes that implement each kind of alternative encoding.

Think about whether you need a class at all

This may seem heretical for C++, but not everything needs to be an object.  Really what this code is about is turning one floating point value into another.  It may be that freestanding functions (with the appropriate parameters) might work as well.  It depends on how often you anticipate changing the parameters of the base object vs. how many times processOneSample is used.

Consider using a modern constructor style

An alternative to this constructor:

CQuantizer::CQuantizer(CQuantizer::qType type, int nBits, float amplitude)
{
    setType(type);
    setNBits(nBits);
    setAmplitude(amplitude);
}

might be this:

CQuantizer::CQuantizer(CQuantizer::qType type, int nBits, float amplitude) :
    m_nBits{nBits},
    m_amplitude{amplitude},
    m_type{type}
{}


Project Euler 25 - x-digit Fibonacci Number I have solved the Problem 25 of the Euler Project:

  The Fibonacci sequence is defined by the recurrence relation:

  \$\qquad F_n = F_{n−1} + F_{n−2}\$, where \$F_1 = 1\$ and \$F_2 = 1\$.

  Hence the first 12 terms will be:

  \$\qquad F_1 = 1\$

  \$\qquad F_2 = 1\$

  \$\qquad F_3 = 2\$

  \$\qquad F_4 = 3\$

  \$\qquad F_5 = 5\$

  \$\qquad F_6 = 8\$

  \$\qquad F_7 = 13\$

  \$\qquad F_8 = 21\$

  \$\qquad F_9 = 34\$

  \$\qquad F_{10} = 55\$

  \$\qquad F_{11} = 89\$

  \$\qquad F_{12} = 144\$

  The 12th term, F12, is the first term to contain three digits.

  What is the first term in the Fibonacci sequence to contain 1000
  digits?

It was pretty easy to get the result, but i also wanted a solution that is fast and well coded.

The solution of this has two major problems:

It is not possible to save a value of 1000 digits in a standard data type (like long)
Getting the length of the number

The first problem was very easy to solve with the BigInteger structure.

For the second problem i would like to provide the code first:

public class EulerHelper
{
    private readonly int digits;

    public EulerHelper(int digits)
    {
        this.digits = digits;
    }

    private List<BigInteger> RecentNumbers { get; } =
        new List<BigInteger>(new[] {new BigInteger(0), new BigInteger(1)});

    private BigInteger Fibonacci(int n)
    {
        if (RecentNumbers.Count > n)
        {
            return RecentNumbers[n];
        }

        for (var counter = RecentNumbers.Count; counter <= n; counter++)
        {
            var fibCounter = RecentNumbers[counter - 1] + RecentNumbers[counter - 2];
            RecentNumbers.Add(fibCounter);
        }

        return RecentNumbers[n];
    }

    public int NumberWithNDigits()
    {
        var counter = 1;
        while (IsXDigitsLong(Fibonacci(counter)))
        {
            counter++;
        }

        return counter;
    }

    public bool IsXDigitsLong(BigInteger number)
    {
        var calculatedLength = GetLength(number);
        return calculatedLength < digits;
    }

    private int GetLength(BigInteger number)
    {
        if (number == 1)
        {
            return 1;
        }

        return (int) Math.Floor(BigInteger.Log10(number) + 1);
    }
}

The above code is the final one. But before IsXDigitsLong was a "string-length-compare":

public bool IsXDigitsLong(BigInteger number)
{
    return number.ToString().Length < digits;
}

The final implementation is around 3.8 times faster as the string one which was a big win of performance.

The main looks like this:

static void Main(string[] args)
{
    Stopwatch sw = Stopwatch.StartNew();
    var executer = new EulerHelper(1000);
    var result = executer.NumberWithNDigits();
    sw.Stop();

    Console.WriteLine($"The result is {result}");
    Console.WriteLine($"Execution time: {sw.ElapsedTicks}");
    Console.ReadKey();
}

Results (Benchmark)

I have done some very rudamentary benchmarks between the .ToString() and the GetLength() implementation:

Method          Time in StopWatch ticks
ToString()      568476
GetLength()     143699

I feel like a class is not a good way to approach this problem.

C# forces you to write classes, but using static methods will allow you to ignore this.

The only method you should keep non-static is the on calculating the n-th fibonacci, as the memoization requires state, and a class is a way to keep state.

Getting the number of digits of a number is instead something without atate that depends only on the input number, so:

public static digitsLength(BigInteger n) {
    return number == 1 ? 1 : (int) Math.Floor(BigInteger.Log10(number) + 1);
}

Minor remarks are:

Naming: You used get in the name, but in OO get has a precise meaning that is inappropriate here.
Ternary: When the condition is simple, like here, using a ternary increases compactness and readability.

Now main is simply:

for (var i upto infinity) {
    if DigitsLength( Fibonacci(i) ) == REQUIRED_DIGITS {
        print( Fibonacci(i) )
    }
}

PS: Note that the above is pseudo-code, it is just to give you an idea.


Ruby Rails - Snippets from a project that don't seem so eloquent I'm brand new to both Ruby and Rails, and while all the following works, it seems a bit messy.

This is on a classified model to generate WHERE/AND WHERE clauses based on whether or not the user supplies a value through a dropdown. I'm not sure if there's a more Active Recordy way of doing this:

class Classified < ActiveRecord::Base
   attr_accessible :car_id, :owner_id, :owner, :price, :condition, :description, :mileage
   belongs_to :car
   belongs_to :owner, :polymorphic => true
   has_one :model, :through => :car
   has_one :make, :through => :model
   has_many :pictures
 scope :filter_active_cars, lambda { |options|
 filter = {}
 filter[:cars] = {:year => options[:year]} unless options[:year].empty?
 filter[:makes] = {:id => options[:make]} unless options[:make].empty?
 filter[:models] = {:id => options[:model]} unless options[:model].empty?

 joins(:car, :model, :make)
 .where(filter)
 .includes(:car, :model, :make)
 }

The view has this

=select_tag('car[year]', options_for_select(years, :selected => @selected_year))
=collection_select(:car, :make, Make.all, :id, :name, {:include_blank => '- Make -', :selected => @selected_make})
=collection_select(:car, :model, @selected_make ? Model.where(:make_id => @selected_make) : Model.all, :id, :name, {:include_blank => '- Model -', :selected => @selected_model})

This is the way I'm passing the "select state" on the classified view. The controller has this snippet in the index action. This is so when a user filters a result and submits the form, the corresponding select box has the same value for the form helpers above.

if params[:car]
  params[:car].each do |option, value|
    next if value.empty?
    eval("@selected_#{option} = #{value}")
  end
end

On the form partial to generate and edit a classified. Obviously the chaining of &&s sucks, but it's the only way to get the default selection on new, edit, and when a user submits a new form that contains errors.

=select_tag('car[year]', options_for_select(years, :selected => @classified.car.nil? ? nil : @classified.car.year), :prompt => "- Year -")
=collection_select(:make, :id, Make.all, :id, :name, {:prompt => '- Make -', :selected => (@classified.car && @classified.car.make && @classified.car.make.try(:id))})
=collection_select(:car, :model_id, Model.all, :id, :name, {:include_blank => '- Model -', :selected => (@classified.car && @classified.car.model && @classified.car.model.try(:id))})

Maybe you can convert this:

scope :filter_active_cars, lambda { |options|
 filter = {}
 filter[:cars] = {:year => options[:year]} unless options[:year].empty?
 filter[:makes] = {:id => options[:make]} unless options[:make].empty?
 filter[:models] = {:id => options[:model]} unless options[:model].empty?

 joins(:car, :model, :make)
 .where(filter)
 .includes(:car, :model, :make)
 }
}

to this:

scope :filter_active_cars, lambda do |options|
 with_option(:car, 'cars.year', :year).
 with_option(:make, 'makes.id', :make).
 with_option(:model, 'models.id', :model)
end

scope :with_option, lambda do |assoc, column, option|
  (options[option].present? ?
    joins(assoc).where(column => options[option]) :
    self).
  includes(assoc)
end

You can reuse the with_option scope in other models


Download an image from a webpage I am trying to write a Python script that download an image from a webpage.  On the webpage (I am using NASA's picture of the day page), a new picture is posted everyday, with different file names. After download, set the image as desktop.

My solutions was to parse the HTML using HTMLParser, looking for "jpg", and write the path and file name of the image to an attribute (named as "output", see code below) of the HTML parser object.

The code works, but I am just looking for comments and advice.  I am new to Python and OOP (this is my first real python script ever), so I am not sure if this is how it is generally done.

import urllib2
import ctypes
from HTMLParser import HTMLParser

# Grab image url
response = urllib2.urlopen('http://apod.nasa.gov/apod/astropix.html')
html = response.read()

class MyHTMLParser(HTMLParser):

    def handle_starttag(self, tag, attrs):
        # Only parse the 'anchor' tag.
        if tag == "a":
           # Check the list of defined attributes.
           for name, value in attrs:
               # If href is defined, print it.
               if name == "href":
                   if value[len(value)-3:len(value)]=="jpg":
                       #print value
                       self.output=value

parser = MyHTMLParser()
parser.feed(html)
imgurl='http://apod.nasa.gov/apod/'+parser.output
print imgurl

# Save the file
img = urllib2.urlopen(imgurl)
localFile = open('desktop.jpg', 'wb')
localFile.write(img.read())
localFile.close()

# set to desktop(windows method)
SPI_SETDESKWALLPAPER = 20
ctypes.windll.user32.SystemParametersInfoA(SPI_SETDESKWALLPAPER, 0, "desktop.jpg" , 0)

# Save the file
img = urllib2.urlopen(imgurl)
localFile = open('desktop.jpg', 'wb')
localFile.write(img.read())
localFile.close()

For opening file it's recommend to do:

with open('desktop.jpg','wb') as localFile:
     localFile.write(img.read())

Which will make sure the file closes regardless.

Also, urllib.urlretrieve does this for you.


Review Simple Logparser This is my first try with Python. I wanted to parse some Log4J so I thought it a good opportunity to write my first Python program.

The format of the logs I deal with are simply like [INFO BLA 08:00:00] Blabla. Most of the time there are single lines  like the above; but sometimes, there is a stack trace logged, so a logical "line" is longer than a physical line. Each "logical" line starts with the character [.

So I came up with

#!/usr/bin/env python

import sys
from collections import namedtuple

Logline = namedtuple("Logline", "level timestamp message")

def apply_filter(current_log_line, filtered_lines):
    parsed_line = parse_line(current_log_line)
    if filter(parsed_line):
        filtered_lines.append(parsed_line)

def get_filtered_lines(source):
    filtered_lines = []
    line_buffer = ""
    for line in source:
        if line.startswith('[') and line_buffer:
            apply_filter(line_buffer, filtered_lines)
            line_buffer = ""
        line_buffer += line
    apply_filter(line_buffer, filtered_lines)
    return filtered_lines

def parse_line(log_line):
    prefix, message = log_line.split("]", 1)
    level, timestamp = parse_prefix(prefix)
    return Logline(level, timestamp, message)

def parse_prefix(prefix):
    prefix = prefix.strip("[]")
    tokens = prefix.split(" ")
    level = tokens[0]
    timestamp = tokens[-1]
    return level, timestamp

def filter(log_line):
    return log_line.level == "ERROR" and log_line.timestamp > "07:00:00"

def console_out(lines):
    for line in lines:
        print("{}{}".format(line.timestamp, line.message.rstrip("\n")))

def main():
    if len(sys.argv) != 2:
        print("Usage: {0} $logfile".format(sys.argv[0]))
    else:
        file_name = sys.argv[1]
        with open(file_name) as file:
            console_out(get_filtered_lines(file))

if __name__ == '__main__':
    main()

Feel free to comment.

One thing, what bugs me is the apply_filter function, which does too many things at once. But I have have no solution for that.

Let's see if I understand what's going on with get_filtered_lines() and apply_filter(). First, get_filtered_lines() reads physical lines from the file and strings them together in line_buffer. When we find the beginning of the next logical line ('['), we pass line_buffer off to apply_filter() so the material that has been collected gets processed. Then we empty out line_buffer and start over with the beginning of the logical line just read.

Here's an approach I think is simpler and clearer:

def apply_filter(lbuf):
    parsed = parse_line(lbuf)
    if filter(parsed):
        return [parsed]
    else
        return []

def get_filtered_lines(source):
    filtered_lines = []
    collected = ""
    for line in source:
        if line.startswith('[') and collected:
            filtered_lines.extend(apply_filter(collected))
            collected = ""
        collected += line
    filtered_lines.extend(apply_filter(collected))
    return filtered_lines

That way, apply_filter doesn't have to be aware of the list being built. It just returns a list of what it finds -- either a line that passes the filter or an empty list. Passing an empty list to extend is a no-op.


Refactoring scoring algorithm I have the following code which I feel that I must refactor, but not sure how to do it.

List<AlgoTuningData> feedbackScoreDatas;
List<AlgoTuningData> topRatedDatas;
int score1 = 0;
int score2 = 0;
boolean matched1 = false;
boolean matched2 = false;
for (AlgoTuningData topRatedData : topRatedDatas) {
    if (!matched1 && isMatchesTopRatedTuning(p1, topRatedData)) {
        score1 += topRatedData.getRank();
        matched1 = true;
    }

    if (!matched2 && isMatchesTopRatedTuning(p2, topRatedData)) {
        score2 += topRatedData.getRank();
        matched2 = true;
    }

    if (matched1 && matched2)
        break;
}

matched1 = false;
matched2 = false;
for (AlgoTuningData feedbackScoreData : feedbackScoreDatas) {
    if (!matched1 && isMatchesFeedbackScoreTuning(p1, feedbackScoreData))           {
        score1 += feedbackScoreData.getRank();
        matched1 = true;
    }
    if (!matched2 && isMatchesFeedbackScoreTuning(p2, feedbackScoreData)) {
        score2 += feedbackScoreData.getRank();
        matched2 = true;
    }

      if (matched1 && matched2)
      break;
}

private boolean isMatchesTopRatedTuning(Product product, AlgoTuningData topRatedData) {
        return product.getIsTopRated().equals(topRatedData.getValue());
    }

private boolean isMatchesFeedbackScoreTuning(Product product, AlgoTuningData feedbackScoreData) {
        return product.getPrice() <= feedbackScoreData.getMaxProductPrice();
    }

As you can see, I have 2 lists. each list has a different type of boolean function that verifies its objects. Each call to the function increases the score1 and score2.

I thought to use Callable, but wasn't sure how to do it for a list of items.

I would like my code to have one-line call for each List<AlgoTuningData>, something like this:

increaseScore(List<Integer> scores, List<AlgoTuningData>, myFunc)

I would not refactor this substantially, except maybe to avoid the reuse of the matched1 and matched2 variables:

{
    boolean matched1 = false;
    boolean matched2 = false;
    for (...) {
        ...
    }
}

// and here again

Your intuition is correct that this could be done much cleaner using functional programming, but FP is a pain with Java 1–7. So we have the following options:

use Java 8
use functional language like Scala
ignore the pain, and do it anyway

The last solution does not involve java.util.concurrent.Callable, but an interface of our own:

interface TuningDataTest {
    boolean test(Product product, AlgoTuningData datum);
}

We can then write a function scoreTuningData:

private static class Score {
    final public int score1;
    final public int score2;

    public Score(int s1, int s2) {
        score1 = s1;
        score2 = s2;
    }

    public Score add(Score that) {
        return new Score(this.score1 + that.score1, this.score2 + that.score2);
    }
}

private Score scoreTuningData(Product p1, Product p2, Iterable<AlgoTuningData> data, TuningDataTest test) {
    boolean matched1 = false;
    boolean matched2 = false;
    int score1 = 0;
    int score2 = 0;

    for (AlgoTuningData datum : data) {
        if (!matched1 && test.test(p1, datum)) {
            matched1 = true;
            score1 += datum.getRank();
        }
        if (!matched2 && test.test(p2, datum)) {
            matched2 = true;
            score2 += datum.getRank();
        }
        if (matched1 && matched2) {
            break;
        }
    }

    return new Score(score1, score2);
}

Note that “data” is the plural form of “datum”, the word “datas” does not exist. One “datum” is a “record”, “data” are a set of records.

And your main code would then look like:

List<AlgoTuningData> topRatedData = ...;
List<AlgoTuningData> feedbackScoreData = ...;
Score score = scoreTuningData(p1, p2, topRatedData, new TuningDataTest() {
    @Override
    public boolean test(Product product, AlgoTuningData datum) {
        return product.getIsTopRated().equals(datum.getValue());
    }
}).add(scoreTuningData(p1, p2, feedbackScoreData, new TuningDataTest() {
    @Override
    public boolean test(Product product, AlgoTuningData datum) {
        return product.getPrice() <= datum.getMaxProductPrice();
    }
}));

int score1 = score.score1;
int score2 = score.score2;


LOL'ing-Up Project Euler One It's been a while since I last wrote some lolcode, so I felt like tackling Project Euler #1.

  If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

  Find the sum of all the multiples of 3 or 5 below 1000.

This is the code/script I wrote to solve this:

HAI 1.2
    VISIBLE "HAI, PROJEK LOLLER ONE!!"

    I HAS A LIMIT ITZ 1000
    I HAS A TOTAL ITZ 0
    I HAS A CHEEZ ITZ 3
    I HAS A BURGER ITZ 5
    I HAS A CHEEZBURGER ITZ PRODUKT OF CHEEZ AN BURGER

    HOW IZ I ADDTOTAL YR VALUE
        I HAS A RESULT ITZ SUM OF VALUE AN TOTAL
        FOUND YR RESULT
    IF U SAY SO

    IM IN YR MIND UPPIN YR NUMBER TIL BOTH SAEM NUMBER AN LIMIT

        I HAS A PICKLE ITZ FAIL

        BOTH SAEM 0 AN MOD OF NUMBER AN CHEEZBURGER
        O RLY?, YA RLY
            PICKLE R WIN
        NO WAI
            BOTH SAEM 0 AN MOD OF NUMBER AN CHEEZ
            O RLY?, YA RLY
                PICKLE R WIN
            NO WAI
                BOTH SAEM 0 AN MOD OF NUMBER AN BURGER
                O RLY?, YA RLY
                    PICKLE R WIN
                OIC
            OIC
        OIC

        BOTH SAEM PICKLE AN WIN
        O RLY?, YA RLY
            TOTAL R I IZ ADDTOTAL YR NUMBER MKAY
        OIC

    IM OUTTA YR MIND

    VISIBLE SMOOSH "TEH ANSWER IZ " AN TOTAL MKAY
    VISIBLE "DOWN WITH PROJEK LOLLER ONE!"

KTHXBYE

This code can be executed on CodingGround, and produces this output:

  HAI, PROJEK LOLLER ONE!!

(spoiler)

      TEH ANSWER IZ 233168

  DOWN WITH PROJEK LOLLER!

Is the fizzbuzz-like logic appropriate, or I fell in the "easy" trap? Would there be a way to collapse all the conditions into one and make a shorter script? Or is this algorithm a bad/inefficient one?

Same Guy, Same Language, same criticism?
Let me start by (again) critzising your variable casing choice of SHOUTCASE
It's so darn hard to tell what's a variable and what's not :(
But your names got better since the last time. Cheez and Burgr aren't the best, but given their content is arbitrarily set and accordingly somewhat meaningless they really stand their ground.
One thing though: your Cheezburgers are not needed.
It's a waste of computation power to check divisors by the biggest one first if you return early. (I recall rolfl having some nice stats on that in one fizz-buzz review I can't find right now). We can completely elimimate that O RLY? YA RLY from your code.
Oh and while we're at cheeseburgers... There's one thing I really dislike on cheeseburgers... Pickles.
The temporary TROOF variable to remove that three YA RLY doing the same thing is completely overkill. Instead of PICKLES R WIN it would be sufficient to add the current Number to the total.
Code until now:
IM IN YR Mind UPPIN YR Number TIL BOTH SAEM Number AN Limit
    BOTH SAEM 0 AN MOD OF Number AN Cheez
    O RLY?, YA RLY
          Total R I IZ AddToTotal YR Number MKAY
    NO WAI
          BOTH SAEM 0 AN MOD OF Number AN Burger
          O RLY?, YA RLY
                Total R I IZ AddToTotal YR Number MKAY
          OIC
    OIC
IM OUTTA YR Mind

Now what have we got here? this looks like it should be either an ElseIf or just a simple combination of conditions into an Or.
The latter is the easier one, but for reference: There is an else if. It's MEBBE
Final Code:
HAI 1.2
    VISIBLE "HAI, PROJEK LOLLER ONE!!"

    I HAS A Limit ITZ 1000
    I HAS A Total ITZ 0
    I HAS A Cheez ITZ 3
    I HAS A Burger ITZ 5

    HOW IZ I AddToTotal YR Value
        I HAS A Result ITZ SUM OF Value AN Total
        FOUND YR Result
    IF U SAY SO

    BTW Iterates from 0 to 999
    IM IN YR Mind UPPIN YR Number TIL BOTH SAEM Number AN Limit
        EITHER OF BOTH SAEM 0 MOD OF Number AN Cheez AN BOTH SAEM 0 MOD OF Number AN Burger
        O RLY?, YA RLY
             Total R I IZ AddToTotal YR Number MKAY
        OIC
    IM OUTTA YR Mind

    VISIBLE SMOOSH "TEH ANSWER IZ " AN Total MKAY
    VISIBLE "DOWN WITH PROJEK LOLLER ONE!"

KTHXBYE


Finding the class to which the IP address belongs I have tried the code for finding the class of the IP address the user inputs, and printing the network and the host bits in the IP address. Could someone please review this approach and provide suggestions. Thanks:).

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<sys/socket.h>
#include<netinet/in.h>
#include<arpa/inet.h>
#include<error.h>
#include<errno.h>
#define size 32
int bin[size];
void print(int class, int nb, int hb);
void convert_to_binary(uint32_t decimal);

void print(int class, int nb, int hb)
{
    int i = 0;

    printf("The Network bits are:\n");
    for(i = class; i < (nb + class); i++) {
        printf("%d ", bin[i]);
    }
    printf("\n");
    printf("The host bits are:\n");
    for(i = (class + nb); i < size; i++) {
        printf("%d ", bin[i]);
    }
}

void convert_to_binary(uint32_t decimal)
{

    int index = 0;
    int temp = 0;
    int i = 0;

    while (decimal > 0) {
        temp = decimal % 2;
        bin[index++] = temp;
        decimal = decimal / 2;
    }
    for (i = 0; i < size / 2; i++) {
        temp = bin[i];
        bin[i] = bin[size - i - 1];
        bin[size - i - 1] = temp;
    }

}

int main(int argc, char **argv)
{
    struct in_addr addr;
    uint32_t result;
    char *IP;
    int ret = 0;
    int value = 0;

    if (argc != 2)
        error(1, errno, "Too many or few arguments\n");
    IP = argv[1];
        ret = inet_aton(IP, &addr);
    if (ret == 0)
        error(1, errno, "Invalid IP- address provided\n");
    printf("The address in the structure is network byte order:(Big- Endian) %X %d\n", addr, addr);
    result = ntohl(addr.s_addr);
    convert_to_binary(result);
    if (bin[0] == 0) {
        printf("Class - A\n");
        print(1, 7, 24);
    } else if (bin[0] == 1 && bin[1] == 0) {
        printf("Class - B\n");
        print(2, 14, 24);
    } else if(bin[0] == 1 && bin[1] == 1 && bin[2] == 0) {
        printf("Class - C\n");
        print(3, 21, 8);
    } else {
        printf("It does not belong to any of the class\n");
    }
}
```

Here are some things that may help you improve your code.

Eliminate unused variables

The parameter hb in the print() function and value in main() are unused variables and should be omitted from the program.

Fix your formatting

There are inconsistent spaces at the beginning of lines and inconsistent indentation.  Being consistent helps others read and understand your code.

Use only required #includes

The code has a number of #includes that are not needed.  In this code, for instance, nothing from <string.h> is used, so that header can and should be omitted.  Only include files that are actually needed.  This makes the code easier to understand and maintain and also may slightly speed up compiling.

Understand what standards don't guarantee

A knowledgeable reader of this code (and possibly the compiler) is likely to be puzzled by this line of code:

printf("The address in the structure is network byte order:(Big- Endian) %X %d\n", addr, addr);

The reason is that "%X" expects an unsigned int and "%d" expects an int but what you're passing is struct in_addr.  The man page tells us that the relevant defintion for that is as follows:

typedef uint32_t in_addr_t;

struct in_addr {
   in_addr_t s_addr;
};

This means that on machines where int is not the same size as uint32_t, there is a potential for trouble.  Specifically, if the code is compiled for a machine with a 16-bit int, this won't give the results you intend.  Better would be to have the compiler check to make sure that this doesn't happen.  One way to do this is to add these lines to the program:

#include <limits.h>
#if UINT_MAX == 65535
#error "This program requires at least a 32-bit unigned int."
#endif

Note that sizeof can't be used for the preprocessor, which is why we use the definition of UINT_MAX.  This isn't foolproof, since there are real machines using 24-bit words, but catches the more common case of a 16-bit.  To be completely sure, we can't really use the preprocessor.  We could do this:

if (sizeof(int) < 4) {
    printf("This program requires at least 32-bit int, but yours has a %ld-bit int\n", sizeof(int) * 8);
    return 1;
}

Then instead of passing addr to your printf, pass addr.s_addr instead.

Try to write portable code

Things that are guaranteed by the C standard are absolutely portable to conforming compilers on all platforms.  Things that are specified by POSIX standards are portable to the subset of machines that conform to POSIX.  Your life will be easier if you aim for those.  Things that are compiler extensions, such as error are not necessarily portable to other machines.  For that reason, I'd suggest that instead of using error in this program (which is a GNU extension), you could use the form shown above and use printf and return.  It's also worth noting that inet_aton is not actually in the POSIX standard, but is widely used (and much better than the poorly defined but standard inet_addr).  However, it may have some surprising attributes.  For example, it will interpret a single number as an IP address.  If we give the argument 3232236027 it will interpret this as the address 192.168.1.251.  If that's what you want, that's fine.  If not, you might consider instead using inet_pton (which is POSIX):

ret = inet_pton(AF_INET, IP, &addr);

Eliminate global variables where practical

The code declares and uses a global variable, bin.  Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult.  It also makes the code harder to reuse.  For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them.  That way the linkage is explicit and may be altered more easily if needed.  In this case, you could define bin within main like this:

int bin[32];

And then call print like this:

printf("Class - A\n");
print(1, 7, bin, 32);

Eliminate complexity where practical

The convert_to_binary() function is rather complex internally.  I'd recommend simplifying it or eliminating it entirely.  Here's one way to simplify it:

void convert_to_binary(uint32_t decimal, int *bin, size_t size)
{
    bin += size - 1;
    for ( ; size; --size) {
        *bin-- = decimal & 1u;
        decimal >>= 1;
    }
}

Rethink function interfaces

The essential piece of work done in the program is to identify the address class of the passed IPv4 address.  I'd put that into a function like this:

char address_class(uint32_t addr) {
    const struct class_mask {
        uint32_t mask;
        uint32_t classbits;
        char designator;
    } class_mask[3] = {
        { 0x80000000u, 0x00000000u, 'A' },
        { 0xc0000000u, 0x80000000u, 'B' },
        { 0xe0000000u, 0xc0000000u, 'C' },
    };
    for (unsigned i=0; i < 3; ++i) {
        if ((addr & class_mask[i].mask) == class_mask[i].classbits) {
            return class_mask[i].designator;
        }
    }
    return '?';
}

With a small change, we could actually turn this into the print function.  If we do so, the entire program then looks like this:

#include <stdio.h>
#include <arpa/inet.h>

char print_classification(uint32_t addr) {
    const struct class_mask {
        uint32_t mask;
        uint32_t classbits;
        uint32_t netmask;
        uint32_t hostmask;
        char designator;
    } class_mask[3] = {
        { 0x80000000u, 0x00000000u, 0x40000000u, 0x00800000u, 'A' },
        { 0xc0000000u, 0x80000000u, 0x20000000u, 0x00008000u, 'B' },
        { 0xe0000000u, 0xc0000000u, 0x10000000u, 0x00000080u, 'C' },
    };
    for (unsigned i=0; i < 3; ++i) {
        if ((addr & class_mask[i].mask) == class_mask[i].classbits) {
            printf("class = %c\nThe network bits are:\n", class_mask[i].designator);
            for (uint32_t mask = class_mask[i].netmask; mask; mask >>= 1) {
                if (mask == class_mask[i].hostmask) {
                    printf("\nThe host bits are:\n");
                }
                printf("%c ", addr & mask ? '1' : '0');
            }
            printf("\n");
            return class_mask[i].designator;
        }
    }
    printf("Unknown address class\n");
    return '?';
}

int main(int argc, char **argv)
{
    struct in_addr addr;

    if (sizeof(int) < 4) {
        printf("This program requires at least 32-bit int, but yours has a %ld-bit int\n", sizeof(int) * 8);
        return 1;
    }
    if (argc != 2) {
        printf("Usage: %s ipv4addr\nwhere ipv4addr is of the form xxx.xxx.xxx.xxx\n", argv[0]);
        return 1;
    }
    if (inet_pton(AF_INET, argv[1], &addr) == 0) {
        printf("Error: couldn't interpret %s as an IPv4 address\n", argv[1]);
    }
    print_classification(ntohl(addr.s_addr));
}

Rethink the purpose of the program

Perhaps this was just a programming exercise, but it's probably worth pointing out that the use of IPv4 address classes has been obsolete since the introduction of CIDR in 1993.


LeetCode 146: LRU Cache I I'm posting my C++ code for LeetCode's LRU Cache. If you have time and would like to review, please do so. Thank you!
Problem

Design and implement a data structure for Least Recently Used (LRU) cache. It should support the following operations: get and put.

get(key) - Get the value (will always be positive) of the key if the key exists in the cache, otherwise return -1.

put(key, value) - Set or insert the value if the key is not already present. When the cache reached its capacity, it should invalidate the
least recently used item before inserting a new item.

The cache is initialized with a positive capacity.

Follow up:

Could you do both operations in O(1) time complexity?

Example:
LRUCache cache = new LRUCache( 2 /* capacity */ );

cache.put(1, 1);
cache.put(2, 2);
cache.get(1);       // returns 1
cache.put(3, 3);    // evicts key 2
cache.get(2);       // returns -1 (not found)
cache.put(4, 4);    // evicts key 1
cache.get(1);       // returns -1 (not found)
cache.get(3);       // returns 3
cache.get(4);       // returns 4

Accepted C++
#include <list>
#include <unordered_map>

class LRUCache {
public:
    const int size;
    std::list<size_t> lru;
    std::unordered_map<int, std::list<size_t>::iterator> cache;
    std::unordered_map<int, int> key_val_map;

    LRUCache(const int capacity) : size(capacity) {}

    // Getter constant memory
    int get(int key) {
        if (key_val_map.count(key) == 0) {
            return -1;
        }

        update(key);
        return key_val_map[key];
    }

    // Setter constant memory
    const void put(int key, int value) {
        if (key_val_map.size() == size && key_val_map.count(key) == 0) {
            clear();
        }

        update(key);

        key_val_map[key] = value;
    }

    // Add a new key
    const void update(int key) {
        if (key_val_map.count(key)) {
            lru.erase(cache[key]);
        }

        lru.push_front(key);
        cache[key] = lru.begin();
    }

    // Erase cache
    const void clear() {
        key_val_map.erase(lru.back());
        cache.erase(lru.back());
        lru.pop_back();
    }
};

Reference
On LeetCode, there is a class usually named Solution with one or more public functions which we are not allowed to rename.

146. LRU Cache - Problem

146. LRU Cache - Discuss

Use size_t for sizes
Although the LeetCode question specifies that the constructor takes an int capacity, using an int to hold a size is not appropriate for two reasons:

int might not be big enough to handle all possible sizes that fit into the available memory.
int is signed, and now you have to deal with potentially negative numbers.

Also note that the standard library uses size_t for things like .size(), and the result of the sizeof operator is also a size_t. So it's best to internally store the capacity as a size_t. This will avoid compiler warnings about comparisons between signed and unsigned values.
Use types consistently
The one place you use size_t is in std::list<size_t> lru. But here, the list is actually holding keys. Everywhere else you write int key, so you should write std::list<int> lru here, otherwise your cache might not work correctly when using negative numbers for keys. The LeetCode question does not say whether or not negative keys are allowed, it only mentions only positive values are stored.
Make helper functions private
Helper functions like update() and clear() are not part of the public API as specified by the LeetCode problem. So make them private.
Use proper names
The function clear(), despite its name and even the comment above it, does not erase the cache. Instead, it just removes the least recently used element. Make sure the name (and also the comment) reflects this. I would name it something like pop_lru(), or perhaps just pop().
Also, the name update() does not match the comment above it. I would remove the comment, and give a more descriptive name: make_most_recent().
Useless use of const
It does not make sense to have a function return const void. Just write void.


Compare version numbers I am wondering if there is any way to meaningfully shorten the chain of conditionals used here to compare two versions. struct VERSIONCODE represents a version number of the form major.minor.revision and CompareVersions returns EQUAL if they're equal; LHS_NEWER if vLHS is newer, and RHS_NEWER if vRHS is newer.

typedef enum
{
    EQUAL = 0,
    LHS_NEWER,
    RHS_NEWER
} ECOMPARISON;

// Specifically not named "VERSION" to avoid conflicting with common names from third-party libraries etc.
typedef struct
{
    int nMajor;
    int nMinor;
    int nRev;
} VERSIONCODE;

ECOMPARISON CompareVersions(VERSIONCODE vLHS, VERSIONCODE vRHS)
{
    if (vLHS.nMajor > vRHS.nMajor)
    {
        return LHS_NEWER;
    }
    else if (vLHS.nMajor < vRHS.nMajor)
    {
        return RHS_NEWER;
    }
    else// if (vLHS.nMajor == vRHS.nMajor)
    {
        if (vLHS.nMinor > vRHS.nMinor)
        {
            return LHS_NEWER;
        }
        else if (vLHS.nMinor < vRHS.nMinor)
        {
            return RHS_NEWER;
        }
        else// if (vLHS.nMinor == vRHS.nMinor)
        {
            if (vLHS.nRev > vRHS.nRev)
            {
                return LHS_NEWER;
            }
            else if (vLHS.nRev < vRHS.nRev)
            {
                return RHS_NEWER;
            }
            else// if(vLHS.nRev == vRHS.nRev)
            {
                return EQUAL;
            }
        }
    }
}

typedef enum
{
    EQUAL = 0,
    LHS_NEWER,
    RHS_NEWER
} ECOMPARISON;

A common convention for three-way comparisons is that of strcmp() - return any negative value if the first argument is less than the second, zero if equal, and positive if greater.

If we take that approach, and if we can ensure that the version values are small enough to avoid integer overflow, we can simplify the comparison to just:

int CompareVersions(VERSIONCODE a, VERSIONCODE b)
{
    int diff = a.nMajor - b.nMajor;
    if (diff) return diff;
    diff = a.nMinor - b.nMinor;
    if (diff) return diff;
    return a.nRev - b.nRev;
}

If we can't ensure overflow won't happen, or if we absolutely must return fixed values, we'll need to convert those subtractions into calls to a custom comparison function - perhaps like this:

int compare_int(int a, int b)
{
    /* standard idiom to return -1, 0 or +1 */
    return (a > b) - (b > a);
}

The naming can be improved.  Most C conventions use ALL_UPPERCASE only for macros, to call attention to their difference from C code.  And most authorities discourage prefixes like the n and v used at the start of variable and member names - I encourage you to research "Hungarian notation" and understand the arguments.


JSON Parser, pulls data from a JSON file for analysis This is the first time I have programmed something that I will actually personally use, so I am happy about that. I tried to write good code whilst applying functional programming concepts. So no state, and a concise function.

I know that when making the lists it pulls information from the JSON that doesn't get used later in the program. The data will be used once I get continue to develop the program further.

At the end there is a dictionary containing hero ID keys and name values. I cut out most of it to minimize the length of this post.

import json
import requests
def parsestats(herodata):
    return [(hero['heroId'],
    hero['pickBan']['pick']['matchCount']/ # Divide by next line
        herodata['matchPickCount']*1000,
    hero['pickBan']['ban']['matchCount']/  # Divide by next line
        herodata['matchBanCount']*1000,
    hero['pickBan']['pick']['wins']*100,
    hero['pickBan']['ban']['wins']*100,
    hero['pickBan']['pickLastWeek']['matchCount']/ # Divide by next line
        herodata['matchLastWeekPickCount']*1000,
    hero['pickBan']['banLastWeek']['matchCount']/  # Divide by next line
        herodata['matchLastWeekBanCount']*1000,
    hero['pickBan']['pickLastWeek']['wins']*100,
    hero['pickBan']['banLastWeek']['wins']*100)
            for hero in herodata['heroes']]
print(f"{'Hero,':<20}{'Value':>7}")
for hero in sorted(
    [(uni[0], imm[1]*((uni[3] if uni[3] > imm[3] else imm[3])-50))
    for uni, imm in zip(
        sorted(parsestats(json.loads(requests.get(
        'https://api.stratz.com/api/v1/Hero/directory/simple').text))),
        sorted(parsestats(json.loads(requests.get(
        'https://api.stratz.com/api/v1/Hero/directory/simple?rank=8').text))))
    ],
    key=lambda tup: tup[1], reverse = True):
    print("{:<20}{:>7.2f}".format({1: 'Anti-Mage', 2: 'Axe', 3: 'Bane', 4: 'Bloodseeker', 5: 'Crystal Maiden', 6: 'Drow Ranger', 7: 'Earthshaker', 8: 'Juggernaut', 9: 'Mirana', 10: 'Morphling', 11: 'Shadow Fiend', 12: 'Phantom Lancer', 13: 'Puck', 14: 'Pudge', 15: 'Razor', 16: 'Sand King', 17: 'Storm Spirit', 18: 'Sven', 19: 'Tiny', 20: 'Vengeful Spirit', 21: 'Windranger', 22: 'Zeus', 23: 'Kunkka', 25: 'Lina', 26: 'Lion', 27: 'Shadow Shaman', 28: 'Slardar', 29: 'Tidehunter', 30: 'Witch Doctor', 31: 'Lich', 32: 'Riki', 33: 'Enigma', 34: 'Tinker', 35: 'Sniper', 36: 'Necrophos', 37: 'Warlock', 38: 'Beastmaster',39: 'Queen of Pain', 40: 'Venomancer', 41: 'Faceless Void', 42: 'Wraith King', 43: 'Death Prophet', 44: 'Phantom Assassin', 45: 'Pugna', 46: 'Templar Assassin', 47: 'Viper', 48: 'Luna', 49: 'Dragon Knight', 50: 'Dazzle', 51: 'Clockwerk', 52: 'Leshrac', 53: 'Nature\'s Prophet', 54: 'Lifestealer', 55: 'Dark Seer', 56: 'Clinkz', 57: 'Omniknight', 58: 'Enchantress', 59: 'Huskar', 60: 'Night Stalker', 61: 'Broodmother', 62: 'Bounty Hunter', 63: 'Weaver', 64: 'Jakiro', 65: 'Batrider', 66: 'Chen', 67: 'Spectre', 68: 'Ancient Apparition', 69: 'Doom', 70: 'Ursa', 71: 'Spirit Breaker', 72: 'Gyrocopter', 73: 'Alchemist', 74: 'Invoker', 75: 'Silencer', 76: 'Outworld Devourer', 77: 'Lycan', 78: 'Brewmaster', 79: 'Shadow Demon', 80: 'Lone Druid', 81: 'Chaos Knight', 82: 'Meepo', 83: 'Treant Protector', 84: 'Ogre Magi', 85: 'Undying', 86: 'Rubick', 87: 'Disruptor', 88: 'Nyx Assassin', 89: 'Naga Siren', 90: 'Keeper of the Light', 91: 'Io', 92: 'Visage', 93: 'Slark', 94: 'Medusa', 95: 'Troll Warlord', 96: 'Centaur Warrunner', 97: 'Magnus', 98: 'Timbersaw', 99: 'Bristleback', 100: 'Tusk', 101: 'Skywrath Mage', 102: 'Abaddon', 103: 'Elder Titan', 104: 'Legion Commander', 105: 'Techies', 106: 'Ember Spirit', 107: 'Earth Spirit', 108: 'Underlord', 109: 'Terrorblade', 110: 'Phoenix', 111: 'Oracle', 112: 'Winter Wyvern', 113: 'Arc Warden', 114: 'Monkey King', 119: 'Dark Willow', 120: 'Pangolier',
        121: 'Grimstroke', 129: 'Mars'}[hero[0]], hero[1]))

What a wall of text!  Also known as write-only code, or job security.

You want to write understandable code, that you can come back to in 6 months, and within 5 minutes understand enough to change it, if necessary.  This has a for-loop over a sorted list comprehension, with embedded if-then-else, of a zip of two sorted json queries, with a lambda thrown in to influence sorting.  This isn't functional programming; it's dysfunctional programming.

Let me begin with parsestats, because I can at least begin to understand it.

First, the name parsestats is itself a wall of characters.  How do I pronounce that word?  "par-sest-ats"?  Oh!  "parse stats"!  Write function names with snake_case, to aid the reader find the words in your compound name.  So this function should be called parse_stats.

In this function, you repeat hero['pickBan'] 8 times.  This is a lot of repetition.  It means the Python interpreter is looking up the same element in a dictionary repeatedly, which means the code is not going to win any speed contests.  Look up the value once, and reuse that value, instead of relooking it up over and over again.  This is awkward to do in a list comprehension, so lets use a helper function:

def parse_stats(herodata):

    def stats(hero_id, pick_ban):
        # ... TBD ...

    return [ stats(hero['heroId'], hero['pickBan']) for hero in herodata['heroes']]

A 14-line list comprehension just got turned into a 1 line list comprehension, and hero['pickBan'] got stored in the function argument pick_ban, for reuse multiple times.  I'm liking the direction this is going.

Now, what stats are being collected?  Looks like you've got 8 stats: 4 from this week, and the same 4 from the previous week.  Maybe we could compute those 4 stats in a function, passing in different selectors to grab the correct data?

def parse_stats(herodata):

    def stats(hero_id, pick_ban):

        def week_stats(pick, ban, pick_count, ban_count):

            return pick_ban[pick]['matchCount'] / pick_count * 1000,
                   pick_ban[ban]['matchCount'] / ban_count * 1000,
                   pick_ban[pick]['wins'] * 100,
                   pick_ban[ban]['wins'] * 100

        this_week = week_stats('pick', 'ban', this_week_pick, this_week_ban)
        last_week = week_stats('pickLastWeek', 'banLastWeek', last_week_pick, last_week_ban)

        return hero_id, *this_week, *last_week

    this_week_pick = herodata['matchPickCount']
    this_week_ban = herodata['matchBanCount']
    last_week_pick = herodata['matchLastWeekPickCount']
    last_week_ban = herodata['matchLastWeekBanCount']

    return [ stats(hero['heroId'], hero['pickBan']) for hero in herodata['heroes']]

Is that the clearest way?  Maybe we want to use a when selector, instead:

def parse_stats(herodata):

    def stats(hero_id, pick_ban):

        def stats_for(when):

            pick = pick_ban[f'pick{when}']
            ban = pick_ban[f'ban{when}']

            return pick['matchCount'] / herodata[f'match{when}PickCount'] * 1000,
                   ban['matchCount'] / herodata[f'match{when}BanCount'] * 1000,
                   pick['wins'] * 100,
                   ban['wins'] * 100

        this_week = stats_for('')
        last_week = stats_for('LastWeek')

        return hero_id, *this_week, *last_week

    return [ stats(hero['heroId'], hero['pickBan']) for hero in herodata['heroes']]

Which would make it easier to select 'LastMonth' stats, too.

Is a tuple really the best data structure for parse_stats to return?  The 0th element is the hero id.  What is the 2nd element?  Which element is last week's pick win percentage?  What is [1], or [3]?

How about using a namedtuple?

from collections import namedtuple

Stats = named tuple('Stats',
                    ('id',
                     'pick_rate', 'ban_rate', 'pick_wins', 'ban_wins',
                     'pick_rate_last_week', 'ban_rate_last_week', 'pick_wins_last_week', 'ban_wins_last_week'))

# ...

def parse_stats(herodata):
    def stats(hero_id, pick_ban):
        # ...
        return Stats(hero_id, *this_week, *last_week)

Or maybe make a tuple of tuples.

Stats = namedtuple('Stats', 'id, this_week, last_week')
WeekStats = namedtuple('WeekStats', 'pick_rate, ban_rate, pick_wins, ban_wins')

def parse_stats(herodata):
    def stats(hero_id, pick_ban):
        # ...
        return Stats(hero_id, WeekStats(*this_week), WeekStats(*last_week))

And you can refer to uni.id and imm.this_week.pick_rate instead of uni[0] and imm[1].

Ok, the elephant in the program:

print(f"{'Hero,':<20}{'Value':>7}")
for hero in sorted(ridiculous_list_comprehension, key=lambda tup: tup[1], reverse = True):
    print("{:<20}{:>7.2f}".format(inline_dictionary_definition[hero[0]], hero[1]))

First off, this should be a function, not code executed when Python parses the program text.  Use a if __name__ == '__main__': guard call the function if you want the code executed immediately upon parsing:

def print_mashup():
    print(f"{'Hero,':<20}{'Value':>7}")
    for hero in sorted(ridiculous_list_comprehension, key=lambda tup: tup[1], reverse = True):
        print("{:<20}{:>7.2f}".format(inline_dictionary_definition[hero[0]], hero[1]))

if __name__ == '__main__':
    print_mashup()

Of course, name the function according to what it really is doing.

for hero in sorted(...):

Uhm, no.  The output of sorted() isn't a list of hero objects.  You parsed all that hero data in parse_stats, and this is a much condensed summary of two values of those statistics, combined from two different data sets.  It is a list of two-value tuples.  Give those individual values names:

for hero_id, score in sorted(...):

I'm guess the "value" is some kind of score.  Maybe rating is a better name.  At any rate, this is way more descriptive than the hero[0] and hero[1] references made in the print statement of the body of the loop.

        print("{:<20}{:>7.2f}".format(inline_dictionary_definition[hero[0]], hero[1]))

Is that inline_dictionary_definition being constructed each and every iteration of the body of the for loop, so that one value can be extracted from the dictionary?  Be kind.  Create it once.

def print_mashup():
    print(f"{'Hero,':<20}{'Value':>7}")

    HERO_BY_ID = {1: 'Anti-Mage', 2: 'Axe', ... 129: 'Mars'}

    for hero_id, score in sorted(ridiculous_list_comprehension, key=lambda tup: tup[1], reverse = True):
        print(f"{HERO_BY_ID[hero_id]:<20}{score:>7.2f}")

As for the ridiculous_list_comprehension, I haven't a clue what "simple" is compared to "simple?rank=8", or what the calculated value is supposed to mean.  Or what uni and imm are supposed to be abbreviations for.  There are all sorts of unknowns.  Do the json queries return exactly the same hero_id sets?  Can there be an extra one in either of the sets, in which case sorting and zipping doesn't guarantee the data matches up properly!

I'm happy that you're happy to have written a program that you will personally use.  I'd be happier if someone else could understand it, and use it too.


Average time to first answer Just trying out my T-SQL foo.

Wrote a script to get the average time (in seconds) to first answer for a specified language.

-- Could not get this so that the user entered
-- the language in a box at the bottom working.
-- so you have to edit the code here.
declare @Language nvarchar(20) = 'php' -- ##LanguageTag##

declare @AnswerTime table (Id int, CreationDate date, AnswerTime date, Elapse bigint, Slot int )

insert @AnswerTime
Select Q.Id,
       max(Q.CreationDate) AS CreateTime,
       min(A.CreationDate) AS AnswerTime,
       DATEDIFF(second, max(Q.CreationDate), min(A.CreationDate)) AS Elapse,
       Year(max(Q.CreationDate))*12+Month(max(Q.CreationDate)) AS Slot
From   Posts Q, Posts A
Where  Q.PostTypeId = 1
  and  Q.Id = A.ParentId
  and  CHARINDEX(@Language, Q.Tags) != 0
Group by Q.Id, Q.Tags
Order by Elapse

Select min(CreationDate),
       count(*)         as [Count of Tickets],
       -- min(Elapse)   as [Min Time to Answer],
       -- max(Elapse)   as [Max Time to Answer],
       avg(Elapse)    as [Average time to first Answer]
From   @AnswerTime
Group by Slot

Here it is running.

There are a few things that are off in this query. Going through them in some sense of order:

I am still a fan of 'old style' joins, but, the CTE concept is a real winner in SQL Server, so, instead of creating the table variable, just use the CTE.
I can tell that your code went through some iterations, and as a result, you have some 'cruft' that can be disposed of. Delete those things that you don't select (like you select min(A.CreationDate) as AnswerTime which is never used.
The SEDE tables take some getting used to. You have done a CharIndex on the tags column. That's fine, but, your example php also pulls in php5 tags, as well as cakephp. To get around this, you should be aware that SEDE stores the tag names in <...> braces in the Tags field, so searching for <php> is what you want.

Using the CharIndex is OK, but more standard will be using like, as in: ... and Tags like '%<php>%'
I have actally given up on using the Tags field completely, and instead I do the join to the PostTags table which allows you to do the join natively without the like/charindex.
There is no need for the aggregate column on the Question's CreationDate. The max(Q.CreationDate) is redundant (there will only be one CreationDate per Question ID).
You should always use as many constants as you can when doing SQL Queries (at least as much as you can, until you find a performance reason to not overdo it....), so you should also add the A.PostTypeId = 2 even though its ParentID is the Q's ID.
I did not like the way you were 'Slotting' the data in to months. I find that subtracting the days-in-month-less-one from the given date brings the date back to the first-of-the-month, and then truncating the time, allows you to have all dates represented by exactly the first-of-the-month:

Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate)) as CreationMonth

Getting the Variable substitution to work was easier once I used the Tags table too. The stupid 'Unexpected error, blame has already been assigned' error is a PITA, but it is now gone.
Finally, the scale of the Y axis was so huge, that the count of the questions was not visibly appealing/apparent. By changing the scale from Seconds to Hours, the scales are better.

I forked your query here, and this is the SQL (I have tried to keep the capitalization and other style conventions consistent with your code):

declare @Language nvarchar(25) = ##LanguageTag:string##;
declare @tagid int;

select @tagid = Id
from Tags
where TagName = @Language

print @Language + '->' + Convert(NVarchar(max), @tagid);

with AnswerTime as (
 Select Q.Id,
       Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate)) as CreationMonth,
       Convert(float, DATEDIFF(second, Q.CreationDate, min(A.CreationDate))) AS Elapsed
 From   PostTags T,
        Posts Q,
        Posts A
 Where  T.TagId = @tagid
  and  T.PostId = Q.Id
  and  Q.PostTypeId = 1
  and  A.PostTypeId = 2
  and  Q.ClosedDate is null
  and  Q.Id = A.ParentId
 Group by Q.Id,
          Q.CreationDate,
          Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate))
)
Select CreationMonth,
       count(*)         as [Count of Tickets],
       avg(Elapsed)/3600.0 as [Average hours to first Answer]
From   AnswerTime
Group by CreationMonth

Edit/Update

I ran the query on Stack Overflow, and it fails because there are so many questions that the avg(Elapsed) fails with an arithmetic overflow. I have updated the query to only consider the past 68 years worth of data... (in case there are large date differences in seconds), and also to convert the elapsed in to a float (which does not overflow on avg())


MVC4 Unit Of Work with Entity FrameWork I am designing a new asp.net MVC4 application. I know that context class generated by EF is Unit of work and Dbset is the repository class. I am just avoiding the extra abstraction. I want to know whether I am following the correct pattern or not.

Entity

  public partial class ProductMaster
  {
    public long ID { get; set; }
    public string ProductCode { get; set; }
    public string ProductName { get; set; }

  }
  public partial class CompanyMaster
  {
    public long ID{ get; set; }
    public string CompanyName { get; set; }
    public string CompanyCode { get; set; }
  }

Context class(Generated By EF)

public partial class SampleEntities : DbContext,IUnitOfWork
{
    public SampleEntities()
        : base("name=SampleEntities")
    {
    }
    public void Commit()
    {
       SaveChanges();
    }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        throw new UnintentionalCodeFirstException();
    }

    public DbSet<ProductMaster> ProductMaster { get; set; }
    public DbSet<CompanyMaster> CompanyMaster { get; set; }
    public DbSet<UserMaster> UserMaster{ get; set; }

    public virtual ObjectResult<SP_UserMaster_Result> SP_UserMaster(Nullable<long> tM_UM_ID, string userName, string userPassword, Nullable<byte> userType)
    {
        var tM_UM_IDParameter = tM_UM_ID.HasValue ?
            new ObjectParameter("TM_UM_ID", tM_UM_ID) :
            new ObjectParameter("TM_UM_ID", typeof(long));

        var userNameParameter = userName != null ?
            new ObjectParameter("UserName", userName) :
            new ObjectParameter("UserName", typeof(string));

        var userPasswordParameter = userPassword != null ?
            new ObjectParameter("UserPassword", userPassword) :
            new ObjectParameter("UserPassword", typeof(string));

        var userTypeParameter = userType.HasValue ?
            new ObjectParameter("UserType", userType) :
            new ObjectParameter("UserType", typeof(byte));

        return ((IObjectContextAdapter)this).ObjectContext.ExecuteFunction<SP_UserMaster_Result>("SP_UserMaster", tM_UM_IDParameter, userNameParameter, userPasswordParameter, userTypeParameter);
    }
}

Unit of Work Interface

public Interface IUnitOfWork
{
    public DbSet<ProductMaster> ProductMaster;
    public DbSet<CompanyMaster> CompanyMaster;
    public Dbset<UserMaster> UserMaster;
    public virtual ObjectResult SP_UserMaster(Nullable<long> tM_UM_ID, string userName, string userPassword, Nullable<byte> userType){};
    public void Commit();
}

Business Logic(Implementation)

 public class ProductMaster
 {
    IUnitOfWork _unitOfWork;
    public ProductMaster(IunitOfWork unitOfWork)//Injected using Ninject
    {
        this._unitOfWork = unitOfWork;
    }
    public void AddProduct(Product product)
    {
        _unitofWork.ProductMaster.Add(product);
        _unitOfWork.Commit();
    }
 }

Interface of Product Logic

 public class IProductMaster
  {
    public ProductMaster(IunitOfWork unitOfWork);
  }

Product Controller

  public class ProductController : Controller
  {
     IProductMaster _productMaster;
     public ProductController(IproductMaster productMaster)
     {
       this._ProductMaster=productMaster;//Injecting using Ninject
     }
     public void SaveProduct(ProductMaster product)
     {
        this._productMaster.AddProduct(product.ToEntity());
     }
  }

I think the problem is you’re starting with an implementation (Entity Framework) and then trying to bind this to two common design patterns – Unit Of Work and Repository.  The Entity Framework handles these two patterns and combines them together in Microsoft’s own implementation – they couldn’t actually work independently, for example you couldn’t just take a DbSet (Repository) and get a Product (entity) without instantiating the context (Unit Of Work).

These design patterns exist so they can be easily replaced within your application without changing much code, but if your IUnitOfWork is returning a DbSet<T> repository and you decide to read your Product entities from a local file instead of a database then you’re not really going to be able to implement this with your current abstraction.

IUnitOfWork should be strongly-named to the domain context – this is actually containing names of repositories which are specific to the domain context i.e. ProductMaster.  You could have IShopUnitOfWork derive from IUnitOfWork (which only has Commit as this is reusable in different domain contexts)
Your IUnitOfWork will get progressively bigger as you add new repositories and stored procedure definitions and I fear you’ll break the Interface Segregation Principle because of this.
I believe your repositories should actually contain the “query” calls specific for an aggregate root entity rather than your Unit Of Work – by putting it in the Unit Of Work you’re giving it two responsibilities:

i.  Maintaining the register of objects to be persisted

ii. Knowing how to authorise a user (in your example) along with all of the other stored procedure calls you’ll have

Therefore this will violate the Single Responsibility Principle

Furthermore SP_UserMaster should probably exist in the User repository as its specific to that domain entity.  Again this is strongly-named to imply a database call with a stored procedure – what if this was to be changed to a web service call?

With these caveats in mind it’s going to be hard to change your implementation to work this way with Entity Framework as Microsoft has taken the design patterns and implemented it in their own way with Unit Of Work and Repository mingled together - I'm not saying this is wrong its just the way they've decided to do it.  The main benefit of having these abstractions in this context is the benefit of being able to create stubs in unit testing so you’ll be mocking stored procedure and query calls without hitting the database.


MVC Repository Insert Using async This is practically my first time using async in my code. I read about dos-and-donts but I wanted to get some feedback on what I've written.

I'm using a repository pattern and I'm inserting asynchronously into the database.

An explanation of the models involved: I have one DomainModel and three tblModels (tblModel1, tblModel2, tblModel3). tblModel2 and tblModel3 hold an ID from tblModel1. The DomainModel is made up of all three of the tblModels. I'm simplifying the domain to database mapping here because I'm focusing on feedback about the asynchronous calls.

Repository

public async Task<int> InsertAsync(tblModel1 tm1)
{
    tm1 = db.tblModel1.Add(tm1);
    await db.SaveChangesAsync();
    return tm1.ID;
}

public async Task<int> InsertAsync(tblModel2 tm2)
{
    tm2 = db.tblModel2.Add(tm2);
    await db.SaveChangesAsync();
    return tm2.ID;
}

public async Task<int> InsertAsync(tblModel3 tm3)
{
    tm3 = db.tblModel3.Add(tm3);
    await db.SaveChangesAsync();
    return tm3.ID;
}

Manager

public async Task<bool> InsertAsync(DomainModel M)
{
    try
    {
        await Task.Run(async () => {
            var tm1ID = await repo.InsertAsync(M.tm1);
            var tm2ID = await repo.InsertAsync(AddID(M.tm2,tm1ID));
            var tm3ID = await repo.InsertAsync(AddID(M.tm3,tm1ID));
        });
    }
    catch (Exception ex)
    {
        return false;
    }

    return true;
}

The method AddID(tblModel, int) is to get the point across that I'm adding the ID from tblModel1 after it's inserted into tblModel2 and tblModel3. In actuality, I do this but I map the DomainModel to completely new tblModels and during that process I add the IDs to tblModel2 and tblModel3. I believe the code above is representative of what I'm doing.

Please let me know if there is a better way for me to optimize these asynchronous inserts into the database. Thank you!

The best thing you could do is let EF do what it's good at... You don't need to save every single time you add an entity - let EF track the changes in its DbSets repositories and then save the changes in the
DbContext unit of work.

As a more general point - why aren't you saving the whole DomainModel at once? I'd expect to just be able to do:

context.DomainModels.Add(someDomainModel);
await context.SaveChanges();

I've hinted at it but I'll say it outright - you get very little to no benefit in writing Repositorys wrapping the db context. It's just not worth it.

I'm not going to comment on the bad naming because I'm guessing that this is actually just an example of your working code.


Refactoring Into Generic Methods This might be a loaded question, but here goes.  I have some code that is very similar between all the methods.  I am trying to find a way to accomplish all methods with a generic method/function. I'm not sure if I should be using delegates, and if so how to implement in this specific situation, or if there is a much more simple way to make a generic method.

Below I have copied the code I am trying to make generic.  All methods take this basic syntax: (Parameters) ConcurrentQueue(pPeople),  With that collection, the first part gets the count of the records.  Each method iterates over a different part of the collection (or collection in the collection).

Thanks for any help or nudges in the right direction!  If you have an idea that would help, it would be great if you could give an example related to this code (or very similar).

Code:  I have copied three methods that give that show similar code (there are many more).

    private void UpdatepPeopleAddressIDs(ConcurrentQueue<pPeople> people)
    {
        //Get Count of records that need an ID from uMaxIDs
        int updateCount = 0; object lockObject = new object();
        Parallel.ForEach<pPeople>(people, person =>
        {
            person.myPeopleAddressList.ForEach(address =>
            {
                if (address.ID == 0)
                {
                    lock (lockObject)
                    {
                        updateCount++;
                    }
                }
            });
        });
    }

    private void UpdatepPeopleContactNumbersIDs(ConcurrentQueue<pPeople> people)
    {
        //Get Count of records that need an ID from uMaxIDs
        int updateCount = 0; object lockObject = new object();
        Parallel.ForEach<pPeople>(people, person =>
        {
            person.myPeopleContactsList.ForEach(contact =>
            {
                if (contact.ID == 0)
                {
                    lock (lockObject)
                    {
                        updateCount++;
                    }
                }
            });
        });
    }

    private void UpdatepPeopleEmailIDs(ConcurrentQueue<pPeople> people)
    {
        //Get Count of records that need an ID from uMaxIDs
        int updateCount = 0; object lockObject = new object();
        Parallel.ForEach<pPeople>(people, person =>
        {
            person.myPeopleEmailsList.ForEach(emails =>
            {
                if (emails.ID == 0)
                {
                    lock (lockObject)
                    {
                        updateCount++;
                    }
                }
            });
        });
    }

This is fairly easy to achieve. You need to make use of callback, which can perform the functionality specific to the method, which your generic method then uses. Something like this:

private void UpdatepPeople<T>(ConcurrentQueue<pPeople> people,
                              Func<PersonClass, List<T>> listProvider,
                              Func<T, bool> updateRequiredTest)
{
    var updateCount = 0;
    var lockObject = new object();

    Parallel.ForEach<pPeople>(people, person =>
    {
        listProvider(person).ForEach(item =>
        {
            if (updateRequiredTest(item))
            {
                lock (lockObject)
                {
                    updateCount++;
                }
            }
        });
    });
}

private void UpdatepPeopleContactNumbersIDs(ConcurrentQueue<pPeople> people)
{
    UpdatepPeople<ContactType>(people,
                               person => person.myPeopleContactsList,
                               contact => contact.ID == 0);
}

private void UpdatepPeopleEmailIDs(ConcurrentQueue<pPeople> people)
{
    UpdatepPeople<EmailType>(people,
                               person => person.myPeopleEmailsList,
                               contact => emails.ID == 0);
}


Using model view presenter (MVP) in C# code I'm a beginner to Model-View-Presenter pattern and I'm finding a way to use it in a sample application

In my C# winforms application has a Employee Class and it has properties like EmployeeID,Name, Address, Designation etc. Also it has behaviors like viewEmployee(), AddNewEmployee(), PromoteEmployee() etc.

Could you please suggest how these things are organized in MVP pattern?
I have several entity classes in my project like Employee, Client, Attendance, Salary etc. So should I make separate DataService and Presenter classes for each one?

My current understanding is like this...

Implementing ViewEmployee()----------

Class Employee
{
    public string EmployeeID { get; set; }
    public string Name { get; set; }
    public string Address { get; set; }
    public string Designation { get; set; }
}

Class EmployeePresenter
{
   public void ViewEmployee(string employeeID, frmEmployeeDetails _form)
   {
     Employee Emp= EmloyeeDataService.GetEmployeeData(employeeID);
     _form.txtName=Emp.Name;
     _form.txtAddress=Emp.Address;
     _form.txtDesignation=Emp.Designation;
   }
}

Class EmployeeDataService
{
    public static Employee GetEmployeeData(String employeeID)
    {
      //Get data from database
      // Set Properties of Employee
      //Return Employee
    }
}

Partial Class frmEmployeeDetails : Form
{
    btnViewEmployee_Click() //Button Event
    {
      EmployeePresenter.ViewEmployee(txtEmployeeID.text,this);
    }
}

I think your question would be better if you fleshed it up with your actual working code, there's a lot of "placeholder" code in your post, which makes it border the line of off-topicness.
You may be able to find more information and high-level design guidance about MVP on Programmers.SE.

There isn't a lot to review here, but this is jumping at me:
public static Employee GetEmployeeData(String employeeID)

And then:
Employee Emp = EmloyeeDataService.GetEmployeeData(employeeID);

By doing that, you have tightly coupled the EmployeePresenter class to the EmployeeDataService class. The static method makes the ViewEmployee method harder to unit test - now whatever you do, if you call ViewEmployee you're going to call EmloyeeDataService.GetEmployeeData.
If you code to an abstraction instead of an implementation:
public interface IEmployeeRepository
{
    Employee GetById(string employeeId);
}

public class EmployeeDataService : IEmployeeRepository
{
    public Employee GetById(string employeeId)
    {
        // do whatever it takes to return an Employee object.
    }
}

Here you're accessing an SQL database with ADO.NET, but then later you might want to implement the same interface with Linq-to-SQL, or with Entity Framework - actually, the data could just as well be in XML files on the file system, or on Azure (cloud), or retrieved with some web service - all you need to care is the object's interface, which takes an employeeId and returns an Employee. Exactly how that's done, is irrelevant to the Presenter:
class EmployeePresenter
{
    private readonly IEmployeeRepository _service;

    public EmployeePresenter(IEmployeeRepository service)
    {
        _service = service;
    }
}

Now whoever is instantiating an EmployeePresenter will also need to instantiate some implementation of IEmployeeRepository, and inject that implementation into the presenter's constructor; you're no longer strongly coupled with a specific implementation, and you can change that anytime, without rewriting anything in the EmployeePresenter code.

This is more about SOLID and DI/IoC than MVP. I suggest you look at this read from Martin Fowler about GUI architectures, there's a section on MVP.
However I can tell you that this isn't MVP:
partial class frmEmployeeDetails : Form
{
    btnViewEmployee_Click() //Button Event
    {
        EmployeePresenter.ViewEmployee(txtEmployeeID.text,this);
    }
}

The Form shouldn't know anything about a presenter, or any repository or service - it's just a view. Instead, you fire events that the presenter can listen to, and respond. Shortly put, the Hollywood Principle: don't call them, they'll call you.


Optimize calculation for the amount of time between when a value changes in Pandas I have some sensor data that contains timestamps from when a machine is turned on and an indicator variable showing whether or not the machine is actively running.  The data is mostly recorded every two minutes, but they there could be more or less time between two sensor readings due to issues with the sensor.

I want to create a new feature that captures at a specific datetime, how long has the machine been in that Run status for.

I currently have the data loaded in a DataFrame, and I have been able to create a new variable that shows whether or not the Run status has changed from the previous reading.  Next, I need to figure out how capture the amount of time since the last time the Run status changed.  If the readings where consistently every two minutes, I could maybe do some type of counter that resets when Run changes.  Instead, I took an approach where if the time changed, I hold onto that value and keep subtracting the next Datetime stamp from it, as long as Run didn't change. When Run changes, I start the processes over.

This approach seems to work fine on this small example, but I feel like it's not the most efficient use of resources, especially when trying to scale up to my 400,000 record dataset.

# load example data
df = pd.DataFrame(data = [['2015-01-01 00:00', 1],
                          ['2015-01-01 00:02', 1],
                          ['2015-01-01 00:04', 1],
                          ['2015-01-01 00:06', 0],
                          ['2015-01-01 00:08', 0],
                          ['2015-01-01 00:10', 1],
                          ['2015-01-01 00:12', 0],
                          ['2015-01-01 00:15', 1],
                          ['2015-01-01 00:17', 1],
                          ['2015-01-01 00:19', 1],
                          ['2015-01-01 00:23', 0],
                          ['2015-01-01 00:25', 0],
                          ['2015-01-01 00:30', 0],
                          ['2015-01-01 00:32', 0],
                          ['2015-01-01 00:34', 0]],
                  columns = ['Datetime', 'Run'])

# convert to datetime object
df.Datetime = pd.to_datetime(df['Datetime'])

# create an empty column to capture change point in Run
df['Run_Change'] = ""

# set the first Run_Change equal to 'Change', since we don't know what happened before it
df = df.set_value(df.index[0], 'Run_Change', 'Change')

# create a column to capture the amount of time between each Run_Change
df['Time_Since_Change'] = df['Datetime'] - df['Datetime']

# set the first time_since_change to 0 since we don't know what happened before it
first_change = df.ix[df.index[0]]['Datetime'] - df.ix[df.index[0]]['Datetime']
df = df.set_value(df.index[0], 'Time_Since_Change', first_change)

# set iniital datetime to based changes on
change_time = df.ix[df.index[0]]['Datetime']

# starting at the second event...
for i in df.index[1:]:

    # ...compare Run at this time to Run at the previous time...and if it changeed...
    if df.ix[i]['Run'] != df.ix[i-1]['Run']:

        # ...set Status equal to 'Change'
        df.set_value(i, 'Run_Change', 'Change')

        # ...and grab the 'Datetime' that the change happeneed
        change_time = df.ix[i]['Datetime']

    # otherwise...
    else:

        #...set equal to 'No Change'
        df.set_value(i, 'Run_Change', 'No Change')

    # calculate the time since the last change
    time_since_change = df.ix[i]['Datetime'] - change_time

    # update DF
    df.set_value(i, 'Time_Since_Change', time_since_change)

# convert Time_Since_Change to minutes
df['Time_Since_Change_2'] = df['Time_Since_Change'] / np.timedelta64(1, 'm')

Cool problem, thanks for sharing.  Here is my take on the implementation. I will discuss each section in turn.

1. Use diff to directly get the state changes

diff() returns a vector with the difference between subsequent elements.  Thus when the state does not change it is equal to zero.

# find when the state changes
run_change = df['Run'].diff()

2. Use diff to directly get time delta between steps

# get the step lengths
step_length = df['Datetime'].diff()

3. Loop and build delta since previous change

Since we need to keep state from previous rows I could not formulate a vector way to do this, and had to resort to a loop.  But in the loop we do not use any indices, instead we iterate directly on the data structure.  zip() is used to take matching index elements of the two series at the same time.

# loop and get the change since last state change
since_change = []
current_delta = 0
for is_change, delta in zip(run_change, step_length):
    current_delta = 0 if is_change != 0 else \
        current_delta + delta.total_seconds() / 60.0
    since_change.append(current_delta)

4. Insert the intermediate data into the data frame

This is just for illustration.  Likely more efficient to use concat, than three separate dataframe constructions.

# add this data to the data frame
df['Run_Change'] = run_change
df['Step_Length'] = step_length
df['Time_Since_Change'] = pd.Series(since_change).values

Whole Code Listing:

import pandas as pd

# load example data
df = pd.DataFrame(data=[['2015-01-01 00:00', 1],
                        ['2015-01-01 00:02', 1],
                        ['2015-01-01 00:04', 1],
                        ['2015-01-01 00:06', 0],
                        ['2015-01-01 00:08', 0],
                        ['2015-01-01 00:10', 1],
                        ['2015-01-01 00:12', 0],
                        ['2015-01-01 00:15', 1],
                        ['2015-01-01 00:17', 1],
                        ['2015-01-01 00:19', 1],
                        ['2015-01-01 00:23', 0],
                        ['2015-01-01 00:25', 0],
                        ['2015-01-01 00:30', 0],
                        ['2015-01-01 00:32', 0],
                        ['2015-01-01 00:34', 0]],
                  columns=['Datetime', 'Run'])

# convert to datetime object
df.Datetime = pd.to_datetime(df['Datetime'])

# find when the state changes
run_change = df['Run'].diff()

# get the step lengths
step_length = df['Datetime'].diff()

# loop and get the change since last state change
since_change = []
current_delta = 0
for is_change, delta in zip(run_change, step_length):
    current_delta = 0 if is_change != 0 else \
        current_delta + delta.total_seconds() / 60.0
    since_change.append(current_delta)

# add this data to the data frame
df['Run_Change'] = run_change
df['Step_Length'] = step_length
df['Time_Since_Change'] = pd.Series(since_change).values


Looking for improvements on basic dice roll code (Python 3) I've created a code based upon a dice roll where the number of rolls is decided by user input. I am new to this, so any and all feedback will be appreciated as I feel the code is quite clunky at best.

number_of_times = int(input("How many times do you want to roll the dice?: "))
print(number_of_times)

def dice_roll(x):
    import random
    for r in range(x):
        roll = random.randint(1,20)
        print(roll)

dice_roll(number_of_times)

You should have imports at the top of your code. Having them in the middle of code makes it needlessly confusing with no benefit.
You code doesn't account for the possibility of a user not entering an integer as input. You can utilize a while, try and except ValueError to allow a user to attempt to re-enter a value.
You should keep code out of the global scope. Wrapping your code in a main function will help prevent future mishaps.
You should really use a better name than x, amount is descriptive.
Rather than r you can use _ or __ to denote that you are throwing away the value. This is no different that what you have done now, but it is a custom to help others understand your code quickly.

import random

def roll_dice(amount):
    for _ in range(amount):
        print(random.randint(1, 20))

def main():
    while True:
        try:
            amount = int(input("How many times do you want to roll the dice? "))
            break
        except ValueError:
            print("That is not a valid whole number.")
    print(amount)
    roll_dice(amount)

main()

There are some more advanced things you can do too.

You can use an if __name__ == "__main__": guard to help prevent your main code from running accidentally. If it is imported by accident from another script.
It would be better for you to return the values of the dice rather than print. This allows you to reuse the function later if you need to use the results of the thrown dice.
You can use random.choices to select k amount of values.
Print is slow. You may want to change to use "\n".join to reduce the overhead from calling print multiple times. However you need to convert all the values to strings.
To easily convert each value in a list to a string you can use a for loop, building a new list.

output = []
for item in roll_dice(amount):
    output.append(str(item))

This contains a lot of noise, so we can use a list comprehension to simplify this.

output = [str(die) for die in roll_dice(amount)]

import random

def roll_dice(amount):
    return random.choices(range(1, 21), k=amount)

def main():
    while True:
        try:
            amount = int(input("How many times do you want to roll the dice? "))
            break
        except ValueError:
            print("That is not a valid whole number.")
    print(amount)
    print("\n".join([str(die) for die in roll_dice(amount)]))

if __name__ == "__main__":
    main()

Note: Personally I don't think using the list comprehension and str.join to be as nice as a for loop when it comes to non-performance critical code. This is to show you how you can improve future code with the code you have today.