Implementation of a vector of pairs In a personal project I was working on, I had a vector of std::vector< std::pair<unsigned,char>>'s that represented a character and its position in the alphabet. I found that using/manipulating that vector to be clumsy so I wrote this implementation. Essentially, it just wraps the usual vector operations. I'm hoping for the following things to be considered, as well as the usual areas of review:

Are there any other operations that should be implemented?
I didn't have a C++20 compiler available at the time of writing this, what needs to be done to "modernize" the code?
Are there specific ways to change or improve the code?

And a few notices

I know that begin() and end() should return a std::vector::iterator, but I was having difficulties making that compile so I kept it as auto for the time begin. (Yes,I know that is a dirty hack. It's on my to do list.
There are other operators that need to be overloaded ( = and == especially), but I wanted to improve the existing code before adding more.

The code is licensed under the BSD Clause 3 license, if you wish to use it/change it.

#include <iostream>
#include <vector>

template< typename Left, typename Right>
class Vecpair
{
  public:

    auto begin(void);
    auto end(void);

    void pushBack(Left,Right);
    void pushBack( std::pair<Left,Right> );
    void popBack(void);
    void clear(void);

    std::size_t getSize(void);
    std::pair<Left,Right> atPos(unsigned);
    std::pair<Left,RIght> front(void);
    std::pair<Left,Right> back(void):

    std:::pair<Left,Right> operator [] (unsigned i){ return( internal.at(i) ); }

  private:

    std::size_t numElem = 0;
    std::vector< std::pair<Left,Right>> internal;
};

//  Returns an iterator to the beginning of 'internal'.
template< typename Left, typename Right>
auto Vecpair<Left,Right>::begin(void)
{
  return( internal.begin() );
}

//  Returns an iterator to the end of 'internal'.
template< typename Left, typename Right>
auto Vecpair<Left,Right>::end(void)
{
  return( internal.end() );
}

// Takes two separate values and pushes them to the back of 'internal' as a pair.
template< typename Left, typename Right>
void Vecpair<Left,Right>::pushBack(Left l, Right r)
{
  internal.push_back( {l,r} );
  numElem++;
}

//  Takes a pair and pushes to the back of 'internal'.
template< typename Left, typename Right>
void Vecpair<Left,Right>::pushBack( std::pair<Left,Right> p )
{
  internal.push_back( p );
  numElem++;
}

//  Pops the last element.
template< typename Left, typename Right>
void Vecpair<Left,Right>::popBack(void);
{
  internal.pop_back();
  numElem--;
}

//  Clears 'internal'.
template< typename Left, typename Right>
void Vecpair<Left,Right>::clear(void)
{
  internal.clear();
  numElem = 0;
}

// Returns the number of elements;
template< typename Left, typename Right>
std::size_t Vecpair<Left,Right>::getSize(void)
{
  return( numElem );
}

//  Returns the pair at the given position.
template< typename Left, typename Right>
std::pair<Left, Right> Vecpair<Left,Right>::atPos(unsigned idx)
{
  return( internal.at(idx) );
}

//  Returns the first pair.
template< typename Left, typename Right>
std::pair<Left,Right> Vecpair<Left,Right>::front(void)
{
  return( internal.front() );
}

//  Returns the last pair.
template<typename Left, Typename Right>
std::pair<Left,Right> Vecpair<Left,Right>::back(void)
{
  return( internal.back() );
}

I believe you had to put typename before std::vector<...>::iterator to make it compile. iterator is a dependent name, thus compiler needs a hint (well, in reality it doesn't need it in context of type alias declaration, it is just how syntax is defined before C++20).

Interface

Lets address the elephant in the room first. This class trims down the features of std::vector<std::pair<T, U>> without any visible benefit. Things like std::back_inserter will not work because members are named with different name, comparison operators will not work, there are no type aliases inside the class so users will have to write template metaprogramming for simple things, and so on.

I do not see any benefit of using this instead of std::vector<std::pair<T, U>>.

Implementation

There is no need to have void in the parameter list, empty parameter list means the functions doesn't take any parameters.

numElem looks redundant.

Access functions should be const, possibly noexcept.

Some functions are defined inline, some are not, although they are of the same complexity. This might lead to nasty linker issues.

Putting return value in () might get problematic. When using things like decltype, that will actually alter the result.

Designing interfaces

Wrappers are usually made to streamline one way of usage, possibly without losing any expressive power over original. I believe the main idea was to provide a two-way mapping from two continuous ranges. When designing such wrappers, it is important to understand under which constraints the wrappers are going to work. Let me throw some from top of my head:

The input will be in the form {index-like, index-like}
Two ranges are continuous (from 1 to 26, from 'a' to 'z' given ASCII)
It is possible to normalize the values (e.g. substract a value to make the first one 0), for example for letters it will be substract the value of 'a'
No values outside of the given ranges are ever to be queried
The mapping is one-to-one and onto, e.g. for each value on one side, there is corresponding value on the other side, and it is unique.

If the (1) and/or (2) don't hold. one will need to consider std::map and siblings.

The usage scenarios are as following:

User can somehow read/provide the mapping in the form of a range of std::pair<T, U> or as two separate ranges
User wants to query both ways, e.g. mapping[right] would return an object of Left type, and mapping[left] will return an object of Right type: mapping[3] == 'c' && mapping['c'] == 3.
User might want to know if two mapping are equal, e.g. operator== and operator!= should be provided.
Users might want to define some relationships after the object was created, thus insertion functions like define(const Left& left, const Right& right) are needed. Perfect forwarding might be needed.
Users do not modify the relationships (the pair itself) after they are defined
Users might want to remove a relationship

With those, I believe something like this could fit in:

template <typename Left, typename Right>
class mapping
{
    // internals
public:
    template <typename InputIterator>
    mapping(InputIterator first, InputIterator last); //extract mappings from iteator range

    // C++20 range version here
    // ditto for version where mappings provided as two ranges

    // rule of 0/3/5 depending on the implementation

    const Right& operator[](const Left& left) const; // no need for non-const version
    const Left& operator[](const Right& right) const;

    // returns false if either side is already in some relationship
    bool define(const Left& left, const Right& right) noexcept;
    // overload for `std::pair` version

    // throws f either side is already in some relationship
    void try_define(const Left& left, const Right& right);
    // overload for `std::pair` version

    // returns true if the relationship existed and now removed
    bool remove(const Left& left, const Right& right) noexcept(destructor(Left) && destructor(Right)); //pseudocode for noexcept
    // pair version
    // throwing version

    template <typename Left, typename Right>
    friend bool operator==(const mapping<Left, Right>& lhs, const mapping<Left, Right>& rhs);

    template <typename Left, typename Right>
    friend bool operator!=(const mapping<Left, Right>& lhs, const mapping<Left, Right>& rhs);

    template <typename Left, typename Right>
    friend void swap(mapping<Left, Right>& lhs, mapping<Left, Right>& rhs);
};

There are other things one might need, since those are basically sets, operations like

Merge
Difference
Intersection

might be needed for the user.


Convert Roman to Int I made a function that converts Roman letters to integers. But I'm not happy with it, as I feel like I used too much code for a such a simple task. Any suggestion on how I can minimize it?

values = (('M',  1000),
         ('CM', 900),
         ('D',  500),
         ('CD', 400),
         ('C',  100),
         ('XC', 90),
         ('L',  50),
         ('XL', 40),
         ('X',  10),
         ('IX', 9),
         ('V',  5),
         ('IV', 4),
         ('I',  1))

def RomanToInt(year):
    result = 0
    boom = []
    for i in range(len(year)):
        for letter, value in values:
            if year[i] == letter:
                boom.append(value)
    boom.append(0)
    for i in range(len(year)):
        if boom[i] >= boom[i+1]:
            result = result + boom[i]
        else:
            result = result - boom[i]
    return result

A few notes:

A tuple of two-tuples is an odd structure for values, if you used a dictionary {'M': 1000, ...} you could avoid the loop over values and just use boom.append(values[year[i]]);
There is no point including e.g. CM in values, as there is no way year[i] (a single character) would ever be equal to those letters;
values should be defined inside the function, perhaps as a default argument, as it's not needed in any other scope;
for i in range(len(year)): ... year[i] is generally unpythonic, use for letter in year: instead (where you need i and i+1, you can use zip(l, l[1:]) to get pairs);
boom is not a very good name for the list, as it doesn't really explain what should be in it (something like numbers might be better) and there's no need to limit yourself to year inputs; and
Per the style guide, the function should be called roman_to_int, and you should consider adding a docstring.

With the above fixes:

def roman_to_int(roman, values={'M': 1000, 'D': 500, 'C': 100, 'L': 50,
                                'X': 10, 'V': 5, 'I': 1}):
    """Convert from Roman numerals to an integer."""
    numbers = []
    for char in roman:
        numbers.append(values[char])
    total = 0
    for num1, num2 in zip(numbers, numbers[1:]):
        if num1 >= num2:
            total += num1
        else:
            total -= num1
    return total + num2

You could also consider simplifying with a list comprehension, and adding error handling for the following cases:

roman is an empty string ""; and
roman contains characters not in values.


Modify a method on the fly in Ruby I have about a hundred Rake tasks that are creating Rails models for me from a legacy datafile. Each task shares some setup and teardown code; all that sets them apart is the code in the middle. For example:

task countries: setup do
  rows = rows_from_file("system/countries", 2)
  bar = RakeProgressbar.new(rows.length)
  info "» Importing countries"
  rows.each do |row|
    Country.new(
      alpha2: row[0],
      name: row[1]
    ).save!
    bar.inc
  end
  bar.finished
end

task telephone_number_types: setup do
  rows = rows_from_file("system/telephone_number_types", 1)
  bar = RakeProgressbar.new(rows.length)
  info "» Importing telephone number types"
  rows.each do |row|
    TelephoneNumberType.new(
      name: row[0]
    ).save!
    bar.inc
  end
  bar.finished
end

task occupations: setup do
  rows = rows_from_file("system/occupations", 3)
  bar = RakeProgressbar.new(rows.length)
  info "» Importing occupations"
  rows.each do |row|
    Occupation.new(
      id: row[1],
      name: row[2]
    ).save!
    bar.inc
  end
  bar.finished
end

How could I extract the progress bar stuff so I don’t keep repeating it? I had thought of writing some generic method that takes the array of rows and some code block (which would contain the Model.new(attrs).save! code) and wraps that stuff with the progress bar stuff, but I can’t quite make it work.

I'd say you're on the right track with you idea to extract a method.

While I can't fully replicate your code and its context, I think something like this should work:

task countries: setup do |task_name|
  rows = rows_from_file("system/#{task_name.to_s}", 2)
  process_rows(task_name, rows) do |row|
    { alpha2: row[0], name: row[1] }
  end
end

# ...

def import_rows(task_name, rows, &block)
  klass = task_name.to_s.classify
  bar = RakeProgressbar.new(rows.length)
  info "» Importing #{task_name.to_s.humanize}"
  rows.each do |row|
    attrs = block.call(row)
    klass.new(attrs).save!
    bar.inc
  end
  bar.finished
end

But it's going pretty far in its assumptions - it's basing a lot off of the task name alone.

A somewhat less fragile approach might be this where the correct class is explicitly passed to the helper method and the file name isn't guessed from the task name

task countries: setup do
  rows = rows_from_file("system/countries", 2)
  process_rows(Country, rows) do |row|
    { alpha2: row[0], name: row[1] }
  end
end

# ...

def import_rows(klass, rows, &block)
  bar = RakeProgressbar.new(rows.length)
  info "» Importing #{klass.to_s.pluralize.humanize}"
  rows.each do |row|
    attrs = block.call(row)
    klass.new(attrs).save!
    bar.inc
  end
  bar.finished
end


Command pattern with structs I'm using the command pattern to enable actions in my game to be undoable. ICommand defines individual commands, ICommandHandler does the undo-redo logic and might be decorated with other handlers like logging etc.

public interface ICommand
{
    void Execute();
    void Undo();
}

public interface ICommandHandler
{
    void Execute(ICommand command);
    void Undo();
    void Redo();
}

public class CommandHandler : ICommandHandler
{
    Stack<ICommand> _undoStack = new Stack<ICommand>();
    Stack<ICommand> _redoStack = new Stack<ICommand>();

    public void Execute(ICommand command)
    {
        if (command == null)
            throw new ArgumentNullException("command");

        command.Execute();
        _undoStack.Push(command);
        _redoStack.Clear();
    }

    public void Undo()
    {
        if (_undoStack.Count > 0)
        {
            ICommand undoCommand = _undoStack.Pop();
            undoCommand.Undo();
            _redoStack.Push(undoCommand);
        }
    }

    public void Redo()
    {
        if (_redoStack.Count > 0)
        {
            ICommand redoCommand = _redoStack.Pop();
            redoCommand.Execute();
            _undoStack.Push(redoCommand);
        }
    }
}

So far I'm not having any problems. Please correct me if I'm doing anything horribly wrong, but my actual questions is: Can I safely use a struct as a concrete command object or should I use a class?

public struct MoveCameraCommand : ICommand
{
    public CameraController cameraController;
    public Vector2 moveDelta;

    public MoveCameraCommand(CameraController controller, Vector2 moveDelta)
    {
        this.cameraController = controller;
        this.moveDelta = moveDelta;
    }

    public void Execute()
    {
        cameraController.Move(moveDelta);
    }

    public void Undo()
    {
        cameraController.Move(-moveDelta);
    }
}

I've noticed that when using a regular class, I'm generating garbage each frame with my camera, because move commands are issues very often. Since the game will be running on Android tablets, I was worried and switched to the struct implementation, although I've never seen it before. Might this cause other problems? Is this bad design in general? Should I not be using the command pattern for something like a camera, that is updated each frame?

It will not work well, I think. The only way to escape boxing on storing is to have a value type collection, so all your commands will need to be of the same type...
Also structs are very specific things, have a look below:

Here about Boxing/unboxing.
Size/speed issues - try to run this test.
See Using Structs:

When you create a struct object using the new operator, it gets created and the appropriate  constructor is called. Unlike classes, structs can be instantiated without using the new operator. In  such a case, there is no constructor call, which makes the allocation more efficient. However, the  fields will remain unassigned and the object cannot be used until all of the fields are initialized.
When a struct contains a reference type as a member, the default constructor of the member must be  invoked explicitly, otherwise the member remains unassigned and the struct cannot be used. (This  results in compiler error CS0171.)
There is no inheritance for structs as there is for classes. A struct cannot inherit from another  struct or class, and it cannot be the base of a class. Structs, however, inherit from the base class  Object. A struct can implement interfaces, and it does that exactly as classes do.


Insertion Sort Implementation Efficiency Is my approach to insertion Sort in Python efficient?

What can be improved efficiency wise?

def insertion_sort(array):
    sorted_array = []

    for elem in array:
        insertion_index = len(sorted_array) #insert at end of list by default
        for elem_sorted in sorted_array:
            if elem_sorted > elem:
                insertion_index = sorted_array.index(elem_sorted)
                break
        sorted_array.insert(insertion_index, elem)
    return sorted_array

num_elems = int(input('Number Of Elements?: '))
array = [int(input(f'Enter Number#{i+1}: ')) for i in range(num_elems)]
a = insertion_sort(array)
print(a)

You are looping over the array elements, and once you find an element greater than the element you are about to insert, you have to again find that element in the array of elements.

        insertion_index = len(sorted_array)
        for elem_sorted in sorted_array:
            if elem_sorted > elem:
                insertion_index = sorted_array.index(elem_sorted)
                break

You could instead use enumerate to extract both the element and its index:

        insertion_index = len(sorted_array)
        for index, elem_sorted in sorted_array:
            if elem_sorted > elem:
                insertion_index = index
                break

When you search for an element using a for loop and break when you find the element of interest, if you don't find the element, the for loop will execute an optional else: clause, so you don't need to preset the insertion_index:

        for index, elem_sorted in sorted_array:
            if elem_sorted > elem:
                insertion_index = index
                break
        else:
            insertion_index = len(sorted_array)

Biggest inefficiency

sorted_array is in sorted order.  You could use a binary search to find the insertion location, \$O(\log N)\$, instead of a linear search \$O(N)\$.


Simplifying Code for Drop-Down-Box in JQuery and HTML I'm trying to come up with a way to make a drop down box that is displayed through a jquery mouse hover event and with nested dropdown boxes displayed through hovering over elements of the original drop down box. I wrote some terribly inefficient code and I'm struggling to find ways of simplifying it. If anyone has any suggestions that will help me shorten this code and get a better idea of how to take advantage of functions of JQuery, please help.

here is the link:

http://cs-dev.dreamhosters.com/dropd.php

$(document).ready(function(){
$(".tab, .drop").hover(function(){
$(".tab").css("color","#FF7722");
$(".drop").css("display","block");
$("#tv, .droptv").hover(function(){
        $(this).css("color","#FF7722");
        $(".droptv").css("display","block");
        $(".droptv").hover(function(){
                $("#tv, .droptv").css("color","#FF7722");
                },function(){
                $(".droptv").css("color","#005BAB");
                });
        },function(){
        $(this).css("color","#005BAB");
        $(".droptv").css("display","none");
        });
$("#interact").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
$("#online").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
$("#vod, .dropvod").hover(function(){
        $(this).css("color","#FF7722");
        $(".dropvod").css("display","block");
        $(".dropvod").hover(function(){
                $("#dai").hover(function(){
                    $(this).css("color","#FF7722");
                    },function(){
                    $(this).css("color","#005BAB");
                    });
                $("#iguide").hover(function(){
                    $(this).css("color","#FF7722");
                    },function(){
                    $(this).css("color","#005BAB");
                    });
                $("#vod").css("color","#FF7722");
                },function(){
                $(".dropvod").css("color","#005BAB");
                });
        },function(){
        $(this).css("color","#005BAB");
        $(".dropvod").css("display","none");
        });
$("#tablet").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
$("#mobile").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
},function(){
$(".tab").css("color","#005BAB");
$(".drop").css("display","none");
 });
});

Your code is very big and messy so this a bit tricky to really see what you are trying to do.  A few obvious things to make the code more readable:

Replace .css("display","block") with .show()
Replace .css("display","none") with .hide()
You repeat the same color changing hover over and over again.  Instead, group all your elements together and specify this function only once:

$("#containo div").hover(function() {
    $(this).css("color","#FF7722");
},function() {
    $(this).css("color","#005BAB");
});
Do not nest hovers inside other hovers.  .hover() creates a new event handler when it is called.  If you nest them then you each time you move your mouse over the parent, the child is assigned a new event handler.  You do not want these duplicates.  Instead assign all of your events in the root level.

This, plus a little refactoring could reduce your code considerably.  Maybe from here the code will be easier to work with so could see how to reduce it further.

$(document).ready(function() {

    function on(selector) {
        $(selector).css("color","#FF7722");
    };
    function off(selector) {
        $(selector).css("color","#005BAB");
    };

    $("#interact,#online,#tablet,#mobile,#dai,#iguide,#tv,.droptv,#vod,.dropvod")
    .hover(function(){
        on(this)
    },function(){
        off(this)
    });

    $("#tv, .droptv").hover(function(){
        $(".droptv").show();
    },function(){
        $(".droptv").hide();
    });

    $("#vod, .dropvod").hover(function(){
        on("#vod");
        $(".dropvod").show();
    },function(){
        off(".dropvod");
        $(".dropvod").hide();
    });

    $(".tab, .drop").hover(function(){
        on(".tab");
        $(".drop").show();
    },function(){
        off(".tab");
        $(".drop").hide();
    });
});


Project Euler # 52 Permuted multiples in Python It can be seen that the number, 125874, and its double, 251748, contain exactly the same digits,
but in a different order.

Find the smallest positive integer, x, such that 2x, 3x, 4x, 5x, and 6x, contain the same digits.

from time import time

def is_permutation(n1, n2):
    """returns True if n1 is permutation of n2"""
    if n1 == n2:
        return False
    str1 = str(n1)
    str2 = str(n2)
    digits1 = {digit: str1.count(digit) for digit in str1}
    digits2 = {digit: str2.count(digit) for digit in str2}
    if not digits1 == digits2:
        return False
    return True

def check_multiples(n, start=10):
    """checks 2x, 3x, 4x, 5x, 6x and returns the numbers if they are permutations."""
    for x1 in range(start, n):
        x2 = x1 * 2
        if is_permutation(x1, x2):
            x3 = x1 * 3
            if is_permutation(x2, x3):
                x4 = x1 * 4
                if is_permutation(x3, x4):
                    x5 = x1 * 5
                    if is_permutation(x4, x5):
                        x6 = x1 * 6
                        if is_permutation(x5, x6):
                            return x1, x2, x3, x4, x5, x6

if __name__ == '__main__':
    start_time = time()
    print(check_multiples(1000000, 125000)[0])
    print(f'Time: {time() - start_time} seconds.')

For the umpteenth time, do not write

if not digits1 == digits2:
    return False
return True

Do write

return digits1 == digits2

The magic number 125000 must be explained. Preferably in the code, not in comments.
Seven levels of indentation should be immediately seen as a big bright red flag, style wise. At least fold it into the loop.
Seven levels of indentation should be immediately seen as a big bright red flag, algorithm wise. How would you approach a question asking for 16, rather than 6, multiples? As a side note, if you continue with PE, you have a chance to encounter it.
Expanding on the bullet above, please take my advice and study some math. This problem does have a nice solution. Ponder on the fact that \$142857 * 7 = 1000000 - 1\$. An Euler's generalization of Fermat's Little theorem is a must. PE is very fond of it.

And stop brute forcing.


What can I do better in this ViewModel Creator? I'm currently creating a ASP.NET MVC page in C#.

I want to hide everything regarding the creation of our "models" and "viewmodels".

I have seen much of the fancy stuff regarding Dependency Injection as well as Action Filters, Custom Controllers and so on! But the people I work with are pretty new to this so I want it to keep it dead simple, but I still want to avoid all the new statement and keep the controller really lightweight.

Please advise me if I'm doing this the wrong way, and be constructive.

Factory Class

 public class Factory
 {
    public T BuildViewModel<T>() where T : class, new()
    {
        IViewModel<T> iVmInterf = (IViewModel<T>)Activator.CreateInstance<T>();

        return iVmInterf.getViewModel();
    }
 }

IViewModel InterFace

 interface IViewModel<T>
 {
    T getViewModel();
 }

interface IProductViewModel

interface IProductViewModel
{
    int ProductCost { get; }
    string ProductName { get; }
}

class ProductViewModel

class ProductViewModel : IViewModel<ProductViewModel>, IProductViewModel
{
    private int _productCost;
    private string _productName;
    private IProductViewModel interf;

    int IProductViewModel.ProductCost
    {
        get { return _productCost; }
    }

    string IProductViewModel.ProductName
    {
        get { return _productName; }
    }

    ProductViewModel IViewModel<ProductViewModel>.getViewModel()
    {
        return this;
    }
}

Usage in controller

Factory fact = new Factory();
IProductViewModel vmdl = fact.BuildViewModel<ProductViewModel>();

Var

Use var for method-scope declarations when the right-hand side of the declaration makes the type obvious. This gives you the convenience during refactoring of being able to change the type in just one place.

e.g.

IViewModel<T> iVmInterf = (IViewModel<T>)Activator.CreateInstance<T>();

should be

var iVmInterf = (IViewModel<T>)Activator.CreateInstance<T>();

Naming

You should not include hints about the type of a variable in its name, unless the variable only makes sense with that name. Additionally you should not abbreviate names, abbreviating names makes your code harder to read for no positive benefit.

e.g.

IViewModel<T> iVmInterf

should be

IViewModel<T> viewModel

and

private IProductViewModel interf;

should be

private IProductViewModel viewModel;

Design

Why does ProductViewModel contain an instance variable of type IProductViewModel when it itself implements ProductViewModel? It doesn't appear to be used anywhere and it doesn't make obvious sense why a view model would need a reference to another view model of the same type.

IViewModel<T> takes an instance of itself for T, which I do not agree with from a design perspective. It doesn't make as much sense to say "A view model for product view models" than it does to say "A view model for products". As such I'd recommend making T refer to the model type the view model is for.

Secondly, IViewModel's one method seems entirely useless. Imagine the scenario in which you want to call GetViewModel. Every time you call that method, you will be doing so with an instance of that viewmodel. It's a wasted call, you already have that data.

With that in mind, your factory method becomes one line:

public T BuildViewModel<T>() where T : class, new()
{
    return Activator.CreateInstance<T>();
}


PHP function to access a database and return json <?php

include_once('../dbInfo.php');

function getReport($user_table) {
    $tables = array(
        "day"        => "p_day",
        "month"      => "p_month"
        ... etc. .....
    );

    $table = $tables[$user_table];

    if(!$table) {
        die(json_encode(array("error" => "bad table name")));
    }

    $con = getConnection(); // getConnection is in '../dbInfo.php'

    $query = "select * from " . $table;
    $res = mysql_query($query, $con);

    if(!$res) {
        die(json_encode(array("error" => "no results from table")));
    }

    $fields_num = mysql_num_fields($res);
    $fields = array();
    for($i=0; $i < $fields_num; $i++) {
        $field = mysql_fetch_field($res);
        $fields[$i] = $field->name;
    }

    $i = 0;
    while($row = mysql_fetch_array($res)) {
        $rows[$i] = $row;
        $i++;
    }
    $json = array("rows" => $rows, "headers" => $fields);

    $jsontext = json_encode($json);
    return $jsontext;
}

?>

What this code is doing:

access the database, selecting rows from a table, and returning them as a serialized json object
a table name is looked up in $tables -- the keys are acceptable user input, the values are actual table/view names in the database
data is selected from the table
the data is put into a big hash
the hash is serialized as a json string and returned

Specific issues I'm concerned about:

security -- is my DB connection info safe?  This file is in the root directory of public content, so dbiInfo.php, with the database connection information, is not publicly accessible (I think)
security -- am I open to SQL injection attacks?  I build a SQL query with string concatenation
security -- $user_table is untrusted input; is it safe?  It's only used as a key to look up trusted input ...
error handling -- have I dealt with all error conditions
there are lots of versions of PHP functions -- am I using the right ones?

General issues:

following conventions
quality/readability/comments

Edit:  the data is publicly available -- I'm worried about somebody getting more than read access to one of the listed tables, or any access to any other table in the DB.

This:

$tables = array(
    "day"        => "p_day",
    "month"      => "p_month"
    ... etc. .....
);

$table = $tables[$user_table];

if(!$table) {
    die(json_encode(array("error" => "bad table name")));
}

will throw a notice if $user_table is not a valid array key, something you should have already tested. Rewrite as:

$tables = array(
    "day"        => "p_day",
    "month"      => "p_month"
    ... etc. .....
);

if(!array_key_exists($user_table, $tables)) {
    die(json_encode(array("error" => "bad table name")));
}

$table = $tables[$user_table];

I really hate it when functions die(), and in your case there's no point in that. You could simply do:

if(!$table) {
    return json_encode(array("error" => "bad table name"));
}

since the function is expected to return a json formatted string. If you really want to die() you should do that where you call the function and the returned string is an error. Or you could just return false when an error occurs and the raw array when everything works, and when calling the function do something like:

$result = json_encode(getReport("some_table"));

That way getReport() will be useful even when you don't need json encoded output. But that's up to you and how you actually use it.

As @LokiAstari mentions mysql_* functions are deprecated and should be avoided. I would skip mysqli_* functions too and use PDO which will give you the extra bonus of switching to another database engine if you ever need to and it has a nice OO interface.

For everything else, I'm with @LokiAstari.


How to optimize two loop I have a dataset and I want to search task name inside initialTasks array.
const stagesTasks = [
    {
      "dataTestID": "stage-0",
      "headerText": "Backlog",
      "initialTasks": ["task 1", "task 2", "task 3"]
    },
    {
      "dataTestID": "stage-1",
      "headerText": "To Do",
      "initialTasks": ["task 4", "task 5", "task 6"]
    },
    {
      "dataTestID": "stage-2",
      "headerText": "Ongoing",
      "initialTasks": ["task 7", "task 8"]
    },
    {
      "dataTestID": "stage-3",
      "headerText": "Done",
      "initialTasks": ["task 9"]
    }
  ]

For example if I want to know dataTestID for "task 8", I have to make two loops. Like below :-
getStageName = task => {
    for(let record of stagesTasks){
      for(let data of record.initialTasks){
        if(data === task){
          return record
        }
      }
    }
    return null
  }

Just need your guidance to identify is there any way to avoid two loops?

You could use find to get the object which includes the given task in its initialTasks array. This also uses 2 loops. But, it returns when the match is found and is less verbose.

const stagesTasks=[{dataTestID:"stage-0",headerText:"Backlog",initialTasks:["task 1","task 2","task 3"]},{dataTestID:"stage-1",headerText:"To Do",initialTasks:["task 4","task 5","task 6"]},{dataTestID:"stage-2",headerText:"Ongoing",initialTasks:["task 7","task 8"]},{dataTestID:"stage-3",headerText:"Done",initialTasks:["task 9"]}];

function getStageName(task) {
  return stagesTasks.find(s => s.initialTasks.includes(task))?.dataTestID
}

console.log(getStageName("task 2"))
console.log(getStageName("task 7"))
console.log(getStageName("doesn't exist"))

If you call this function repeatedly, then you could create a mapper object which maps each task to it's dataTestID. You can just use mapper[task] to obtain the dataTestID. Since you are returning early, I'm assuming the tasks are unique.

const stagesTasks=[{dataTestID:"stage-0",headerText:"Backlog",initialTasks:["task 1","task 2","task 3"]},{dataTestID:"stage-1",headerText:"To Do",initialTasks:["task 4","task 5","task 6"]},{dataTestID:"stage-2",headerText:"Ongoing",initialTasks:["task 7","task 8"]},{dataTestID:"stage-3",headerText:"Done",initialTasks:["task 9"]}];

const mapper = stagesTasks.reduce((acc, o) => {
  o.initialTasks.forEach(t => acc[t] = o.dataTestID)
  return acc
}, {})

function getStageName(task) {
  return mapper[task]
}

console.log(getStageName("task 2"))
console.log(getStageName("task 7"))
console.log(getStageName("task 9"))


C program to crack passwords I have implemented a program in C to crack passwords by generating all possible combinations of words ([A-Z][a-z]) up to the length of 5. While the program works, I would like to receive comments on the efficiency of the algorithm and other design decisions that would improve the code. The exercise is part of the course CS50 by Harvard.

I timed the program using unix's time and the time the program took to print all the combinations was

**real**    14m39.433s;
**user**    0m10.040s;
**sys**     0m36.356s.

CS50.h is a library developed for the course as training wheels for students. String (char*) and Bool are types defined in this library.

#define _XOPEN_SOURCE
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <cs50.h>

int main(int argc, char *argv[])
{
    if(argc != 2)
    {
        printf("Usage: ./crack hash\n");
        return 1;
    }

    string alphabet = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
    char password[6] = "\0\0\0\0\0\0";
    string hash = argv[1];
    char salt[3];
    memcpy(salt, hash, 2);
    salt[2] = '\0';

    bool flag = false;
    int alphabet_len = 52;

    for(int i = 0; i < alphabet_len; i++)
    {
        password[0] = alphabet[i];
        password[1] = password[2] = password[3] = password[4] = '\0';
        if(!strcmp(hash, crypt(password, salt)))
        {
            flag = true;
            break;
        }
        for(int j = 0; j < alphabet_len; j++)
        {
            password[1] = alphabet[j];
            password[2] = password[3] = password[4] = '\0';
            if(!strcmp(hash, crypt(password, salt)))
            {
                flag = true;
                break;
            }
            for(int k = 0; k < alphabet_len; k++)
            {
                password[2] = alphabet[k];
                password[3] = password[4] = '\0';
                if(!strcmp(hash, crypt(password, salt)))
                {
                    flag = true;
                    break;
                }
                for(int l = 0; l < alphabet_len; l++)
                {
                    password[3] = alphabet[l];
                    password[4] = '\0';
                    if(!strcmp(hash, crypt(password, salt)))
                    {
                        flag = true;
                        break;
                    }
                    for(int m = 0; m < alphabet_len; m++)
                    {
                        password[4] = alphabet[m];
                        if(!strcmp(hash, crypt(password, salt)))
                        {
                            flag = true;
                            break;
                        }
                    }
                    if(flag)
                        break;
                }
                if(flag)
                    break;
            }
            if(flag)
                break;
        }
        if(flag)
            break;
    }

    if(flag)
        printf("Password: %s\n", password);
    else
        printf("Password not found\n");
}

cs50.h?

This seems less like a set of "training wheels" and more like a bicycle for fish. It's potentially confusing, opaque, and doesn't seem all that useful. If I were you, I'd be learning how to code in real C - using char*, and bool from stdbool.h.

Don't store things that should be computed

Your string alphabet shouldn't exist. Just iterate a char between a-z and A-Z. Characters can be incremented the same way that integers can.

Input validation

It seems like you expect hash to be two characters long, but you don't check that. You should be checking it with strlen; then you can issue memcpy without later setting a null terminator, as it'll be null-terminated already.

DRY

Don't repeat yourself. This is the most important aspect of the program that needs improvement. This block:

                for(int m = 0; m < alphabet_len; m++)
                {
                    password[4] = alphabet[m];
                    if(!strcmp(hash, crypt(password, salt)))
                    {
                        flag = true;
                        break;
                    }
                }
                if(flag)
                    break;

is repeated nearly verbatim five times. There are many different ways to condense this. The easiest is probably a recursive function that calls itself with an increasing depth integer. This may actually decrease the performance of the application, but that's up to you to test. There are also ways to rewrite this loop to have state so that neither copy-and-paste nor recursion are necessary; you'll probably want to compare such a method against a recursive method to see which is more performant and clean.


Memory issues when copying JPEG into bmp I am trying as a part of a bigger job to merge parts of several JPEGs into one huge BMP which will be processed further.

As a start, I tried to just copy a few JPEGs each to his own BMP file. In doing so I noticed that Visual Studio 2015 Community shows increased memory consumption if I repeat the copying of files in a single debug-run. The responsible part of the code is as follows.

Please, can you help me with this increasing memory consumption? In the future, I might need to read a few thousand JPEGs so this might be a serious issue.

private void GluePhotos(string[] files) {
    foreach (string file in files) {
        //Read original JPEG
        Stream imageSource = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.Read);
        JpegBitmapDecoder jpegDecoder = new JpegBitmapDecoder(imageSource, BitmapCreateOptions.PreservePixelFormat, BitmapCacheOption.Default);
        BitmapSource bitmapSource = jpegDecoder.Frames[0];

        //Prepare memory
        int stride = bitmapSource.PixelWidth * ((bitmapSource.Format.BitsPerPixel + 7) / 8);
        byte[] pixels = new byte[bitmapSource.PixelHeight * stride];

        //Copy JPEG into memory
        bitmapSource.CopyPixels(pixels, stride, 0);

        //Copy memory into a new BMP-to-be
        WriteableBitmap bitmap = new WriteableBitmap(bitmapSource.PixelWidth, bitmapSource.PixelHeight, bitmapSource.DpiX, bitmapSource.DpiY, bitmapSource.Format, bitmapSource.Palette);
        bitmap.WritePixels(new System.Windows.Int32Rect(0, 0, bitmapSource.PixelWidth, bitmapSource.PixelHeight), pixels, stride, 0);

        //Encode it as a BMP
        BmpBitmapEncoder bmpEncoder = new BmpBitmapEncoder();
        BitmapFrame newBitmapFrame = BitmapFrame.Create(bitmap);
        bmpEncoder.Frames.Add(newBitmapFrame);

        //Prepare paths for saving, save and cleaning(?)
        string dirPath = file.Insert(file.LastIndexOf('\\'), "\\ripoffs");
        dirPath = dirPath.Remove(dirPath.LastIndexOf('\\'));
        Directory.CreateDirectory(dirPath);
        string fileName = file.Insert(file.LastIndexOf('\\'), "\\ripoffs");
        fileName = fileName.Remove(fileName.LastIndexOf('.')) + "_ripoff.bmp";
        Stream imageOutput = new FileStream(fileName, FileMode.Create);
        bmpEncoder.Save(imageOutput);
        imageOutput.Close();
        imageSource.Close();
    }
}

If you need comments to document the flow of your method, then you're doing it wrong. Obviously this needs to be split into much smaller methods, each dedicated to their own logic.

That was you could move the three lines below //Read original JPEG to a method of their own that would return a BitmapSource, and you could encapsulate imageSource in a using block (considering it's an IDisposable).

Same for imageOutput: another stream you're not properly disposing of.

Quite frankly, I'd move everything inside the foreach into a class of its own and start refactoring from there.

I had to look up what this does:

string dirPath = file.Insert(file.LastIndexOf('\\'), "\\ripoffs");

I can't help but feel that this is a hacky way of something that should be done with for instance FileInfo and Path etc;

Consider this (I'm assuming file is a global variable of the class that contains all of the code in the foreach):

    private string GetResultPath()
    {
        var fileInfo = new FileInfo(file);

        var directoryPath = fileInfo.DirectoryName;
        var newDirectoryPath = Path.Combine(directoryPath, "ripoffs");

        Directory.CreateDirectory(newDirectoryPath);

        var fileName = fileInfo.Name.Replace(fileInfo.Extension, string.Empty)
                                   + "_ripoff"
                                   + fileInfo.Extension;

        return Path.Combine(newDirectoryPath, fileName);
    }

Yes, that's longer, but it's also much clearer and informs the next developer who looks at that code what it does.


Function to split strings on multiple delimiters I have this implementation of the split algorithm that different from .split() method you can use with multiple delimiters. Is this a good way of implementing it (more performance)?

def split(str, delim=" "):
    index = 0
    string = ""
    array = []
    while index < len(str):
        if str[index] not in delim:
            string += str[index]
        else:
            if string:
                array.append(string)
                string = ""
        index += 1
    if string: array.append(string)
    return array

Using the standard .split() method:

>>> print "hello = 20".split()
['hello', '=', '20']

>>> print "one;two; abc; b ".split(";")
['one', 'two', ' abc', ' b ']

Using my implementation:

>>> print split("hello = 20")
['hello', '=', '20']

>>> print split("one;two; abc; b ", ";")
['one', 'two', ' abc', ' b ']

Multiple delimiters:

>>> print split("one;two; abc; b.e. b eeeeee.e.e;;e ;.", " .;")
['one', 'two', 'abc', 'b', 'e', 'b', 'eeeeee', 'e', 'e', 'e']

>>> print split("foo barfoo;bar;foo bar.foo", " .;")
['foo', 'barfoo', 'bar', 'foo', 'bar', 'foo']

>>> print split("foo*bar*foo.foo bar;", "*.")
['foo', 'bar', 'foo', 'foo bar;']

Obs: We can do something like using re.split().

There's no need to iterate using that while, a for is good enough.

Also string concatenation (+=) is expensive. It's better to use a list and join its elements at the end1.

def split(s, delim=" "):
    words = []
    word = []
    for c in s:
        if c not in delim:
            word.append(c)
        else:
            if word:
                words.append(''.join(word))
                word = []
    if word:
        words.append(''.join(word))
    return words

As Maarten Fabré suggested, you could also ditch the words list and transform the function into a generator that iterates over (yields) each word. This saves some memory if you're examining only one word at a time and don't need all of them in one shot, for example when you're counting word frequency (collections.Counter(isplit(s))).

def isplit(s, delim=" "):  # iterator version
    word = []
    for c in s:
        if c not in delim:
            word.append(c)
        else:
            if word:
                yield ''.join(word)
                word = []
    if word:
        yield ''.join(word)

def split(*args, **kwargs):  # only converts the iterator to a list
    return list(isplit(*args, **kwargs))

There's also a one-liner solution based on itertools.groupby:

import itertools

def isplit(s, delim=" "):  # iterator version
    # replace the outer parentheses (...) with brackets [...]
    # to transform the generator comprehension into a list comprehension
    # and return a list
    return (''.join(word)
            for is_word, word in itertools.groupby(s, lambda c: c not in delim)
            if is_word)

def split(*args, **kwargs):  # only converts the iterator to a list
    return list(isplit(*args, **kwargs))

1 From https://wiki.python.org/moin/PythonSpeed: "String concatenation is best done with ''.join(seq) which is an O(n) process. In contrast, using the + or += operators can result in an O(n**2) process because new strings may be built for each intermediate step. The CPython 2.4 interpreter mitigates this issue somewhat; however, ''.join(seq) remains the best practice".


Drawing circles with triangles As pyglet has no in-built method to return a vertex list of circle, I had to build one myself. This is a critical piece of code that I will use very frequently. I need it to be of very fine performance.

Here's what I've written:

""" Pyglet utilities. Designed to ease drawing of primitives with Pyglet. """

# Dependencies
import pyglet
from math import sin, cos, pi

# Constants
TAU = 2 * pi

def circle(x, y, r, n, c, b):
    """ Adds a vertex list of circle polygon to batch and returns it. """
    rad = TAU / n # getting 360 / n in radians
    index = list(chain.from_iterable( (0, x-1, x)  for x in range(2, n+1) ))
    index.extend( (0, 1, n) ) # end of fan
    p = x, y # adding center of fan
    for i in range(1, n+1):
        d = rad * i
        p += int(r * cos(d)) + x, int(r * sin(d)) + y
    p += x+r, y # adding end of fan
    return b.add_indexed(n+2, pyglet.gl.GL_TRIANGLES, None, index, ('v2i', p), ('c3B', (c+c[-3:])))

This is a test that draws a circle of increasing quality:

import pyglet
from utilities import circle

# Constants
WIN = 900, 900, 'TEST', False, 'tool' # x, y, caption, resizable, style
CENTER = WIN[0] // 2, WIN[1] // 2
RADIUS = 450
SPEED = 0.3 # in seconds

# Color constants
WHITE = (255, 255, 255) # for center
RED = (255, 0, 0)       # for points

# Variables
win = pyglet.window.Window(*WIN)
win.set_location(510, 90)
batch = pyglet.graphics.Batch()
n = 3 # starting with triangle

def on_step(dt):
    """ Logic performed every frame. """
    global batch, n
    batch = pyglet.graphics.Batch()
    circle(CENTER[0], CENTER[1], RADIUS, n, WHITE+RED*n, batch)
    n += 1

@win.event
def on_draw():
    """ Drawing perfomed every frame. """
    win.clear()
    batch.draw()

pyglet.clock.schedule_interval(on_step, SPEED)
pyglet.app.run()

This is a test that adds more and more circles to the batch:

import pyglet
from utilities import circle
from random import randrange

# Constants
WIN = 900, 900, 'TEST', False, 'tool', False, True, False # vsync off to unlimit fps
RADIUS = 30
CORNERS = 20

# Color constants
WHITE = (255, 255, 255) # for center
RED = (255, 0, 0)       # for points

# Variables
win = pyglet.window.Window(*WIN)
win.set_location(510, 90)
batch = pyglet.graphics.Batch()

def on_step(dt):
    """ Logic performed every frame. """
    global counter
    x = randrange(RADIUS, WIN[0] - RADIUS)
    y = randrange(RADIUS, WIN[1] - RADIUS)
    circle(x, y, RADIUS, CORNERS, WHITE+RED*CORNERS, batch)

@win.event
def on_draw():
    """ Drawing perfomed every frame. """
    win.clear()
    batch.draw()

pyglet.clock.schedule(on_step)
pyglet.app.run()

I would be glad if you would help to make my method faster, more readable and better overall.

And by the way, this is another version that uses GL_TRIANGLE_FAN and doesn't work because I don't know how to cut off one circle from another. You can try it out:

def circle2(x, y, r, n, c, b):
    """ Adds a vertex list of circle polygon to batch and returns it. """
    rad = TAU / n # getting 360 / n in radians
    p = x, y # adding center of fan
    for i in range(n):
        d = rad * i
        p += int(r * cos(d)) + x, int(r * sin(d)) + y
    p += x+r, y # adding end of fan
    return b.add(n+2, pyglet.gl.GL_TRIANGLE_FAN, None, ('v2i', p), ('c3B', (c)))

but use

# Additional specification of the last gradient
circle(x, y, RADIUS, CORNERS, WHITE+RED*CORNERS+RED, batch)

instead.

Oh, and my math:

I can suggest a bit of math trickiness here. I highly doubt that would improve the readability of your code, but these reduce the amount of required computations. And one remark, I haven't worked with Python, so, no code. Sorry about that. Only performance suggestions.

Keep your angles constant

The heaviest part you have in your vertex computation is constantly recomputing sine and cosine, as you're creating a circle by "rotating the radius" and its angle is constantly changing by a fixed value.

You can reduce this to computing only a single pair of sine and cosine by relying on the rotation matrix. Once computed, it allows you to rotate an arbitrary point around zero by a constant angle.

You push in the first "radius" yourself, then you rotate it by rad, then by 2*rad (and you're forced to compute its sine and cosine again) and so on. But you don't really need it! Instead, you can take the previous computed radius and rotate it by rad again, using the same sine and cosine. The math looks like:

x_new = x * cos(rad) - y * sin(rad)
y_new = x * sin(rad) + y * cos(rad)

There's only one thing to be cautious of: floating point isn't perfect and absolutely precise. And in extreme cases the error will add up with each iteration and may become noticeable. You can't really counter that in any way but reducing the number of iterations. And yes, you can do that too.

Exploit your axes

You have 2 orthogonal axes. And you're drawing a circle which has a neat property of having all the points within a specific distance from its center. Assuming the circle is centered at (0; 0) for brevity, how many points can you get by performing a single rotation? One? No, actually eight. Here are 3 independent things you can do with a single point to get some others, each doubles the amount of points you get:

Negate X
Negate Y
Swap X and Y

  1 * 2 * 2 * 2 == 8 # Yeah, okay.

Imagine you're traversing the circle only knowing 1/8th of its points.

You start from angle 0 (right), you reach pi/4 and...
You follow your pointset in reverse with coordinates swapped, until you reach pi/2.
You swap coordinates back, negate X and follow the pointset straight again, you reach 3pi/4.
You swap coordinates and follow the pointset in reverse again until you reach pi.
You swap coordinates, negate Y and traverse the lower semicircle the same way as above.

So it makes sense to only compute 1/8th of the circle, points in [0; pi/4]. This takes 8 times less point computations, but you should store the results and reuse them when drawing the circle. This requires some extra memory.

Of course, this does not relieve you of benchmarking the improvement. The above is math, just theory.


Function for formatting an array This function is working fine but I'm looking for any better alternative:

function formate(array) {
    var formattedArray = [];
    var count = array.length;

    for (var i = 0; i < count; i++) {
        formattedArray.push({
            all: array[i].all ? array[i].all.split(',') : [],
            any: array[i].any ? array[i].any.split(',') : [],
            not: array[i].not ? array[i].not.split(',') : [],
        });
    }

    return formattedArray;
}

Input:

[{all: "one,two", any: "three,four", not: ""},{all: "five,six", any: "", not: "seven"}]

Output:

[{all: ["one", "two"], any: ["three", "four"], not: []}, {all: ["five", "six"], any: [], not: ["seven"]}]

const formate = array => array.map(({all, any, not}) => ({all: all.split(","), any: any.split(","), not: not.split(",")}));

With empty arrays:

const formate = array => array.map(({all, any, not}) => ({all: all?all.split(","):[], any: any?any.split(","):[], not: not?not.split(","):[]}));


Matlab code demonstrating use of fft (Fast Fourier Transform) I posted the following Matlab script in response to a question on Signal Processing. Here is the questions with my answer.

I am looking for comments on how to make this code more instructive for the questioner. Any other code review comments are also welcomed.

fs = 2^10;        %sample frequency in Hz
T  = 1/fs;        %sample period in s
L  = 2^20;        %signal length
t  = (0:L-1) * T; %time vector

A1 = 0.2; %amplitude of x1 (first signal)
A2 = 1.0; %amplitude of x2 (second signal)
f1 = 1;   %frequency of x1
f2 = 50;  %frequency of x2

x1 = A1*sin(2*pi*f1 * t); %sinusoid 1
x2 = A2*sin(2*pi*f2 * t); %sinusoid 2
y  = x1 + x2;

%Plot signal
figure;
set(gcf,'Color','w'); %Make the figure background white
plot(fs*t(1:100), y(1:100));
set(gca,'Box','off'); %Axes on left and bottom only
str = sprintf('Signal with %dHz and %dHz components',f1,f2);
title(str);
xlabel('time (milliseconds)');
ylabel('Amplitude');

%Calculate spectrum
Y = fft(y)/L;
ampY = 2*abs(Y(1:L/2+1));
f = fs/2*linspace(0,1,L/2+1);
i = L/fs * (max(f1,f2)) + 1; %show only part of the spectrum

%Plot spectrum.
figure;
set(gcf,'Color','w');  %Make the figure background white
plot(f(1:i), ampY(1:i));
set(gca,'Box','off');  %Axes on left and bottom only
title('Single-Sided Amplitude Spectrum of y(t)');
xlabel('Frequency (Hz)');
ylabel('|Y(f)|');

Your time vector definition is defined a bit strange, I would go for the more standardized form of

startpoint:stepsize:endpoint

endpoint = T*(L-1) % is this correct?
0:T:endpoint

Besides this you could perhaps add some comments in the calculate spectrum section, but for the rest it looks quite clear.


Determine if 2 lists with similar objects contain a partial duplicate I have two lists of Person objects. I need to compare the lists to determine if items in list1 exist in list2 and vice versa.

This is not a straight equals() scenario as the objects are not the same, but may have identical field values. It is those values that I need to compare.

Currently, I am using Java 8's Streams API to return either a match or null:

import java.util.ArrayList;
import java.util.List;

public class Main {

    public static void main(String[] args) {

        ArrayList<Person> originalPeople = new ArrayList<>();
        ArrayList<Person> newPeople = new ArrayList<>();

        originalPeople.add(new Person("William", "Tyndale"));
        originalPeople.add(new Person("Jonathan", "Edwards"));
        originalPeople.add(new Person("Martin", "Luther"));

        newPeople.add(new Person("Jonathan", "Edwards"));
        newPeople.add(new Person("James", "Tyndale"));
        newPeople.add(new Person("Roger", "Moore"));

        // Create a list of people that no longer exist in the new list
        for (Person original : originalPeople) {
            if (getPersonInList(
                    newPeople, original.getFirstName(), original.getLastName()) == null) {
                System.out.printf("%s %s is not in the new list!%n",
                        original.getFirstName(), original.getLastName());
            }
        }
    }

    private static Person getPersonInList(
            final List<Person> list, final String firstName, final String lastName) {

        return list.stream()
                .filter(t -> t.getFirstName().equalsIgnoreCase(firstName))
                .filter(t -> t.getLastName().equalsIgnoreCase(lastName))
                .findFirst().orElse(null);
    }
}

class Person {

    private final String firstName;
    private final String lastName;

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

    String getFirstName() {
        return firstName;
    }

    public String getLastName() {
        return lastName;
    }
}

In my real-world application, however, both lists have over 30,000 items and this is very time-consuming.

Is there a better way to perform this comparison? I would want this to work both ways as well, so if there are items in newPeople that were not in originalPeople, I would want that list as well.

There are certain reasons I would not prefer your approach. These are:

1) There are multiple method calls which reduces the readability.

2) You are using filter twice which decreases the performances. You could do it inside the same filter like I've shown below

3) With this approach you don't have a consolidated List which contains common elements. For that you need another else condition which increases the cyclomatic complexity.

4) A null check which could produce NPE if not handled carefully as you are using findFirst() which returns Optional (Although null can be replaced with default new Person("","") object which is again not recommended).

5) Finally don't use ArrayList<Person> originalPeople = new ArrayList<>();; rather declare as List, to follow programming to interface.

Rather I would use below approach:

private static List<Person> getPersonInList(
            final List<Person> newPeople, List<Person> originalPeople) {
        List<Person> list = new ArrayList<>();

        newPeople.forEach(p ->
                originalPeople.stream()
                              .filter(p1 -> p.getFirstName().equals(p1.getFirstName()) &&
                                      p.getLastName().equals(p1.getLastName()))
                              .forEach(list::add));
        return list;
    }


Message queues in Common Lisp General advice for a change. This is an implementation of message queues that I'm going to use for some work on an actors model library.

(defclass message-queue ()
  ((messages :accessor messages :initarg :messages :initform nil)
   (last-cons :accessor last-cons :initarg :last-cons :initform nil
    :documentation "Cached end of the list")
   (len :accessor len :initarg :len :initform 0
    :documentation "Cached message queue length. Modified by enqueue and dequeue")
   (lock :initform (bt:make-lock) :accessor lock
    :documentation "Lock for this message queue")
   (max-len :accessor max-len :initarg :max-len :initform nil
    :documentation "If present, queue maintains at most this many elements")
   (flag :initform (bt:make-condition-variable) :accessor flag
    :documentation "Condition variable used to notify that a message was enqueued")))

(defun make-queue (&optional max-len)
  (make-instance 'message-queue :max-len max-len))

(defmethod full-p ((queue sized-queue))
  (with-slots (len max-len)
      (and max-len (>= len max-len))))

(defmethod empty-p ((queue message-queue))
  (= (len queue) 0))

(defmethod enqueue (object (queue message-queue))
  "Adds an element to the back of the given queue in a thread-safe way."
  (with-slots (lock messages max-len len flag last-cons) queue
    (with-lock-held (lock)
      (let ((o (list object)))
    (cond ((empty-p queue)
           (setf messages o
             last-cons messages
             len 1))
          ((full-p queue)
           (pop messages)
           (setf (cdr last-cons) o
             last-cons o))
          (t (setf (cdr last-cons) o
               last-cons o)
         (incf len)))))
    (condition-notify flag)
    messages))

(defmethod dequeue ((queue message-queue) &optional (timeout 0))
  "Pops a message from the given queue in a thread-safe way.
If the target queue is empty, blocks until a message arrives.
If timeout is not zero, errors after timeout."
  (with-slots (messages lock flag len) queue
    (with-timeout (timeout)
      (with-lock-held (lock)
    (unless messages (condition-wait flag lock))
    (decf len)
    (pop messages)))))

(defmethod dequeue-no-hang ((queue message-queue))
  "Pops a message from the given queue in a thread-safe way.
If the target queue is empty, returns NIL.
The second value specifies whether an item was found in queue (this is meant
to disambiguate the situation where a queue contains the message NIL)"
  (with-slots (messages lock flag len) queue
    (with-lock-held (lock)
      (if messages
      (progn
        (decf len)
        (values (pop messages) t))
      (values nil nil)))))

I noticed you defined full read/write accessors on all the slots on your message queue class, yet you bypass all of that and just use with-slots. Are you planning on exporting those accessors as part of the API? It seems like you wouldn't want to encourage people to monkey with those slot values, so probably not. You could also drop :initarg from everything but max-len.


Uploading a file using Net::SFTP I have a method to upload a file using the Net::SFTP gem:

class Uploader

 def initialize(host,user,password)
   @host = host
   @user = user
   @password = password
 end

 def upload(local_file_path,remote_file_path)
  Net::SFTP.start(@host,@user,:password => @password) do |sftp|
   sftp.upload!(local_file_path,remote_file_path) do |event,uploader,*args|
     case event
     when :open
       Rails.logger("Starting upload...")
     when :finished
       Rails.logger("Finishing upload...")
     end
   end
  end
 end
end

The upload method seems rather large to me. Any suggestions on how I could split it up into smaller components?

I don't see much reason to worry about this method's length. At least not right now - if you add more event handling, yeah, it might become a little cumbersome. But for what it does now, there's no pressing need to do anything.

I would recommend more whitespace, though. There's no reason to compress,every,line,that,has,commas,in,it. Just makes it harder to read.

I'm also fairly sure that the :finished event is actually called :finish

Structurally, just for the sake of it, you could do something like:

class Uploader
  attr_reader :host, :user, :password

  def initialize(host, user, password)
    @host = host
    @user = user
    @password = password
  end

  def upload(local_file_path, remote_file_path)
    Net::SFTP.start(host, user, password: password) do |sftp|
      sftp.upload!(local_file_path, remote_file_path) do |event, uploader, *args|
        send(event, uploader, *args) if respond_to?(event, true)
      end
    end
  end

  private

  def open(uploader, *args)
    Rails.logger("Starting upload...")
  end

  def close(uploader, *args)
    Rails.logger("Upload complete")
  end

  def finish(uploader, *args)
    Rails.logger("All done")
  end
end

Basically, we're handling events with methods instead of a case block. I've added the close method/event handler as an example.


Adapter design pattern I am referring to the book Design Patterns For Dummies to learn about design patterns.  I wrote an adapter design pattern, as mentioned in the book.  Is it proper or does it need some modification?

#include<iostream>
#include<sstream>

using namespace std;

class AceInterface
{
public:
    virtual void setName(string n) = 0;
    virtual string getName() = 0;
};

class Aceclass : public AceInterface
{
    string name;
public:
    void setName(string n){
        name = n;
    }
    string getName() {
        return name;
    }
};

class AcmeInterface
{
public:
    virtual void setFirstName(string fn) = 0;
    virtual void setLastName(string ln) = 0;
    virtual string getFirstName() = 0;
    virtual string getLastName() = 0;
};

class Acmeclass :public AcmeInterface
{
    string First_name;
    string Last_name;
public:
    void setFirstName(string fn) {
        First_name = fn;
    }
    void setLastName(string ln) {
        Last_name = ln;
    }
    string getFirstName(){
        return First_name;
    }
    string getLastName(){
        return Last_name;
    }
};

class AcetoAcmeAdapter:public AcmeInterface
{
    Aceclass aceObject;
    string First_name;
    string Last_name;
public:
    AcetoAcmeAdapter(Aceclass a) {
        aceObject = a;
        istringstream iss(aceObject.getName());
        iss >> First_name ;
        iss >> Last_name;
    }
    void setFirstName(string fn) {
        First_name = fn;
    }
    void setLastName(string ln) {
        Last_name = ln;
    }
    string getFirstName(){
        return First_name;
    }
    string getLastName(){
        return Last_name;
    }
};

int main()
{
    Aceclass aceobj;

    aceobj.setName("Mohandas Gandhi");

    cout << "name from aceobj :" << aceobj.getName() <<endl;

    AcetoAcmeAdapter ace2acme(aceobj);

    cout << "First_name from ace2acme :" << ace2acme.getFirstName() << endl;
    cout << "Last_name from ace2acme :" << ace2acme.getLastName() << endl;

    return 0;
}

Minor things:

Don't use using namespace std. Read
this.
When you are naming variables, keep it consistent. Either stick with
camelCase or underscore.
For the inline functions, keep it on one line. It's easier to read.

void setName(string n){ name = n; }
string getName() { return name; }

I would suggest, anything over two lines, define outside of class.
Particularly the AcetoAcmeAdapter constructor.


Coredata delete all data in an entity I have this code that I am using to delete all records from an entity. The question is can it be done better or is already the best?

    func deleteIncidents() {
    let appDel: AppDelegate = UIApplication.sharedApplication().delegate as! AppDelegate
    let context: NSManagedObjectContext = appDel.managedObjectContext!
    let request = NSFetchRequest(entityName: "Incidents")
    request.returnsObjectsAsFaults = false

        do {
            let incidents = try context.executeFetchRequest(request)

            if incidents.count > 0 {

                for result: AnyObject in incidents{
                    context.deleteObject(result as! NSManagedObject)
                    print("NSManagedObject has been Deleted")
                }
                try context.save() } } catch {}
    }

let appDel: AppDelegate = UIApplication.sharedApplication().delegate as! AppDelegate
let context: NSManagedObjectContext = appDel.managedObjectContext!

The type annotations are not necessary, the Swift compiler can infer
the type automatically:

let appDel = UIApplication.sharedApplication().delegate as! AppDelegate
let context = appDel.managedObjectContext!

request.returnsObjectsAsFaults = false

This makes no sense here because you don't want to access
any properties of the returned objects. On the contrary, only the
managed object ID is needed to delete objects and no properties need
to be fetched at all, so this should be replaced by

request.includesPropertyValues = false

to increase the performance.

I prefer to cast the return value from the fetch request immediately

let incidents = try context.executeFetchRequest(request) as! [NSManagedObject]

this makes both the type annotation : AnyObject and the cast
as! NSManagedObject obsolete.

You have to decide how an error should be handled, but I would
at least print some message when running in debug mode

} catch let error as NSError {
    debugPrint(error)
}

to detect possible problems.

Starting with iOS 9, objects can be deleted directly in the store
without loading them into memory.
This would look like this:

func deleteIncidents() {
    let appDel = UIApplication.sharedApplication().delegate as! AppDelegate
    let context = appDel.managedObjectContext!
    let coord = appDel.persistentStoreCoordinator

    let fetchRequest = NSFetchRequest(entityName: "Incidents")
    let deleteRequest = NSBatchDeleteRequest(fetchRequest: fetchRequest)

    do {
        try coord.executeRequest(deleteRequest, withContext: context)
    } catch let error as NSError {
        debugPrint(error)
    }
}

There is a catch (!) however: Existing objects already loaded into
the managed object context are not automatically removed, and
doing so it a bit tricky. See https://www.bignerdranch.com/blog/new-in-core-data-and-ios-8-batch-updating/ for more information.


Extracting text data from a file Can I make this program that extracts text data from a file (size in MBs) even better performance-wise?  I need to reduce the execution time.

import java.io.*;

public class ReadData {

    public static void main(String[] args) {

        FileReader input = null;
        BufferedReader br = null;

        try {

            input = new FileReader("C:/Projects/Test/HPCamDrv.log");
            br = new BufferedReader(input);
            String str;

            while ((str = br.readLine()) != null) {
                System.out.println(str);
            }
        }

        catch (IOException e) {
            e.printStackTrace();
        }

        finally {

            try {
                input.close();
                br.close();
            }

            catch (IOException x) {
                x.printStackTrace();
            }

        }
    }
}

It's a pretty small and straight-forward program and not really much to improve on. ChrisWue also brings up the good point that printing to stdout for every line is costly but if you need to print it to stdout there's not much to do about it.

But one thing you can actually do is implement Java's try-with resources which will ensure your resources are closed at the end of the statement, so you can get rid of all your .close() calls and your finally block.


Assign object keys to another object's keys dynamically I have an array called viewObject.terms. This array can contain up to 3 items, I need to iterate through this array and save the values into a data object.

ie:

data.term_id_1 = viewObject.terms[0].term_id
data.ticker_1  = viewObject.terms[0].its_ticker

This is what I first tried, but I get the error: Cannot set property '0' of undefined

for (var i=0; i<viewObject.terms.length; i++) {
    data.term_id_[i+1] = viewObject.terms[i].term_id;
    data.ticker_[i+1]  = viewObject.terms[i].its_ticker;
}

So for the moment, I'm just using a switch case to handle this currently (albeit in a super ugly way):

if (viewObject.terms.length > 0) {
    switch(viewObject.terms.length) {
        case 1:
            data.term_id_1 = viewObject.terms[0].term_id;
            data.ticker_1  = viewObject.terms[0].its_ticker;
            break;
        case 2:
            data.term_id_1 = viewObject.terms[0].term_id;
            data.ticker_1  = viewObject.terms[0].its_ticker;
            data.term_id_2 = viewObject.terms[1].term_id;
            data.ticker_2  = viewObject.terms[1].its_ticker;
            break;
        case 3:
            data.term_id_1 = viewObject.terms[0].term_id;
            data.ticker_1  = viewObject.terms[0].its_ticker;
            data.term_id_2 = viewObject.terms[1].term_id;
            data.ticker_2  = viewObject.terms[1].its_ticker;
            data.term_id_3 = viewObject.terms[2].term_id;
            data.ticker_3  = viewObject.terms[2].its_ticker;
            break;
    }
}

Another solution to the problem could be expressed using Array.prototype.forEach(), as the following:

viewObject.terms.forEach(function(term, index){
    data['term_id_' + (index + 1)] = term.term_id;
    data['ticker_'  + (index + 1)] = term.its_ticker;
});


DRY Kleisli wrappers with lens This currently does what I intend:

import Control.Arrow (Kleisli, runKleisli)
import Control.Applicative (WrappedArrow(WrapArrow))
import Control.Lens.Traversal (preview)

nested :: Request -> Maybe Response
nested = runKleisli . unwrapArrow
       $ WrapArrow (Kleisli (preview (prefixed "/name") >=> app1))
     <|> WrapArrow (Kleisli (preview (prefixed "/surname") >=> app2))

  where
    prefixed :: Text -> Prism' Request Request
    prefixed = _

    app1,app2 :: Request -> Maybe Response
    app1 = _
    app2 = _

The idea here is to implement a kind of nested HTTP routing. Given the already existing applications app1 and app2, I want to reroute the incoming Request to either of those applications based on whether the path of the Request is prefixed with "/name" or "/surname". The prefixed Prism' strips such prefix from the Request path if present, otherwise it fails.

However I'd like to get rid, somehow, of WrapArrow, Kleisli and preview, and possibly, runKleisli . unwrapArrow. I suspect that this can be done with Control.Lens.Wrapped or Data.Profuntor but I still can't see exactly how.

I wouldn't like to a add an extra dependency for this case but I'm currently using lens in my project, so anything from lens or its dependencies is welcome.

I came out with this:

nested :: Request -> Maybe Response
nested = nest [ (prefixed "/name", app1)
              , (prefixed "/surname", app2)
              ]

  where
    nest xs request =
        getAlt $ foldMap (Alt . (\(p,a) -> reroute p a request)) xs

    reroute p app = preview p >=> app

Alt is only available in base-4.8.0.0, however it shouldn't be difficult to implement your own Alt.


Streaming my UPS metrics for graphical viewing in bash I have an APC UPS, which comes with the apcaccess utility to get various statistical values from the apcupsd daemon service.

I am using the following script to stream my line voltage data to thingspeak.com website which allows us to see the results streaming in real time.

The script is set as a cron job, executing every minute.

#!/bin/bash

API_URL="https://api.thingspeak.com/update?api_key=XXXXXXXXXXXXX&field%d=%s"
UPS_PROPS=(LINEV)

for index in ${!UPS_PROPS[@]}
do
    value=$(/sbin/apcaccess -up ${UPS_PROPS[$index]})
    curl $(printf $API_URL $(expr $index + 1) $value)
done

I have the UPS_PROPS as an array because I want to later extend the code to also stream some other data points.

The chart can be seen over at this link.

Good things

Using $() over backticks (``) for command substitution is a current best practice and does make code like this more readable.
It is also good use variables for long unchanging items like your API_URL.
Thanks for including the link to the graph.  This doesn't make your code any better, but it does lead to happier reviewers.  I'm such a fan of graphs I'd have thrown it inline.

Suggestions

Since you're using the $() form of command substitution, it is pretty easy to add some extra spaces inside of the parenthesis.  I think this makes it even more readable.  $( /sbin/apcaccess -up ${UPS_PROPS[$index]} )
Using the variable as a printf format is a good way to do it, but that means the value of the variable isn't a valid URL.  If this were a bit longer than 10 lines it wouldn't hard to imagine a naive maintenance programmer coming along and doing curl $API_URL somewhere and wondering why it is giving him errors.  So I would prefer to name the variable API_URL_FORMAT or API_URL_PROTO to make it clear it isn't a usable URL yet.
#!/usr/bin/env bash is considered more portable because it works with folks who can't install bash in the OS-proper place.
Run your code through https://www.shellcheck.net/ for several hints about quoting your substitutions.
It would be a bit clearer what's happening with the curl if you split it into two lines.  The first would set a variable like ADDING_URL="$(printf $API_URL_FORMAT $(expr $index + 1) $value)" then the second line would just be curl $ADDING_URL.  This leaves a pretty boring curl line I admit, but by using the variable it is easier to see what is happening with the printf before thinking about sending it out to the web.  And you may want more curl options later so the curl line could get more complicated.
Please use consistent case for your variable names!  Some are upper case, which is pretty traditional for shell scripts, but other half are lower case.  Some people like upper case because it makes the variables stand out more.  I don't think that makes much difference with modern syntax highlighting editors.  But it should definitely be consistent, either way, in such a short script.  One convention is to use upper case for exported/in the env variables and lower case for local/not in the env variables.  In the case all of your variables would be lower case since none of them are exported into the env.
Why are you looping over an array with one item in it?  Will it ever contain anything other than LINEV?  If not the code can be significantly simplified by getting rid of the for and just putting the values into the two inner lines.
If the for loop has some value I would try to use it like a shell for loop and loop over the values instead of the indexes.  I see that you use the index inside of the curl, but I'm not sure what that is doing for you.


Rearranging numbers of an array like a wave Given an array of integers, sort the array into a wave like array and
  return it, In other words, arrange the elements into a sequence such
  that a1 >= a2 <= a3 >= a4 <= a5.....

  Example

  Given [1, 2, 3, 4]

  One possible answer: [2, 1, 4, 3]
  Another possible answer : [4, 1, 3, 2]

  NOTE: If there are multiple answers possible, return the one thats
  lexicographically smallest.  So, in example case, you will return [2, 1, 4, 3]

My approach:

public class Solution {
    public ArrayList<Integer> wave(ArrayList<Integer> A) {

        Collections.sort(A);
        int swap1, swap2,temp1,temp2;
        //We will have to swap all the odd numbered elements with the even numbered elements
        //such that all numbers with a gap 0f 1 are sorted
        for( swap1= 0, swap2 = swap1 + 1; swap1 <= A.size()- 2; swap1 = swap1+2,swap2 = swap2+2 )
            {
                //Check if swap1 reaches last element
                if( (swap1 == A.size() - 1) || (swap2 == A.size()) )
                    break;
                else
                    {
                        temp1 = A.get(swap1);
                        temp2 = A.get(swap2);
                        A.set(swap1,temp2);
                        A.set(swap2, temp1);
                    }
            }
        return A;
    }
}

I have the following questions regarding my code:

How can I further optimize my code?  (I am unable to solve these questions within the prescribed time limit. What can I do?)
Is there any better approach to do this question?
Have I violated any convention in coding?
Are there any redundancies in my approach?

Reference

Few observations.

You don't need to maintain swap2 separately, it is always swap1 + 1, so can be simply declared inside the loop's body.
Your boundaries check inside the loop body is redundant, loop condition alone is enough.
You're probably performing too many int-to-Integer and back conversions. Even more, the question deals with arrays, you don't need to pre-convert the argument into a list. Accept, process and return an array of plain ints.


stat the word count for a given stackexchange user Sometimes I would like use word count to describe someone's contribution, so, I wrote a query to stat the word count for a given stackexchange user, based on SEDE. click to run it online

DECLARE @total_count int, @post_id int, @post_body nvarchar(max);

SET @total_count = 0;

DECLARE vendor_cursor CURSOR FOR
SELECT id, Body
FROM posts
WHERE posts.OwnerUserId = 9872;

OPEN vendor_cursor

FETCH NEXT FROM vendor_cursor
INTO @post_id, @post_body
PRINT FORMATMESSAGE('  id   | count | total count')
WHILE @@FETCH_STATUS = 0
BEGIN
    DECLARE @count int;
    SELECT @count = (LEN(@post_body ) - LEN(replace(@post_body , ' ', '')))
    SELECT @total_count = @total_count + @count
    PRINT FORMATMESSAGE('%6d | %5d | %d', @post_id, @count, @total_count)
    FETCH NEXT FROM vendor_cursor
    INTO @post_id, @post_body
END

Reviewing your T-SQL script I see that you have a cursor to iterate over Posts from a specific owner. For each body you use a neat trick to determine the word count of the body and use an extra variable to keep the running sum. You use FORMATMESSAGE to shape the data in each row.
Do know that the neat trick for the word count doesn't compensate for cases where two or more spaces are used, or where words are not separated by spaces but by other characters, like line-breaks for example. I'll ignore the fact here that in case of Posts.Body not only the text is stored but also the HTML markup.
I find the script clear and well structured.
If you're looking for an alternative way to accomplish the same output (I ignore the formatting here) I can offer this SEDE query:
DECLARE @UserId int = ##UserId##  -- SDE parameter syntax

SELECT Id
     , LEN(Body ) - LEN(replace(Body , ' ', '')) [count]
     , SUM(LEN(Body ) - LEN(replace(Body , ' ', '')))
         OVER (ORDER BY id) [total count]
FROM posts
WHERE posts.OwnerUserId = @UserId
ORDER BY Id

I have started with your base query as found in the CURSOR but instead added the third column with an OVER( ORDER BY id) clause which enables the calculation of running totals over sets.  I'm a firm believer that when there is a feature available in the language it should be the preferred option, instead of running your own version in script. There is also a higher chance this gets better optimized by the query plan, if not today then possible in the future. Looking at wall-time and the query plan for your version and mine seems to support my gut feeling.
If you want to address the neat trick there are different ways to solve that, for example by evaluation the answers on  Removing repeated duplicated characters but all of those have pros and cons and without confirmation if this is an area you want to have addressed I assume for now you're fine with your method.


Given an array of integers, find and print its number of negative subarrays on a new line Given an array of N integers, find and print its number of negative subarrays (i.e sub arrays having negative summation) on a new line.

My code is taking \$\mathcal{O}(n^2)\$ time. How can I improve this?

import java.util.Scanner;

public class Solution {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        int numberOfInts = sc.nextInt();
        int[] arr = new int[numberOfInts];

        for (int i = 0; i < numberOfInts; i++) {
            arr[i] = sc.nextInt();
        }
        int count = 0;
        for (int i = 0; i < numberOfInts - 2; i++) {
            int sum = arr[i];
            if (arr[i] < 0) {
                count++;
            }
            for (int j = i + 1; j < numberOfInts; j++) {
                sum = sum + arr[j];
                if (sum < 0) {
                    count++;
                }
            }
        }
        if (arr[numberOfInts - 1] < 0) {
            count++;
        }
        System.out.println(count);
    }
}

Your code doesn't work for an empty array. Therefore it is conceptually wrong.

You should rewrite your code to be easily and automatically testable. This means to extract the interesting part of the program and separate it from the rest. In this case, the interesting part is the given an array, find. The code should therefore look like this:

static int countNegativeSubarrays(int... numbers) {
    int count = 0;
    // Your code here
    return count;
}

This method can be called easily from a test method:

static void runSelfTest() {
    if (countNegativeSubarrays() != 0)
        throw new AssertionError();
    if (countNegativeSubarrays(-1) != 1)
        throw new AssertionError();
    if (countNegativeSubarrays(-1, -1) != 3)
        throw new AssertionError();
    if (countNegativeSubarrays(-1, -1, -1) != 6)
        throw new AssertionError();
    if (countNegativeSubarrays(1, -1, -2, 5) != 4)
        throw new AssertionError();
    // Add more test cases here
}

In your main method you should run this self-test before accepting any input from the user.


Find the missing element in vector I'm solving this simple challenge:

Given a vector A with N unique elements greater than 0 and lower or equal than N+1, find the missing element. Example:

A = [1,3,2,5] -> missing number 4

A = [1,3,5,4] -> missing number 2

I've come to the following solution. I'm interested in thoughts and ideas on how to write it as expressive as possible:

Option 1, compact but not very expressive:

int solution_1(std::vector<int> &v) {
    sort(v.begin(), v.end());
    for (std::size_t i = 0; i < v.size(); ++i) {
        if (v[i] != i+1) return i+1;
    }
    return v.size()+1;
}

Option 2

int solution_2(std::vector<int> &v) {
    sort(v.begin(), v.end());
    auto missing_element = std::find_if(
        v.begin(), v.end(),
        [index=1](auto& element) mutable {
            if (element != index++) {
                return true;
            } else {
                return false;
            }
        });
    if (missing_element == v.end()) {
        return v.size() + 1;
    } else {
        return *missing_element - 1;
    }
}

Any ideas on how to improve this or how to make it more expressive?

You dont need the sorting. The vector contains numbers 1 to (n + 1) with one number omitted. If it wasnt omitted it would sum up to (n+ 1)*(n+ 2)/ 2. Sum the vector, subtract it from the full sum And what Is left over Is the number that was omitted.


Maximize result of bitwise AND This was the problem from "30 Days of Code" on Hackerrank:

  Given set \$S = \lbrace{1,2,3,\dots,N}\rbrace\$, find two integers \$A\$ and \$B\$ (where \$A \lt B\$), from set \$S\$ such that the value of \$A\&B\$ is the maximum possible and also less than a given integer, \$K\$. In this case, \$\&\$ represents the bitwise AND operator.

  The first line contains an integer, \$T\$ , the number of test cases.
  Each of the subsequent \$T\$ lines defines a test case as 2 space-separated integers, \$N\$ and \$K\$ , respectively.

I created the program which I know is not refined but passed every test case. I want to know that does my code comes under bad programming practices.

import java.io.*;
import java.util.*;

public class Solution {

public static void main(String[] args) {
        /* Enter your code here. Read input from STDIN. Print output to STDOUT. Your class should be named Solution. */
        Scanner in = new Scanner(System.in);
        int T = in.nextInt();
        int k[] = new int [T];
        int n[]  = new int [T];
        for(int  i= 0;i<T;i++){
         n[i] = in.nextInt();
         k[i]=in.nextInt();
        }
        for(int karr =0 , narr=0;karr<k.length&& narr<n.length;karr++,narr++){
             int  a[] = new int [n[narr]];
             int max =0;
             for (int i=0;i<n[narr];i++){
                 a[i]= i+1;
             }
             for(int i=0;i<a.length;i++){
                 for(int j=1;j<a.length;j++){
                     if(a[i]<a[j]){
                     int andvalue = a[i]&a[j];
                         int temp = max ;
                     if( andvalue<k[karr] ){
                         if(andvalue>temp){
                            max = andvalue;
                         }}

                     }

                 }
             }
             System.out.println(max);
        }

    }
}

I think your code is difficult to read/understand because of:

Inexpressive or confusing naming
Lack of any documentation
Confusing indentation and spacing. Consider using an online code beautifier

Let's start from the top.

int T = in.nextInt();
int k[] = new int [T];
int n[]  = new int [T];
for(int  i= 0;i<T;i++){
 n[i] = in.nextInt();
 k[i]=in.nextInt();
}

Mathematical notation doesn't necessarily translate very well to code, where mathematical notation is meant to represent complex concepts in short form, code is meant to be concrete applications of concepts in a way that computers can process, and humans can hopefully understand.

I did a bit of renaming to make these clearer. Note that using underscore_case is not typical for Java naming, but I think casesN or nCases didn't read well at all.

int numCases = in.nextInt();
int[] cases_N = new int[numCases];
int[] cases_K = new int[numCases];
// Populate the cases from STDIN
for (int i = 0; i < numCases; i++) {
    cases_N[i] = in.nextInt();
    cases_K[i] = in.nextInt();
}

This for loop's iterator naming is misleading:

for(int karr =0 , narr=0;karr<k.length&& narr<n.length;karr++,narr++)

In reality, karr and narr are not arrays, so let's not call them arr. Furthermore, since they are always only ever incremented together, and you already know you'll have exactly the same number of \$K\$ and \$N\$ cases, there's really no point in having both.

Also, you already have a value \$T\$ (or numCases) so there is no need to get the length of the arrays.

for (int caseNum = 0; caseNum < numCases; caseNum++)

I also replaced each other occurrence of those two variables with caseNum.

This kind of reference is used a lot inside that big loop: cases_N[caseNum] and cases_K[caseNum]. Now would be a good time to just declare N and K with those values.

This makes the next section make more sense:

int N = cases_N[caseNum];
int K = cases_K[caseNum];
int a[] = new int[N];
for (int i = 0; i < N; i++) {
    a[i] = i + 1;
}

Now we can give this a array a meaningful name: intsUpToN

int[] intsUpToN = new int[N];
for (int i = 0; i < N; i++) {
    intsUpToN[i] = i + 1;
}

We can also replace the two instances of intsUpToN.length, since that value is already known (it's the current N value) and we are not adding or removing anything from the array.

The code is already much easier to understand now. I added little snippets of code comments to make it more clear. I also combined the last two if checks into one with &&.

I removed temp because it is assigned the value of max and never used for anything except comparison, so we can just use max in the comparison instead.

Note that I did not address the algorithm, as algorithms are not my forte. It is clearly a brute force algorithm, and chances are someone who is good with mathematics can suggest a more efficient algorithm.

The resulting code passes all tests cases.

public class Solution {

    public static void main(String[] args) {

        Scanner in = new Scanner(System.in);
        int numCases = in.nextInt();
        int[] cases_N = new int[numCases];
        int[] cases_K = new int[numCases];
        // Populate the cases from STDIN
        for (int i = 0; i < numCases; i++) {
            cases_N[i] = in.nextInt();
            cases_K[i] = in.nextInt();
        }
        // Outer loop for each case
        for (int caseNum = 0; caseNum < numCases; caseNum++) {
            int N = cases_N[caseNum];
            int K = cases_K[caseNum];
            // Fill in array integers from 1 to N
            int[] intsUpToN = new int[N];
            for (int i = 0; i < N; i++) {
                intsUpToN[i] = i + 1;
            }
            // Brute force method to logically "AND" all possible values,
            // returning with the maximum "AND" result at the end
            int max = 0;
            int andValue = 0;
            for (int i = 0; i < N; i++) {
                for (int j = 1; j < N; j++) {
                    if (intsUpToN[i] < intsUpToN[j]) {
                        // This is the logical "AND" calculation
                        andValue = intsUpToN[i] & intsUpToN[j];
                        if (andValue < K && andValue > max) {
                            max = andValue;
                        }
                    }
                }
            }
            System.out.println(max);
        }
    }
}


Rspec test for Rails method to return alphabetically sorted list of tags I have one model class as listed here:

class Post < ActiveRecord::Base
  has_many :comments

  def tags
    comments.map(&:tag).uniq.sort
  end
end

and below is the best rspec as per my understanding:

context 'tags' do
  let(:post) { Post.find_by_code('TX') }

  it 'returns sorted alphabetically list of available tags' do
    expect(post.tags).to eq(post.comments.map(&:tag).uniq.sort)
  end
end

Do we any better way to test it or even it is nor require to test.

Your test code is merely a copy of your model code, so you're not really testing anything.

Rather, your unit test should rely on fixtures, and the expected answer should be hard-coded in the test.


Place div's of different heights in a responsive parent div Can somebody help me with optimizing this code? I'd like to have some control about placement. Masonry doesn't do the job in that way.

What I have done now is to create some sets of columns. For different screen width one of the column sets will be active. Big screens have 3 columns middle screens 2 and small screens one one column. The columns are filled with several divs with the same width but different heights.

In this case the html loads double content, which is turned off by css.

Can somebody help me to find a way to avoid double content?

At the moment of writing it is in use by [www.pcsupportaanhuis.nl][1]

 #l_wrapper_topic_left {
    margin: 0;
    margin-right: 2%;
    padding: 0;
    width: 32%;
    float:left;
    overflow: auto;
    clear: both;
    display: inline;
    }

   #l_wrapper_topic_middle {
    margin: 0;
    margin-right: 2%;
    padding: 0;
    width: 32%;
    float:left;
    overflow: auto;
    display: inline;
   }

   #l_wrapper_topic_right {
    margin: 0;
    padding: 0;
    width: 32%;
    float: right;
    overflow: auto;
    display: inline;
  }

   #m_wrapper_topic_left {
    display: none;
  }

  #m_wrapper_topic_right {
    display: none;
  }

   #s_wrapper_topic {
    display: none;
  }

   .incolumn {
    margin-bottom: 4%;
  }

  @media only screen and (max-height: 480px)
    and (max-width: 531px) and (orientation: landscape) {
    #m_wrapper_topic_left {
        margin: 0% 2% 0% 0%;
        padding: 0;
        width: 49%;
        float:left;
        overflow: auto;
        clear: both;
        display: inline;
      }

         #m_wrapper_topic_right {
          margin: 0;
         padding: 0;
        width: 49%;
        float: right;
        overflow: auto;
        display: inline;
       }

       #l_wrapper_topic_left {
         display: none;
        }

       #l_wrapper_topic_middle {
        display: none;
       }

       #l_wrapper_topic_right {
        display: none;
    }

      .incolumn {
        margin-bottom: 4%;
      }
   }

   <?php include('topics/t1.php'); ?>

   <?php include('topics/t4.php'); ?>

   <?php include('topics/t2.php'); ?>

   <?php include('topics/t3.php'); ?>

   <?php include('topics/t1.php'); ?>

   <?php include('topics/t3.php'); ?>

   <?php include('topics/t4.php'); ?>

   <?php include('topics/t2.php'); ?>

   <?php include('topics/t1.php'); ?>

   <?php include('topics/t2.php'); ?>

   <?php include('topics/t3.php'); ?>

   <?php include('topics/t4.php'); ?>

Introducing... CSS Flexbox (Tada)!

The markup.

<?php include('topics/t1.php'); ?>

<?php include('topics/t2.php'); ?>

<?php include('topics/t3.php'); ?>

<?php include('topics/t4.php'); ?>

We'll set #topics-wrapper to be our flexbox container. Put space between children when necessary, and allow wrapping.

#topics-wrapper {
    display: flex;
    justify-content: space-between;
    flex-wrap: wrap;
}

Hide the ghost element for now, it'll come in handy later.

.flex-ghost { display: none; }

Normally, we want children to be 32% in width.

#topics-wrapper > div {
    flex-basis: 32%;
}

On medium screens, switch to 49%.

@media (...) {
    #topics-wrapper > div {
        flex-basis: 48%;
    }
}

Go all out on small screens.

@media (...) {
    #topics-wrapper > div {
        flex-basis: 100%;
    }
}

Now, we want column 4 on the left on medium screens, so in your medium media query, allow the ghost to fill the space it would take.

.flex-ghost { display: block; }

You'll never completely be able to imitate Masonry, but hopefully I've given you a good start. Especially experiment with flex-direction and flex-wrap. Learn Flexbox, it'll change the way you write CSS forever!