High school social network query for friends in common This is sourced from the Stanford Coursera self study DB class SQL quizzes:

  Students at your hometown high school have decided to organize their
  social network using databases. So far, they have collected
  information about sixteen students in four grades, 9-12. Here's the
  schema:

Highschooler (ID, name, grade)

  English: There is a high school student with unique ID and a given
  first name in a certain grade.

Friend (ID1, ID2)

  English: The student with ID1 is friends with the student with ID2.
  Friendship is mutual, so if (123, 456) is in the Friend table, so is
  (456, 123).

Likes (ID1, ID2)

  English: The student with ID1 likes the student with ID2. Liking
  someone is not necessarily mutual, so if (123, 456) is in the Likes
  table, there is no guarantee that (456, 123) is also present.

  Database
  here

  Prompt: For each student A who likes a student B where the two are not friends, find if they have a friend C in common (who can introduce
  them!). For all such trios, return the name and grade of A, B, and C.

I would appreciate a brief code review of this working query:

select distinct a.name, a.grade, b.name, b.grade, c.name, c.grade
from highschooler a, highschooler b, highschooler c, likes, friend
where a.id=likes.id1
and b.id=likes.id2
and a.id=friend.id1
and b.id not in (select id2 from friend where a.id=id1)
and b.id in (select id1 from friend where id2=c.id)
and a.id in (select id1 from friend where id2=c.id)

Generic issues

The outline of the query should be made visible at a glance using indentation.  This SELECT has a FROM clause and a WHERE clause.  Within the WHERE clause, there are multiple ANDed conditions.
It is conventional to use ALL CAPS for SQL keywords (though some people dislike that convention and disregard it).
All selected columns should be given their own distinct names.  Otherwise, MySQL will automatically name them, and in this case, you'll get three columns each called name and grade.
Use the same capitalization for table and column names as given in the question, since MySQL identifiers are case-sensitive on some platforms.

Query formulation

SELECT DISTINCT should be avoided wherever possible.  Deduplicating the results could hurt performance, and is a sign of a "wrong" query.  A properly formulated query based on a well designed schema and accurate data should almost never require deduplicating the results.
The a-b and b-c friendship relationships should be symmetrical:

$$\newcommand{rel}[2]{\overset{\mathtt{#1}}{\underset{\mathtt{#2}}{\longleftrightarrow}}}
  \fbox{a} \rel{Friend}{ac} \fbox{c} \rel{Friend}{bc} \fbox{b}
  $$

Therefore, I would expect two Friend tables included in the join on equal footing.
In fact, I'm not convinced that your query is correct.  You have the condition a.id=friend.id1, but there is no mention of friend.id2 at all.  Your query merely specifies that a is not friendless.
It should be possible to formulate the join without so many subselects.

Here's one way to write it:

SELECT a.name AS a_name, a.grade AS a_grade
     , b.name AS b_name, b.grade AS b_grade
     , c.name AS c_name, c.grade AS c_grade
    FROM Highschooler a, Highschooler b, Highschooler c, Likes, Friend ac, Friend bc
    WHERE
        a.ID = Likes.ID1 AND b.ID = Likes.ID2
        AND a.ID = ac.ID1 AND ac.ID2 = bc.ID1 AND b.ID = bc.ID2
        AND a.ID = ac.ID1
        AND b.ID = ac.ID2
        AND c.ID = bc.ID2
        AND NOT EXISTS (
            SELECT *
                FROM Friend ab
                WHERE ab.ID1 = a.ID AND ab.ID2 = b.ID
        );

However, joins would be better written using the INNER JOIN syntax:

SELECT a.name AS a_name, a.grade AS a_grade
     , b.name AS b_name, b.grade AS b_grade
     , c.name AS c_name, c.grade AS c_grade
    FROM Likes
        INNER JOIN Friend AS ac
            ON Likes.ID1 = ac.ID1
        INNER JOIN Friend AS bc
            ON Likes.ID2 = bc.ID2
        INNER JOIN Highschooler AS a
            ON ac.ID1 = a.ID
        INNER JOIN Highschooler AS b
            ON bc.ID2 = b.ID
        INNER JOIN Highschooler AS c
            ON ac.ID2 = c.ID
    WHERE
        ac.ID2 = bc.ID1
        AND NOT EXISTS (
            SELECT *
                FROM Friend AS ab
                WHERE ab.ID1 = a.ID AND ab.ID2 = b.ID
        );


Searching for keywords in dataframe Given a dataframe with three columns of text blobs to search through, which can be found in this Gist.

And three keywords that I want to identify in this text:

branches_of_sci = ['bio', 'chem', 'physics']

I've written the following code to identify the presence of these keywords:

dfq_col = ['Text A', 'Text B', 'Text C']

for branch in branches_of_sci:
    for col in dfq_col:
        temp_list = []
        for row in df[col]:
            if type(row) is not str:
                temp_list.append(False)
            elif type(row) is str:
                temp_list.append(row.find(branch)>0)
        df[branch] |= temp_list

This is the result of the data I linked to:

I think the main problem here is that I'm using a for-loop when I should be using some sort of dataframe-specific function, but I'm not sure how to restructure the code to accomplish this.

import pandas as pd

df = pd.read_clipboard(sep=',') # copied data from the gist

branches_of_sci = ['bio', 'chem', 'physics']

for branch in branches_of_sci:
    df[branch] = df.astype(str).sum(axis=1).str.contains(branch)

In my limited experience, for loops are almost always wrong when using Pandas.  The primary benefit of Pandas is vectorization, so using the built-in methods is typically best.

Here is a breakdown of the main function:

df[branch]  creates a new dataframe column
df.astype(str) converts all of the dtypes in the dataframe to strings
.sum(axis=1) concatenates all dataframe columns horizontally (i.e. axis=1)
.str.contains() use built-in string search (see docs)

Hopefully that helps.


Optimizing a particle filter weighted selection step I have a vector with 10000 particles that I use in a Particle Filter. The problem is that it's taking too long to compute the weighted selection of the particles.

I want to use a vector of weights (probabilities) to select elements from a vector (particles) to generate a new vector (new_particles) with the same size as the original, which has the most random items from the original set based on their weights.

Is there any way I can optimize this?

// util::NUMBER_OF_PARTICLES is a const int equal to 10000

// vector that holds particles weights
std::vector< double > probabilities;
probabilities.reserve(util::NUMBER_OF_PARTICLES);

// vector that will hold new particles
std::vector< GameParticle > new_particles;
new_particles.reserve(util::NUMBER_OF_PARTICLES);

// sum of all particle's weights
double sum_prob_all_particles = 0;

// calculate particle weights and save in vector
for(std::vector< GameParticle >::iterator it = game_particles_.begin(); it != game_particles_.end(); ++it)
{
    double probability = util::getProbOfMeasurementGivenParticle(*it, measurement);
    probabilities.push_back(probability);
    sum_prob_all_particles += probability;
}

ROS_ERROR_STREAM_COND(sum_prob_all_particles == 0, "Error, all particles have a zero probability of being correct");

// select new particle group
// counter used to count number of selected particles
int particlesCounter = util::NUMBER_OF_PARTICLES;
// random variable used to select particle value from 0 to sum_prob_all_particles
double random_variable = ( std::rand() / (double) RAND_MAX ) * sum_prob_all_particles;
double sum_probs = 0;

// counter of current particle to select
int counter = util::NUMBER_OF_PARTICLES - 1;

// while there are particles to select
while (particlesCounter > 0)
{
    // loops through all old particles and their weights
    int counter = util::NUMBER_OF_PARTICLES - 1;
    for(std::vector< double >::reverse_iterator it = probabilities.rbegin(); it != probabilities.rend(); ++it)
    {
        sum_probs += (double) *it;
        // if random variable selected this weight, get this particle
        while(sum_probs >= random_variable)
        {
            particlesCounter--;
            new_particles.push_back(game_particles_[counter]);
            random_variable += ( std::rand() / (double) RAND_MAX ) * sum_prob_all_particles;

            // get out of loops if no more particles
            if(particlesCounter == 0)
                goto AFTER_PARTICLES_SELECTED;
        }
        counter--;
    }
}

AFTER_PARTICLES_SELECTED:

ROS_INFO_STREAM("Observation step complete");

The part which is actually taking too long is after the // select new particle group comment. It takes an average of 0.984703 seconds for 5000 particles and 3.89787 seconds for 10000 particles, which is just too much.

The part before that comment only takes an average of 0.00142544 seconds for 10000 particles, so it's good enough.

I don't see any need for that goto, and it should be avoided whenever possible.  It would be better to just perform the action after the condition, then break from the loop.

if (particlesCounter == 0)
{
    ROS_INFO_STREAM("Observation step complete");
    break;
}

Although this will only break from the for loop, the state of particlesCounter at that point will stop the while loop as well.


Critique of LinkedList class I'm working my way through The Java Programming Language, 4th edition.  This is exercise 2.2:

  Write a LinkedList class that has a field of type Object and a reference to the next LinkedList element in the list.

Is this an adequate solution? Is there a more efficient way to build up a test list for this example?

class Node {
    private Object data;
    private Node next;

    public Node(Object data) {
        this(data, null);
    }

    public Node(Object data, Node next) {
        this.data = data;
        this.next = next;
    }

    public void setData(Object data) {
        this.data = data;
    }

    public Object getData() {
        return data;
    }

    public Node getNext() {
        return next;
    }
}

class LinkedListTest {

    public static void main(String[] args) {

        Node node3 = new Node("puke");
        Node node2 = new Node("vomit", node3);
        Node node1 = new Node("blah", node2);

        Node node = node1;
        while (node != null) {
            System.out.println("Data = " + node.getData());
            node = node.getNext();
        }
    }
}

I would say it is an adequate solution. The product may not feel very useful yet, but it's probably not supposed to be. In some sense, the exercise is formulated ambiguously. Your Node class can be used to construct linked lists and as such is a building block of instantiated linked lists. However, I would not consider that a LinkedList implementation. A LinkedList implementation would consist of a wrapper class that hides this Node from the user and allows you to say thinks like

List list = new LinkedList();
list.add("foo");
list.add("bar");
list.remove("foo");
int index = list.indexOf("bar");
etc.

It may seem somewhat unsatisfactory that you have to construct your test list so clumsily, but ease of use wasn't a goal of the exercise ;)


Read employee number and salary (exercise to use static keyword) WAP to create a class called Employee having empid and salary as data members and salary as data members. Create objects of this class and count no. of employees working in an organization using static keyword.

This is my solution for the above question. Is it correct way of doing this? I am a beginner so I would appreciate your help. Let me know of my mistakes.

    import java.util.*;
    class Employee{
private int empid;
private int salary;
private static int count;

private void setValue(int empid,int salary){
this.empid=empid;
this.salary=salary;
count++;
}

private void getValue(){
System.out.println("EmpId = "+empid);
System.out.println("Salary ="+salary);
System.out.println("Number of employees: = "+count);
}

public static void main(String[] args)
{
    Scanner kb=new Scanner(System.in);
    Employee e=new Employee();
    int id,sal;
    System.out.println("Enter Employee Id: ");
    id=kb.nextInt();
    System.out.println("Enter Salary: ");
    sal=kb.nextInt();
    System.out.println("The Values entered are :");
    e.setValue(id,sal);
    e.getValue();
    Employee e1=new Employee();
    System.out.println("Enter Employee Id: ");
    id=kb.nextInt();
    System.out.println("Enter Salary: ");
    sal=kb.nextInt();
    System.out.println("The Values entered are :");
    e1.setValue(id,sal);
    e1.getValue();
}}

No.

You use a setter to modify an existing employee. But it makes no sense to change the id and salary together. In fact, you should never change the id of an employee.

This should be the constructor of the class instead. In your main I would expect to see the folowing flow:

Scanner kb=new Scanner(System.in);
int id,sal;
System.out.println("Enter Employee Id: ");
id=kb.nextInt();
System.out.println("Enter Salary: ");
sal=kb.nextInt();
System.out.println("The Values entered are :");
Employee e=new Employee(id, sal);

A second thing to note is that a method with prefix "get" is expected to return some value of the object. Either rename that getValue() method to something like printInfo() or provide actual getters:

public String getId() {
    return id;
}

public static int getNbOfEmployees(){
    return count;
}

These can then be used somewhere else to print out the info, for example in your main:

System.out.println("EmpId = " + e.getId());
System.out.println("Salary =" + e.alary());
System.out.println("Number of employees: = " + Employee.getNbOfEmployees());


The best approach to validate fields I wonder if there is a better approach to validate the object fields. For specific types of objects some fields cannot be empty:
public class CredentialsValidator {

  public boolean validateCredentialsRequest(ValidationCredentialsRequest credentialsRequest) {
    switch (credentialsRequest.getCredentialType()) {
      case TYPEA:
        return validatetypeACredentials(credentialsRequest);
      case TYPEB:
        return validateTypeBCredentials(credentialsRequest);
      case TYPEC:
        return validateTypeCCredentials(credentialsRequest);
    }
    return true;
  }

  private boolean validateTypeCCredentials(ValidationCredentialsRequest credentials) {
    if (credentials.getIsNewCredential()) {
      return nonNull(credentials.getUsername())
          && !credentials.getUsername().isEmpty()
          && nonNull(credentials.getPassword())
          && !credentials.getPassword().isEmpty();
    }
    return nonNull(credentials.getCredentialsId());
  }

  private boolean validateTypeBCredentials(ValidationCredentialsRequest credentials) {
    if (credentials.getIsNewCredential()) {
      return nonNull(credentials.getHostname())
          && !credentials.getHostname().isEmpty()
          && nonNull(credentials.getPort())
          && !credentials.getPort().isEmpty()
          && nonNull(credentials.getUsername())
          && !credentials.getUsername().isEmpty()
          && nonNull(credentials.getPassword())
          && !credentials.getPassword().isEmpty();
    }
    return nonNull(credentials.getCredentialsId())
        && nonNull(credentials.getHostname())
        && !credentials.getHostname().isEmpty()
        && nonNull(credentials.getPort())
        && !credentials.getPort().isEmpty();
  }

  private boolean validatetypeACredentials(ValidationCredentialsRequest credentials) {
    if (credentials.getIsNewCredential()) {
      return (nonNull(credentials.getDomain())
              && !credentials.getDomain().isEmpty()
              && nonNull(credentials.getUsername())
              && !credentials.getUsername().isEmpty()
              && nonNull(credentials.getPassword())
              && !credentials.getPassword().isEmpty())
          || (nonNull(credentials.getSharePoint().getUsername())
              && !credentials.getSharePoint().getUsername().isEmpty()
              && nonNull(credentials.getSharePoint().getPassword())
              && !credentials.getSharePoint().getPassword().isEmpty());
    }
    return nonNull(credentials.getCredentialsId())
        && nonNull(credentials.getDomain())
        && !credentials.getDomain().isEmpty();
  }

My main class that invokes Validator:
public Mono<ValidationCredentialsResponse> validateCredentials(
      Mono<ValidationCredentialsRequest> validationCredentials) {
    return validationCredentials
        .filter(credentialsValidator::validateCredentialsRequest)
        .switchIfEmpty(Mono.error(new InvalidCredentialsException(MESSAGE)))
        .flatMap(
            validCredentials ->
                securityService

...
...
I use Spring Boot and Webflux for reactive programming

From an object-oriented point of view there is a better way you can handle this scenario.
Feature Envy
Currently the code snipped has a Code Smell that is called Feature Envy:

The whole point of objects is that they are a technique to package data with the processes used on that data. A classic [code] smell is a method that seems more interested in a class other than the one it is in. The most common focus of the envy is the data.
— Martin Fowler, Refactoring: Improving the Design of Existing Code, Chap. 3, p. 80

The class CredentialsValidator only interacts with data from ValidationCredentialsRequest by accessing it via all the getters:

nonNull(credentials.getUsername())
&& !credentials.getHostname().isEmpty()
/* ... */

Instead, this logic could be in ValidationCredentialsRequest, and CredentialsValidator only interacts with the validation methods instead of the getters:

public boolean validateCredentialsRequest(ValidationCredentialsRequest credentialsRequest) {
   switch (credentialsRequest.getCredentialType()) {
       case TYPEA:
           return credentialsRequest.isTypeAValid();
       case TYPEB:
           return credentialsRequest.isTypeBValid();
       case TYPEC:
           return credentialsRequest.isTypeCValid();
   }
   return true;
}

class ValidationCredentialsRequest {
   /* ... */

   public boolean isTypeAValid() {/* ... */ }

   public boolean isTypeBValid() {/* ... */ }

   public boolean isTypeCValid() {
       if (newCredential) {
           return nonNull(username)
               && !username().isEmpty()
               && nonNull(password)
               && !password.isEmpty();
       }
       return nonNull(credentialsId);
   }

}

Polymorphism
Polymorphism is a concept where one thing can be something else at the same time: for example, a Shape can be a Rectangle at the same time.
With Polymorphism we can achieve that we no longer need the switch case. This would have the advantage that we don't have to change the CredentialsValidator class when adding new validation logic:
class CredentialsValidator {

    public boolean validateCredentialsRequest(ValidationCredentialsRequest credentialsRequest) {
        return credentialsRequest.isValid();
    }

}

From here all different "shapes" of ValidationCredentialsRequest need to share the same interface - all need the isValid method:
abstract class ValidationCredentialsRequest {
    /* ... */
    abstract boolen isValid();
}

class TypeARequest extends ValidationCredentialsRequest {
    /* ... */
    public boolean isValid() { /* ... */ }
}

class TypeBRequest extends ValidationCredentialsRequest {
    /* ... */
    public boolean isValid() { /* ... */ }
}

class TypeCRequest extends ValidationCredentialsRequest {
    /* ... */
    public boolean isValid() {
        if (newCredential) {
            return nonNull(username)
                && !username().isEmpty()
                && nonNull(password)
                && !password.isEmpty();
        }
        return nonNull(credentialsId);
    }
}

Now when a new kind of ValidationCredentialsRequest is needed, we just have to create a new "form" of it, and it works without changing the CredentialsValidator.
Edit
Use in Main
In the code for the main you provided nothing needs to be changed.
But if you would like to add the Polymorphism to your code, you need to change it where you create the ValidationCredentialsRequest objects. There you would create the different "shapes" of ValidationCredentialsRequest instead of ValidationCredentialsRequest itself.


State Pattern for traffic lights I tried to implement a traffic light system using state pattern, please comment on the OOP/design pattern use.

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace DesignPatternsQuestions
{
    /// <summary>
    /// implement a traffic light system
    /// </summary>
    [TestClass]
    public class StatePatternTest
    {
        [TestMethod]
        public void StateTrafficLightTest()
        {
            TrafficLight light = new TrafficLight(new RedState());
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State,typeof(GreenState));
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State, typeof(YellowState));
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State, typeof(RedState));
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State, typeof(GreenState));
        }
    }

    public class TrafficLight
    {
        private State _state;

        public TrafficLight(State state)
        {
            _state = state;
        }
        // Gets or sets the state

        public State State
        {
            get { return _state; }
            set
            {
                _state = value;
                Console.WriteLine("State: " +
                  _state.GetType().Name);
            }
        }
        //this is the trickS
        public void ChangeLight()
        {
            _state.SetState(this);
        }
    }

    public abstract class State
    {
        public abstract void SetState(TrafficLight light);
    }

    public class RedState: State
    {
        public override void SetState(TrafficLight light)
        {
            light.State = new GreenState();
        }
    }

    public class YellowState : State
    {
        public override void SetState(TrafficLight light)
        {
            light.State = new RedState();
        }
    }

    public class GreenState : State
    {
        public override void SetState(TrafficLight light)
        {
            light.State = new YellowState();
        }
    }
}

Design

This sort of design isn't necessarily a bad thing, but in this example it's making things a lot more complicated, without providing any benefits:

ChangeLight suggests that null is not a valid state, but your TrafficLight class does not prevent its state from being initialized or set to null.
It's possible to create classes that inherit from State or one of your other state classes. This essentially means that there are an infinite number of possible states (PurpleState, RandomState, SchrodingerCatState, ...). That makes it unclear how exactly your traffic light is meant to be used.
Those state classes don't contain any actual state - it's only their type (and functionality) that matters. In that case, you might as well create static instances that can be reused. And because they all contain essentially the same logic (just different values), you don't really need separate classes either.
Methods like AllowTraffic and StopTraffic(TimeSpan delay) are probably more useful than a ChangeLight method that doesn't say anything about what the next state will be. But supporting such methods with logic inside those state classes alone is cumbersome.

Using a simple enum instead makes things a lot easier: there's no null to worry about and it's immediately clear what all the possible states are. The light-changing logic is fairly simple, so that can easily be kept in TrafficLight itself.

Further considerations

Different countries use different light patterns, and traffic lights may have different modes (such as daylight versus night, or an error state). In other words: red is not always followed by green - in some countries there's an orange-red state in-between, in some modes a light is flashing red continually, and so on.

With such requirements, the current light doesn't tell us everything about the traffic light's internal state. It would be more useful to treat each pattern as a different state. But even then I wouldn't create separate state classes with their own logic: a single data-driven class and some state-tracking logic inside TrafficLight itself should be sufficient.

Other notes

That Console.WriteLine is obviously some left-over test code, but be careful not to leave it lingering around. There's no reason why this code should depend on the presence of a console. If you're using Visual Studio, you may want to use tracepoints instead (breakpoints with a logging action). They're more flexible and don't clutter the code.
Comments like // Gets or sets the state are of no use: they're just stating the obvious, and they can only cause confusion when they become out-of-date.
State is a very generic name - TrafficLightState or TrafficLightColor may be more appropriate here.


Entity management design & smart pointers I'd like to get some guidance on whether or not I'm approaching my design the correct way, especially with regards to usage of smart pointers and references.

I've started developing a small game engine which among other things consists of Entities, an EntityManager to 'own' the entities, a Quadtree for 2d entity position lookups which will be regenerated each update and a World to encapsulate manager and tree. Here are the relevant bits:

World:

EntityManager mEntityManager;
Quadtree<Entity&> mTree;

void World::Init() {
    // Add some initial entities
    for( int i=0 ; i < 200; i++ ) {
        std::unique_ptr<Entity> entity( new Entity(*this) );
        mEntityManager.AddEntity(&entity);
    }
}

void World::Update() {
    // Rebuild quadtree and update entities
    mTree.clear();
    for( const auto &i : mEntityManager.getEntities() ) {
        mTree.insert(*i.get(), i->position().x, i->position().z );
    }
    mEntityManager.update();
}

EntityManager:

std::vector<std::unique_ptr<Entity>> mEntities;

const std::vector<std::unique_ptr<Entity>> &getEntities(){ return mEntities; }

void EntityManager::AddEntity(std::unique_ptr<Entity>* entity) {
    mEntities.push_back(std::move(*entity));
}

Entity:

World&      mWorld;

Entity(World& world) : mWorld( world ) { };

void Entity::update() {
    // Call world to retrieve a list of
    for( const auto &i : world.getNearbyEntities( this->position, radius ) ) {
        // etc...
    }
}

All currently seems to works fine, but I'd just like to know if I'm going about this the right way or if I'm lining myself up for problems in the future based on how I'm defining/passing data.

EDIT
Okay, big thanks @Morwenn . How does this look, assuming I want to create a new instance and then perhaps perform some operations on the newly created entity. Anything I've misunderstood?

EntityManager:

template<typename Derived, typename... Args>
Derived& EntityManager::EmplaceEntity(Args&&... args) {
    std::unique_ptr<Entity> ptr( new Derived{std::forward<Args>(args)...} );
    mEntities.push_back(std::move(ptr));
    return dynamic_cast<Derived&>(*(mEntities.back()));
}

World:

for( int i=0 ; i < 200; i++ ) {
    BlueEntity& entity = mEntityManager.EmplaceEntity<BlueEntity>(*this);
    entity.setPosition(x, y);
}

The fact that AddEntity leaves behind a unique_ptr to a possibly destroyed entity bothers me (right now, moving your entity does not "destroy" it, but you might add destructible components later). In my opinion, AddEntity should not take its argument by pointer but by reference:

void EntityManager::AddEntity(const Entity& entity) {
    mEntities.push_back(entity);
}

You can provide an overload for rvalue-references to move entities that are meant to disappear anyway though:

void EntityManager::AddEntity(Entity&& entity) {
    mEntities.push_back(std::move(entity));
}

You probably didn't care because in your use case, you passed temporary variables that are destroyed right after having been created. But what you might actually want to do is to create the entities directly in the vector insteas of creating them, then adding them to the vector. Therefore, what you really need is an emplace_back function:

template<typename... Args>
void EntityManager::EmplaceEntity(Args&&... args) {
    mEntities.emplace_back(std::forward<Args>(args)...);
}

With such a function, you can init your world like this:

void World::Init() {
    // Add some initial entities
    for( int i=0 ; i < 200; i++ ) {
        mEntityManager.EmplaceEntity(*this);
    }
}

EDIT: Answering your comment. If Entity is an abstract class and you only add instances of derived classes, then I think that push_back still works fine since polymorphism will be achieved via reference semantics. On the other hand, EmplaceEntity could take another argument if you want to emplace instances of classes derived from Entity:

template<typename Derived, typename... Args>
void EntityManager::EmplaceEntity(Args&&... args) {
    std::unique_ptr<Entity> ptr( new Derived{std::forward<Args>(args)...} );
    mEntities.push_back(std::move(ptr));
}

I did not test the code above, but theorically it should. That allows you to create instances of classes derived from Entity directly without having to bother with the pointers. Here is how you could use it:

mEntityManager.EmplaceEntity<BlueEntity>(*this);

Anyway, whenever possible, try to use reference smantics and to hide pointer semantics inside the classes so that the users of the classes don't have to bother with the pointers.


Iterator Implementation I had to write my own iterator implementation for my structure which has ArrayList<Vector> field. An iterator is supposed to iterate over mentioned List. Any suggestions on improving it, or anything else?

public class ExamplesIterator implements Iterator<Vector> {

    private List<Vector> examples;  //ArrayList<Vector> will be set here
    private int          index;

    public ExamplesIterator(List<Vector> examples) {
        this.examples = examples;
        index = 0;
    }

    @Override
    public Vector next() {
        if(hasNext()) {
            return examples.get(index++);
        } else {
            throw new NoSuchElementException("There are no elements size = " + examples.size());
        }
    }

    @Override
    public boolean hasNext() {
        return !(examples.size() == index);
    }

    @Override
    public void remove() {
        if(index <= 0) {
            throw new IllegalStateException("You can't delete element before first next() method call");
        }
        examples.remove(--index);
    }
}

I think your implementation is overall very good, two small comments:

Improving readability for return statement in hasNext to return examples.size() != index;
Making the examples field final: private final List<Vector> examples;

However, if the Vector class here is java.util.Vector you should know that it is considered deprecated in favor of the ArrayList class.

Also, since you create your iterator from a List<Vector> you could get an Iterator<Vector> by calling list.iterator();


Op-code decoding in an emulator I've written the larger part of an emulator for a 6502 CPU but my method of op-code decoding is giving me concern, mainly because of how long the method is getting but also because I need a variable for each code, I need to declare them three times and I've stuck them all in a very non-OO class.  I wouldn't mind some scathing critique.

I've written it in a TDD fashion (in an effort to understand pure TDD) which has led to the simplest implementation with refactors only when necessary.  Even though I think this is probably the most efficient solution (making full use of the benefits of a switch statement), I'm thinking there's maybe a better way.

Extract:-

public void step() {
    System.out.println("\n*** STEP >>>");

    int accumulatorBeforeOperation = registers.getRegister(Registers.REG_ACCUMULATOR);
    int opCode = nextProgramByte();

    //Execute the opcode
    System.out.println("Instruction: " + InstructionSet.getOpCodeName(opCode) + "...");
    switch (opCode){
        case InstructionSet.OP_ASL_A: {
            int newFakeByte = registers.getRegister(Registers.REG_ACCUMULATOR) << 1;
            setCarryFlagBasedOn(newFakeByte);
            registers.setRegisterAndFlags(Registers.REG_ACCUMULATOR, newFakeByte);
        }
        break;

        case InstructionSet.OP_ASL_Z: {
            int location = nextProgramByte();
            int newFakeByte = memory.getByte(location) << 1;
            setCarryFlagBasedOn(newFakeByte);
            registers.setFlagsBasedOn(newFakeByte);
            memory.setByteAt(location, newFakeByte);
        }
        break;
        ...
        case InstructionSet.OP_DEY:
            registers.decrementRegisterWithFlags(Registers.REG_Y_INDEX);
            break;

        case InstructionSet.OP_LDX_I:
            registers.setRegisterAndFlags(Registers.REG_X_INDEX, nextProgramByte());
            break;

        case InstructionSet.OP_LDX_Z:
            registers.setRegisterAndFlags(Registers.REG_X_INDEX, getByteOfMemoryAt(nextProgramByte()));
            break;
        ...
        default:
            throw new UnknownOpCodeException("Unknown 6502 OpCode:" + opCode + " encountered.", opCode);
    }

Thoughts

I've looked at storing op-codes as Enums but it means reading bytes (in Javas case this is int represented bytes) and writing conversion code from raw byte to Enum but that seems unnecessary for the small maintainability improvement.

It seems excessive and not very OO to do it with a class per op-code.

I figured a much nicer way to deal with this.  Break it down into the repeatable elements, i.e. Addressing Mode and Operation then code them all separately and immutably, combining them in the right way to get all OpCodes.  Effectively doing functional programming in Java.

First: If you represent an operation, say ASL(arithmatic shift left) as an Enum (Operation), which has a parameter that is a lamba of a @FunctionalInterface (in this case called AddressedValueInstruction) and accepts two bytes and returns a result.
 (Note: RoxByte is just my custom implementation of a byte)

public enum Operation implements AddressedValueInstruction {
/** Shift all bits in byte left by one place, setting flags based on the result */
    ASL((a,r,m,v) -> {
        final RoxByte newValue = a.asl(v);
        r.setFlagsBasedOn(newValue);
        return newValue;
    }),

    ...

    @Override
    public RoxByte perform(Mos6502Alu alu, Registers registers, Memory memory, RoxByte value) {
        return instruction.perform(alu, registers, memory, value);
    }

Second: If you represent the Addressing Mode as an Enum then -similar to Operation- have one of the Enum parameters as a @FunctionalInterface (called Addressable) lambda you can say MyEnum.address(AddressedValueInstruction) which will address a value, then sent it to the given operation then address the result.

public enum AddressingMode implements Addressable {
    /** Expects no argument, operation will be performed using the Accumulator Register*/
    ACCUMULATOR("Accumulator", 1, (r, m, a, i) -> {
        final RoxByte value = r.getRegister(Registers.Register.ACCUMULATOR);
        r.setRegister(Registers.Register.ACCUMULATOR, i.perform(a, r, m, value));
    }),

    ...

    @Override
    public void address(Registers r, Memory m, Mos6502Alu alu, AddressedValueInstruction instruction) {
        address.address(r, m, alu, instruction);
    }

Third: This means you can create opcodes in the same way by creating combinations of addressing modes and operations with no specific code required, i.e. ASL_ACCUMULATOR(ACCUMULATOR, ASL) and it can be run with ASL_ACCUMULATOR.perform()

public enum OpCode implements Mos6502Instruction {
    ASL_A(0x0A, ACCUMULATOR, ASL),
    ...

    @Override
    public void perform(Mos6502Alu alu, Registers registers, Memory memory) {
        addressingMode.address(registers, memory, alu, operation::perform);
    }

This approach has the benefits of a class per opcode approach, without all the boiler plate.


Check if n-th root of product of numbers is a whole number C++ I've got an assignment which sounds like this:

  Given a number \$m\$ and \$n\$ other numbers, say if the \$m\$-th root of the product of those numbers is a whole number (print 1) or not (print 0). If yes, also print, in sorted order, the result of the \$m\$-th root, decomposed in prime factors.

This is how the expression would look:

$$\sqrt[m]{x_1 x_2 x_3 \dots x_n}$$

My idea was this: for the result to be a whole number, if we were to decompose all of the numbers, then make the product of all of them, every power of every base has to be divisible with \$m\$.

This is what I came up with:

#include <iostream>
#include <algorithm>
#include <vector>
using namespace std;

using Prime_factorization = vector < pair<int, int> >;

void add_base_and_power(Prime_factorization& prime_factorization, int    base, int power)
{
    // Check if we've already got the base we're trying to add
    auto it = find_if(prime_factorization.begin(), prime_factorization.end(), [&](pair <int, int> p) { return p.first == base; } );

    if (it != prime_factorization.end()) // If yes, add the power to the existing base
        it->second += power;
    else
        prime_factorization.push_back(make_pair(base, power)); // Else, create a new base
}

void check_if_is_factor(Prime_factorization& prime_factorization, int &number, int divisor)
{
    if (number % divisor == 0) { // If the divisor is a base, we divide until it no longer can

        int power { 0 };
        while (number % divisor == 0) {
            number /= divisor;
            ++power;
        }

        // Add base and power to factorization
        add_base_and_power(prime_factorization, divisor, power);
    }
}

void decompose(Prime_factorization& prime_factorization, int number)
{
    check_if_is_factor(prime_factorization, number, 2); // Check if it divides by 2 so after that we can only check for uneven bases

    for (int div { 3 }; div * div <= number && number > 1; div += 2)
        check_if_is_factor(prime_factorization, number, div);

    // If number is bigger than 1 it means we got out of the previous for loop with the first condition, so the number must be prime
    if (number > 1)
        add_base_and_power(prime_factorization, number, 1);
}

int main() {

    // Get m-th root
    int m;
    cin >> m;

    // Get number of terms
    int n;
    cin >> n;

    // Take n numbers and decompose each of them
    Prime_factorization prime_factorization;
    for (int i { 1 }; i <= n; ++i) {

        int x;
        cin >> x;

        decompose(prime_factorization, x);
    }

    // After decomposing all of the numbers, we check to see if each power can be divided by m(the root)
    bool okay { true };
    for (auto& i : prime_factorization)
        if (i.second % m != 0) {
            okay = false;
            break;
        }

    // We must print the base-power combinations in sorted order
    sort(prime_factorization.begin(), prime_factorization.end(), [](pair<int, int> p1, pair<int, int> p2) { return p1.first < p2.first; });

    if (okay) {
        cout << 1 << '\n';

        for (auto& i : prime_factorization)
            cout << i.first << ' ' << (i.second / m) << '\n';
    } else
        cout << 0;
}

How could I improve my code? Is there any better algorithm you might've thought of?

The basic approach looks sensible, but I do have a few suggestions for improvements.

using namespace std;

Bad idea. It may be a nuisance having to prepend std:: everywhere, but it's a short name in order to reduce the nuisance, and the things which could go wrong are quite bad.

void add_base_and_power(Prime_factorization& prime_factorization, int    base, int power)

What happened to the whitespace in int    base? A single space suffices.

What do the names communicate and fail to communicate? The name prime_factorization communicates nothing: the type already says that. What purpose does this particular prime factorisation serve? The name base doesn't really fit the context IMO: you're not accumulating to a Base_power_pairs. add_prime_power(Prime_factorization accumulator, int prime, int power) would make it crystal clear what the function does.

void check_if_is_factor(Prime_factorization& prime_factorization, int &number, int divisor)

Again, names.

    if (number % divisor == 0) { // If the divisor is a base, we divide until it no longer can

This isn't wrong, but I wonder whether you made a conscious choice between this approach and

    int power { 0 };
    while (number % divisor == 0) {
        number /= divisor;
        ++power;
    }

    if (power > 0)
    {
        // Add base and power to factorization
        add_base_and_power(prime_factorization, divisor, power);
    }

I find the latter more aesthetically pleasing. It doesn't do a test which it knows will succeed.

NB this point is purely a subjective point about style - I'm not trying to microoptimise, and microoptimisations should be done with good benchmarks.

void decompose(Prime_factorization& prime_factorization, int number)

What is being decomposed here? My instinct would be that the first argument is the most relevant to interpreting the function name, but that doesn't make sense in this case.

    for (int div { 3 }; div * div <= number && number > 1; div += 2)

I've seen you use this int name { val } a few times, and (not being a C++ user) I'm not familiar with it. Does it have some advantage over int name = val to counter the disadvantage of requiring more effort to port to a different language?

There's a case to be made for calculating the square root of number outside the loop: firstly on micro-optimisation grounds, but secondly on correctness grounds. div * div could overflow the type.

    // We must print the base-power combinations in sorted order
    sort(prime_factorization.begin(), prime_factorization.end(), [](pair<int, int> p1, pair<int, int> p2) { return p1.first < p2.first; });

    if (okay) {
        cout << 1 << '\n';

        for (auto& i : prime_factorization)
            cout << i.first << ' ' << (i.second / m) << '\n';
    } else
        cout << 0;

Two things here: firstly, since the sorted values are only needed in one branch of the if it makes more sense to push the sort into that branch. Secondly,

if (test) {
    foo;
} else
    bar;

is easily the weirdest bracing style I've ever seen, and weird style is not good for collaboration. Do you use a company or published style guide?

Finally, my one algorithmic observation.

    // Check if we've already got the base we're trying to add
    auto it = find_if(prime_factorization.begin(), prime_factorization.end(), [&](pair <int, int> p) { return p.first == base; } );

    if (it != prime_factorization.end()) // If yes, add the power to the existing base
        it->second += power;
    else
        prime_factorization.push_back(make_pair(base, power)); // Else, create a new base

Linear search is never ideal. If you're using a recent standard, std::unordered_map is asymptotically preferable, and also IMO conveys the purpose better. Or maybe you would be better off with std::map, which would remove the need to sort at the end.


Implementing boost::upgrade_mutex using only standard locks Is this my_upgrade_mutex class a valid implementation of boost::upgrade_mutex semantics? (Ignoring the try_* and *_for/*_until part.)

class my_upgrade_mutex
{
    std::mutex xmutex;
    std::shared_mutex smutex;
public:
    void lock_shared()//u->s
    {
        smutex.lock_shared();
    }
    void unlock_shared()//s->u
    {
        smutex.unlock_shared();
    }
    void lock()//u->x
    {
        xmutex.lock();
        smutex.lock();
    }
    void unlock()//x->u
    {
        smutex.unlock();
        xmutex.unlock();
    }
    void lock_upgrade()//u->g
    {
        xmutex.lock();
        smutex.lock_shared();
    }
    void unlock_upgrade()//g->u
    {
        smutex.unlock_shared();
        xmutex.unlock;
    }
    void unlock_upgrade_and_lock()//g->x
    {
        smutex.unlock_shared();
        smutex.lock();
    }
    void unlock_and_lock_upgrade()//x->g
    {
        smutex.unlock();
        smutex.lock_shared();
    }
    void unlock_and_lock_shared()//x->s
    {
        smutex.unlock();
        smutex.lock_shared()
        xmutex.unlock();
    }
    void unlock_upgrade_and_lock_shared()//g->s
    {
        xmutex.unlock();
    }
};

I think it actually is valid.

Here's all of the possible states of the two mutexes.

/* u: unlocked, s: shared locked, g: upgrade locked, x: exclusive locked
   (state): of which upgrade_mutex state, the states are as this line
   (inter-state): in the middle of these transitions, the states might be as this line
                  Xab means the original state is of symbol X,
                      and the thread is doing the lock/unlock of a->b
   symbol xmutex smutex (state) (inter-state)
     A      u      u       u
     B      x      u             Aux Axu Aug Agu Agx Axg Axs Dgu Dgx Exu Exg Exs
     C      u      s       s     Cs
     D      x      s       g     Axs Bs Cux Cug Ds Dgu Dgx Exs
     E      x      x       x
*/

The 10 lock/unlock methods all behave reasonably good in all of the states
Given the prior-method state is in the set, the during-/post-method state is in the set


Efficiency and techniques for Matrix class that inherits from Python's list I have a class named Matrix, this class inherits from Python's list. The class currently can perform matrix multiplication, adding rows using list.extend, add a row using list.append, and also transpose.

The emphasis is on using only built-in tools.

I would like to know if this code can be made more efficient and readable.
Also, if there are alternative techniques to produce the same result.
a supplementary problem (more appropriate in StackOverflow) : I cannot use copy.deepcopy(A) with A a Matrix object. That's why i use res = Matrix(...) in the multiplication function.

Thanks.

Here is the code, break into three parts.

Global functions :

# Author: Arief Anbiya
# 11 February, 2018

def dot_product(u, v):
    return sum([i*j for i,j in zip(u,v)]

def multiplication(M,N):
    assert type(M)==Matrix or type(N)==Matrix
    res = None;
    if type(M)==type(N)==Matrix and M.ncol()==N.nrow():
        res = Matrix([[0 for i in range(N.ncol())] for j in range(M.nrow())]);
        for i in range(M.nrow()):
            for j in range(N.ncol()):
                res[i][j] = dot_product(M[i], [N[k][j] for k in range(N.nrow())]);
    elif (type(M)==int or type(M)==float):
        res = Matrix([[N[j][i] for i in range(N.ncol())] for j in range(N.nrow())]);
        for i in range(res.nrow()):
            for j in range(res.ncol()):
                res[i][j] *= M;
    elif (type(N)==int or type(N)==float):
        res = Matrix([[M[j][i] for i in range(M.ncol())] for j in range(M.nrow())]);
        for i in range(res.nrow()):
            for j in range(res.ncol()):
                res[i][j] *= N;
    else:
            raise TypeError("M and N should be either a compatible Matrix object, or a constant");
    return res

Class :

# Author: Arief Anbiya
# 11 February 2018
class Matrix(list):

    def __init__(self, the_list):
        super().__init__(the_list);

    def nrow(self):
        m = len(self);
        return m
    def ncol(self):
        n = len(self[0]);
        return n
    def get_dim(self):
        return (self.nrow(),self.ncol())

    def __add__(self, M):
        res = Matrix(self);
        for row in range(self.nrow()):
           dumrow = [self[row][col] + M[row][col] for col in range(self.ncol())];
           res[row]=dumrow;
        return res
    def __mul__(self, M):
        return multiplication(self, M);
    def __rmul__(self,M):
        return multiplication(M, self);

    def add_rows(self, rows):
        super().extend(rows);
    def append(self, row):
        try:
          sum(row); # Check if all numbers
          if len(row)<self.ncol():
             row.extend([0 for i in range(self.ncol()-len(row))]);
          elif len(row)>self.ncol():
             [row.pop() for i in range(len(row)-self.ncol())];
          super().append(row);
        except:
           raise AssertionError('Elements in row must be mumbers');

    def transpose(self):
        return Matrix([[row[i] for row in self] for i in range(self.ncol())]);

    def show(self):
        print("Printing matrix: ");
        for i in self:
           print(i);

Test for outputs :

A=Matrix([[1,2,3], [2,3,3]]);
A.show();
B = A+A;
B.show();
B.append([1,11,12])
B.show();
C = 3*B;
C.show();
D = A*B;
D.show()

This review will focus on using Python more effectively.  I will pull some examples from your Matrix class, and show those recast in a more Pythonic fashion.
Pep8
But before we start, you should consider formatting your code in accordance with pep8.  This is important when sharing code, as the consistent style makes it much easier for other programmers to read your code.  There are various tools available to assist in making the code pep8 compliant.  I use the PyCharm IDE which will show pep8 violations right in the editor.
No Semi Colons
On the same front, lose the semi colons, they are very rarely (never really) needed in Python.
Iterate directly
Python has many great ways to iterate. In general if you find yourself looping on an integer, like in the following code, there is a good chance you are doing it wrong. It is generally better to iterate on the data structure itself.  This example:
def __add__(self, M):
    res = Matrix(self);
    for row in range(self.nrow()):
        dumrow = [self[row][col] + M[row][col] for col in
                  range(self.ncol())];
        res[row] = dumrow;
    return res

Can be reduced to:
def __add__(self, M):
    return Matrix([[sum(x) for x in zip(*rows)] for rows in zip(self, M)])

Note that the lengths of the rows or columns are not even looked at in this code.  The underlying iterators take care of those details.
zip() is transpose
This hand coded transpose:
def transpose(self):
    return Matrix(
        [[row[i] for row in self] for i in range(self.ncol())]);

can be recast to:
@property
def transpose(self):
    return Matrix(map(list, zip(*self)))

zip() does the transpose, and the map(list(...)) converts the tuples from zip to lists. And...
@property make for a cleaner interface
Instead of:
def nrow(self):
    m = len(self);
    return m

Consider this instead:
@property
def nrow(self):
    return len(self)

It does two things:

Removes unneeded intermediate values
Use the @property decorator to allow calculated value to be returned without the need for the ().  This can provide a cleaner api.

Full Listing
class Matrix(list):

    def __init__(self, the_list):
        super().__init__(the_list)

    @property
    def nrow(self):
        return len(self)

    @property
    def ncol(self):
        return len(self[0])

    def get_dim(self):
        return self.nrow, self.ncol

    def __add__(self, M):
        return Matrix(
            [[sum(x) for x in zip(*rows)] for rows in zip(self, M)])

    def __mul__(self, M):
        return multiplication(self, M)

    def __rmul__(self, M):
        return multiplication(M, self)

    def add_rows(self, rows):
        super().extend(rows)

    def append(self, row):
        try:
            sum(row)  # Check if all numbers
            if len(row) < self.ncol:
                row.extend([0] * (self.ncol - len(row)))
            elif len(row) > self.ncol:
                [row.pop() for i in range(len(row) - self.ncol)]
            super().append(row)
        except:
            raise AssertionError('Elements in row must be mumbers')

    @property
    def transpose(self):
        return Matrix(map(list, zip(*self)))

    def show(self):
        print("Printing matrix: ")
        for i in self:
            print(i)


Loading .txt file content and filename into pandas dataframe Is there a better way to read in

the name of a textfile and
the content of a text file into a dataframe?

(Or is my implementation even okay?) Can I avoid storing the data in lists?

path =r'.../test_age7'
allFiles = glob.glob(path + "/*.txt")
df_7 = pd.DataFrame()  # create empty DF
stories = []
filenames = []
for file_ in allFiles:
    with open(file_) as f:
        textf = " ".join(line.strip() for line in f)
    stories.append(textf)
    filenames.append(os.path.basename(file_[0:-4]))    # extract filename without .txt

df_7["filename"] = filenames
df_7["stories"] = stories
df_7["age"] = path[-1]

As mention in the comments, pandas work really really well with csv so if you are generating the data your self you might consider to save the data in csv format.
allFiles is just used once, dont define it; use glob in loop instead.
Replace stories and filenames with just one DataFrame, and use pandas.concat()
If you are just updating the script evertime you run it, you can just have a age variable.
Never use file_[0:-4] to remove filextensions, use os.path.splitext.
I guess you will run this code for a lot of diffrent ages, so make a function out of it.

from os.path import basename, splitext
import pandas as pd

def getDataByAge(age)
    res = pd.DataFrame()
    for file_ in glob.glob(".../test_age%d/*.txt" % (age)):
        with open(file_) as f:
            textf = " ".join(line.strip() for line in f)
        res = pd.concat([res,pd.DataFrame(data = {"filename" : [splitext(basename(file_))[0]], "stories" : [textf], "age" : [age]})])
    return res


Insert a digit to maximize the given number Was asked in an interview today -

You are given an integer (in finite range [-N, N]) and you wish to maximize the number by inserting somewhere the digit '5'.

My solution - the idea here is going from left to right and checking if the digit is bigger than 5 or not. The first time you reach one that isn't you insert the 5 there. With the exception of negative numbers where you check and do the opposite.

I did it by converting ints to strings and vice versa.

def solution(N):
    array = str(N)
    result = []
    mod = 1
    for i in range(len(array) + 1):
        if i == len(array):
            result.append('5')
            break

        if array[i] == '-':
            mod = -1
            result.append(array[i])
            continue

        if int(array[i]) * mod > 5 * mod:
            result.append(array[i])
        else:
            result.append('5')
            result.append(array[i:])
            break

    return int("".join(result))

So - solution(128) will output 5128; solution(0) will output 50, solution(-74) will output -574 etc.

Presumably, if array[i] == '-' is meant to check if the number is negative. Having it like you have though doesn't make a lot of sense. If it's in the loop, that suggests that a - could be expected at any point within the number. In reality though, it will only ever occur as the first character in the string. I'd just check N the number with if N < 0 to see if it's negative. You can streamline this check right in the definition of mod:

mod = 1 if N >= 0 else -1

Unfortunately, that slightly complicates adding - to result. The simplest way to deal with it I think is just to change the definition of result to be similar to that of mod:

result = [] if n >= 0 else ['-']

Since the checks are the same though, you could combine then into returning a tuple instead then unpacking it:

mod, result = (1, []) if n >= 0 else (-1, ['-'])

And, array will need to have the - cleaved off if we're doing it before the loop:

mod, result, checked_digits = (1, [], digits) if n >= 0 else (-1, ['-'], digits[1:])

Normally, I don't think a triple variable definition like this should be encouraged, but there's nothing too fancy or complicated going on here to make the line confusing. I think it's fine.

I'd rename array and N:

array isn't really a correct or useful description. I think digits would be better.
N should be n.

You're iterating over the indices of digits and using digits[i] all over the place even though you don't even need the index in most cases. I'd iterate over the digits themselves, and use enumerate to get the index in the couple cases that you actually need it for:

for i, digit in enumerate(digits):
    . . .

    if int(digit) * mod > 5 * mod:
        result.append(digit)

I'd move the if i == len(array): check out of the loop as well. Really, it's just checking if at no point a break was encountered. That's exactly the use case for for's else clause (yes, for can have an else):

for i, digit in enumerate(checked_digits):
    . . .

else:
    result.append(5)

The else will only execute if no break was encountered. In the case of this code, that means that no digit greater than 5 was found.

Honestly, this is the first time I've ever found a useful case for for...else. It worked out nice here though.

I'd add 5 and '5' as constants at the top. Having the magic strings and numbers floating your code makes your code harder to change latter, and doesn't indicate what the 5 and '5' actually are for:

TARGET_NUM = 5
TARGET_DIGIT = str(TARGET_NUM)

def solution(n):

Or, just make that number a parameter with a default to make it easy to use different target numbers:

def solution(n: int, target_num: int = 5) -> int:
    target_digit = str(target_num)

I also added some type hints in there, because why not?

In the end, this is what I ended up with:

def solution(n: int, target_num: int = 5):
    target_digit = str(target_num)
    digits = str(n)

    mod, result, checked_digits = (1, [], digits) if n >= 0 else (-1, ['-'], digits[1:])

    for i, digit in enumerate(checked_digits):
        if int(digit) * mod > TARGET_NUM * mod:
            result.append(digit)

        else:
            result.append(TARGET_DIGIT)
            result.append(checked_digits[i:])
            break

    else:
        result.append(TARGET_DIGIT)

    return int("".join(result))


Map with different types for values I am trying to implements a map (that can work as a cache) that has strings as keys and values of different types. In my real case once set the key/values in the map will not change.

I used these classes, I take as example the Item 29 on Effective Java 2nd edition.

This is the class for values:

final class CachedValue {

    private Object value;

    public <T> void setValue(Class<T> type, T instance){

        if(type == null){

            log.error("type is null");

            throw new NullPointerException("Type is null");
        }

        value = type.cast(instance);
    }

    public <T> T getValue(Class<T> type){

        return type.cast(value);
    }
}

This for the map that works like a cache:

  public final class Cache {

            private final Map<String, CachedValue> map = Maps.newHashMap();

            public <T> void put(String key, Class<T> type, T value){

                CachedValue cachedValue = new CachedValue();

                cachedValue.setValue(type, value);

                map.put(key, cachedValue);
            }

            public <T> T get(String key, Class<T> type){

                return map.get(key).getValue(type);
            }

            public boolean contains(String key){

                return map.containsKey(key);
            }

      }

Can someone review my code and tell me if it's right or if it can be simplify? I don't need it to be thread-safe.

No Type Safety

Currently your classes provide no type safety:

Cache cache = new Cache();
cache.put("my key", Integer.class, 5);
// ...
String cached = cache.get("my key", String.class);

This compiles but fails at runtime with a ClassCastException thrown by CachedValue.getValue.

What's The Key?

The reason why this can happen is that your class is not clear on the role of the type given to put and get. Considering the internal map, the type is clearly part of the value. But get uses it as it were part of the key. This makes it possible to request a value which is there (considering the string key in the map) but of the wrong type.

Compare this to the typesafe heterogeneous container as defined in Effective Java. There the type is part of the key (actually the only part) so if something is found in the map, it must be of the correct type. It is hence safe to cast to that type.

So in order to make your classes type safe you have to decide on what exactly the key is.

String

If the key is only the string, I see no way to provide type safety. To keep the API from surprising its users you should then simply return Object. This effectively degrades your cache to a map from String to Object, which I guess is not your intention.

String & Class

The other possiblitiy is to make the type a full memeber of the key. There are several ways to implement this and I only present one. It uses nested maps which define this mapping:

String -> (Class -> Object)

This can be interpreted as

(String, Class) -> Object

so the type is part of the key.

public class Cache {

  private final Map<String, Map<Class<?>, Object>> internalMap;

  public Cache() {
    internalMap = new HashMap<>();
  }

  public <T> void put(String textKey, Class<T> typeKey, T chachedValue) {
    Map<Class<?>, Object> mapForTextKey = getMapForTextKey(textKey);
    mapForTextKey.put(typeKey, chachedValue);
  }

  private Map<Class<?>, Object> getMapForTextKey(String textKey) {
    if (!internalMap.containsKey(textKey))
      internalMap.put(textKey, new HashMap<>());
    return internalMap.get(textKey);
  }

  public <T> T get(String textKey, Class<T> typeKey) {
    Object untypedCachedValue = getUntyped(textKey, typeKey);
    // the cast can not fail because:
    // - if there is no value for those keys, the argument is null,
    //      which can always be cast
    // - if there is a value for those keys, `put` made sure that
    //      its type matches the type used as a key
    return typeKey.cast(untypedCachedValue);
  }

  private Object getUntyped(String textKey, Class<?> typeKey) {
    if (internalMap.containsKey(textKey))
      return internalMap.get(textKey).get(typeKey);
    else
      return null;
  }

}

Improvements

My proposal above is only the barebones of an implementation. There is a lot of room for improvements. Some of them are:

handling null aftert answering these questions:

should it be valid for keys? (please not!)
should it be valid for values? (also rather not; doesn't seem to make a lot of sense for a cache)

let get return an Optional<T>
use Map.computeIfAbsent for more concise code
or use Guava's Table instead of the nested maps
designing the class for extensibility or making it final

Furhter Comments After The Update

Looks much better. :) These comments are mostly nitpicking...

Comments regarding the code:

About null:

@NotNull is nice but as far as I know it's purely for tools. To have a runtime guarantee you should add Objects.requireNonNull. On the other hand, you use the HashBasedTable so you're fine  - it's documentation says: "Null row keys, columns keys, and values are not supported.". If you rely on that, you should declare table as that type and make sure do write a comment explaining this unusual decision (normally you declare the interface as you did).
I'd also make typeKey not-nullable.
I like the use of Optional (see here how much). I recommend to use Java 8's version because I expect it to become more common than guava's version.

Since Table.getis so concise, getUntyped looks a little pointless now. I would remove the method.
I like logging the calls. For symmetry I would also add it to put.

On style:

You are very liberal with empty lines. I'd recommend to not do that and only insert an empty line to separate distinct blocks. There seems to be little need for that in the code above.


Optimize jQuery code includes autocomplete function for different input box with different autocomplete content I have this code below for autocomplete feature of several inputTextbox. The input box also share same class".wikiInput". When user typed something in either of them, a relevant autocomplete dropdown box should show up. Instead of hard code one by one. How do I optimize them into a simpler format. Note the lookup has different arrays.

(function () {
    var cdTeamInput = $("#ctl00_ContentPlaceHolder1_txtAssociation");
    if (cdTeamInput.length > 0) {
        cdTeamInput.autocomplete({
            deferRequestBy: 0,
            autoSelectFirst: true,
            lookup: txtAssociation,
            onSelect: function (value, data) {
                cdTeamInput.val(value.value);
                $(".wikiSubmit").click();
            }
        });
    }
})();

(function () {
    var cdSubjectInput = $("#ctl00_ContentPlaceHolder1_txtSubject");
    if (cdSubjectInput.length > 0) {
        cdSubjectInput.autocomplete({
            deferRequestBy: 0,
            autoSelectFirst: true,
            lookup: txtSubject,
            onSelect: function (value, data) {
                cdSubjectInput.val(value.value);
                $(".wikiSubmit").click();
            }
        });
    }
})();

Update: I really like user2734550's first answer. however, the lookup array will be undefined in cross page situation. Like one of the page have only one lookup array but don't have others. so js file would be broken. How do I avoid this problem? Seems like if lookup.length > 0 statement doesn't work.

Quick answer: Use arguments.

function initAutocomplete(selector, lookup) {
  $(selector).autocomplete({
    deferRequestBy: 0,
    autoSelectFirst: true,
    lookup: lookup,
    onSelect: function (value, data) {
      cdSubjectInput.val(value.value);
      $(".wikiSubmit").click();
    }
  });
}

initAutocomplete("#ctl00_ContentPlaceHolder1_txtAssociation", txtAssociation);
initAutocomplete("#ctl00_ContentPlaceHolder1_txtSubject", txtSubject);

Of course, it might be nice to decouple the markup and the code some more. Depending on how the lookup lists (their length and contents), you could do something like this in your HTML:

<input data-lookup="foo,bar,baz" ...>

and init every autocompleting input on the page in one go with:

$("input[data-lookup]").each(function () {
  var input = $(this),
      lookup = input.data("lookup").split(","); // get the lookup strings

  input.autocomplete({
    deferRequestBy: 0,
    autoSelectFirst: true,
    lookup: lookup,
    onSelect: function (value, data) {
      cdSubjectInput.val(value.value);
      $(".wikiSubmit").click();
    }
  });
});


Timer implementation I have a Visual Studio 2008 C++03 project where I need a timer. I have an implementation that works well, but I'd like any general suggestions from the community on how to improve it. (More generic, lower cost of creating timers, etc...)

class Timer
{
public:
    typedef boost::function< void( ) > OnTimer;

    Timer( const OnTimer& f, DWORD timeout )
        : cancel_event_( ::CreateEvent( NULL, FALSE, FALSE, NULL ), ::CloseHandle ),
          timer_thread_( boost::bind( &Timer::TimerThread, cancel_event_.get(), f, timeout ) ),
          timeout_( timeout )
    {
    };

    ~Timer() { ::SetEvent( cancel_event_.get() ); };

    DWORD Timeout() const { return timeout_; };

private:

    static void TimerThread( HANDLE cancel_event, const OnTimer& f, DWORD timeout )
    {
        if( WAIT_OBJECT_0 != ::WaitForSingleObject( cancel_event, timeout ) )
            f();
    };

    /// time to wait for the timer to fire
    UINT timeout_;
    /// event signaled when the timer should be canceled
    boost::shared_ptr< void > cancel_event_;
    /// thread that runs the timer (similar to boost::thread, but doesn't require `localtime`, `gmtime`, or `abort`)
    util::CThread timer_thread_;
}; // class Timer

Preemptive:

My project does not have a window message pump so SetTimer is unsuitable.
I do not want to incur the cost of importing the mmtimer library.
I can only use boost header libraries, therefore boost::timer is unsuitable.
I would prefer to keep the discussion limited to this block of code.

I think that you can create one thread to handle all timers. When a new timer is created, you should add it in a (synchronized) queue of "timers to trigger". The background thread must wait for the closest timer to be triggered. Also you should wake it up each time you modify the queue of timers and recalculate "which is the closest timer now?".

When the queue is empty, the background thread can be destroyed (joined) or you can wait an "exit" event (or the creation of another timer). Those are just details. The background thread can be created by the first timer and joined when the last timer is destroyed, or it can just live in the whole program execution.

Another thing, to improve portability you could use boost::thread library and boost's condition variables instead of raw Windows events.


Assigning values to array elements based on a lookup table I am writing a C# program wherein I need to populate an array based on a lookup table and set of string arrays with metadata. My lookup table looks like this (Table with key: transmitter, value: Array of receiver):

{
    LED1: ["px1","px2","px3"],
    LED2: ["px4","px5","px6"]
}

and my meta arrays looks like this (it is dynamic (just an example) and comes as a response from a DB query):

var transmitters = new string[] { "LED1", "LED2" };
var receivers = new string[] { "px1", "px2", "px3", "px4", "px5", "px6" };

My requirement is:

If the transmitter LED1 or LED2 (or any other transmitter) is present in the lookup table, the value of the transmitter (i.e. ["px1","px2","px3"]) has to be compared with the receiver which are present in the lookup and led has to be marked yellow.
Orphan transmitter or/ receiver has to be marked red.

Example

Lookup

{
        LED1: ["px1", "px2", "px3"],
        LED2: ["px5", "px8"]
}

Transmitters and receivers

var transmitters = new string[] { "led1", "led2" };
var receivers = new string[] { "px1", "px2", "px3", "px4", "px5", "px6" };

The result should be a list as:

led1-yellow
px1-yellow
px2-yellow
px3-yellow
led2-yellow
px5-yellow
px4-red
px6-red.

I have written code that works:

using System;
using System.Collections.Generic;
using System.Linq;

public class Program
{
    public static void Main()
    {
      var transmitters = new string[] { "led1", "led2", "led3" };
      var receivers = new string[] { "px1", "px2", "px3", "px4", "px5", "px6" };
      var lookup = new Dictionary<string, string[]>() {
          { "led1", new string[] { "px1", "px2", "px3" } },
          { "led2", new string[] { "px5", "px8"} }
      };

      var blocks = new List<Block>();
      var blocksTracker = new List<string>();

      foreach (var transmitter in transmitters)
      {
        if (lookup.ContainsKey(transmitter))
        {
          var receiverLookup = lookup[transmitter];
          var intersection = receivers.Intersect(receiverLookup).ToArray();
          if (intersection.Length > 0)
          {
             blocks.Add(new Block() { Id = transmitter, status = "yellow"});
             blocksTracker.Add(transmitter);
            foreach (var receiver in intersection)
            {
              blocks.Add(new Block(){Id = receiver, status = "yellow"});
              blocksTracker.Add(receiver);
            }
          } else
          {
            blocks.Add(new Block(){Id = transmitter, status = "red"});
            blocksTracker.Add(transmitter);
          }
        }
      }

      var ungrouped = receivers.Except(blocksTracker).ToArray();

      foreach (var receiver in ungrouped)
      {
        blocks.Add(new Block(){Id = receiver, status = "red"});
        blocksTracker.Add(receiver);
      }

      foreach (var i in blocks)
      {
        Console.WriteLine(i.Id + "-"+i.status);
      }
    }

    public class Block
    {
      public string Id { get; set; }

      public string status { get; set; }
    }
}

I am new to C# and I wanted to know if there is a better way of doing this. You can see the working Fiddle here.

if (lookup.ContainsKey(transmitter))
        {
          var receiverLookup = lookup[transmitter];

This searches for the KeyValuePair twice. There's a more efficient approach:

        if (lookup.TryGetValue(transmitter, out var receiverLookup))

      var ungrouped = receivers.Except(blocksTracker).ToArray();

      foreach (var receiver in ungrouped)
      {
        blocks.Add(new Block(){Id = receiver, status = "red"});
        blocksTracker.Add(receiver);
      }

The ToArray() there is unnecessary: the enumerable can be left as a lazy enumerable because the only use is to iterate over it once.

          var intersection = receivers.Intersect(receiverLookup).ToArray();
          if (intersection.Length > 0)
          {
             blocks.Add(new Block() { Id = transmitter, status = "yellow"});
             blocksTracker.Add(transmitter);
            foreach (var receiver in intersection)
            {
              blocks.Add(new Block(){Id = receiver, status = "yellow"});
              blocksTracker.Add(receiver);
            }
          } else
          {
            blocks.Add(new Block(){Id = transmitter, status = "red"});
            blocksTracker.Add(transmitter);
          }

This seems rather complicated. I think the whole thing could be simplified:

var transmittersPaired = new HashSet<string>();
var receiversPaired = new HashSet<string>();

foreach (var transmitter in transmitters)
{
    if (lookup.TryGetValue(transmitter, out var receiverLookup) && receiverLookup.Any())
    {
        transmittersPaired.Add(transmitter);
        foreach (var receiver in receiverLookup)
        {
            receiversSeen.Add(receiver);
        }
    }
}

var blocks = new List<Block>();
foreach (var transmitter in transmitters)
{
    blocks.Add(new Block { Id = transmitter, status = transmittersPaired.Contains(transmitter) ? "yellow" : "red" });
}
foreach (var receiver in receivers)
{
    blocks.Add(new Block { Id = receiver, status = receiversPaired.Contains(receiver) ? "yellow" : "red" });
}

There's still some repeated code, which might be simplified in one of two ways. If there's a guarantee that the transmitters and receivers will never share IDs then transmittersPaired and receiversPaired could be merged into one set, and the foreach loops at the end could be merged into one loop over transmitters.Concat(receivers). Alternatively, a method could be factored out.


ScheduledExecutorService running a couple of scheduleAtFixedRate I have created a task that I need to query database for List of String and store it in neededDataList. I will use neededDataList in a scheduled task every 10 minutes to do a simple task.
And I need to retrieve a fresh data of neededDataList every 1 hour. Will the scheduledExecutorService2 pick up the fresh neededDataList?
Can someone review my code if it will cause some problem?
Assuming my code has its main method and it will call the run(Database database):
    public class Handler {

            private static final ScheduledExecutorService scheduledExecutorService1 = MDScheduledExecutorService.wrap(
                        new ScheduledThreadPoolExecutor(1), "ses-1-thread-%d");
            private static final ScheduledExecutorService scheduledExecutorService2 = MDScheduledExecutorService.wrap(
                        new ScheduledThreadPoolExecutor(5), "ses-2-thread-%d");

            private List<String> neededDataList;

            private void run(Database database) {
                scheduledExecutorService1.scheduleAtFixedRate(() -> {
                    neededDataList= database.getNeededData();
                }, 1, 60, TimeUnit.MINUTES);

                scheduledExecutorService2.scheduleAtFixedRate(new HandlerImpl(), 1, 10, TimeUnit.MINUTES);
            }

            private class HandlerImpl implements Runnable {

            @Override
            public void run() {
               executeTask(neededDataList);
            }

            }
}

Your code is not thread-safe.
The Handler.run() method creates two services which will end up being deparate threads.  These threads are accessing and updating the neededDataList shared variable without any synchronization.  This could lead to unpredictable behavior, including the service that is reading the list seeing stale values.
Also, this code won't compile because the executeTask method is not defined.


Unique binary search trees I have written some code that prints out all unique pre-ordered binary search trees containing numbers 1 to n.

It is not an especially efficient solution and has a time complexity (I believe) of O(n!) but hopefully it is fairly easy to understand.

As I am using sets the output is not sorted.

"""Module constructs and prints unique BST."""
import itertools
class Node():
    """Builds a BST"""
    def __init__(self, value, left=None, right=None):
        self.value = value
        self.left = left
        self.right = right
    def add_node(self, value):
        """Adds new elemenet to binary search tree."""
        if self.value:
            if value > self.value:
                if not self.right:
                    self.right = Node(value)
                else:
                    self.right.add_node(value)
            else:
                if not self.left:
                    self.left = Node(value)
                else:
                    self.left.add_node(value)
    def print_tree(self):
        """Return the BST as a string"""
        string_tree = ""
        if self.value:
            string_tree += (str(self.value) + " ")
            if self.left:
                string_tree += self.left.print_tree()
            if self.right:
                string_tree += self.right.print_tree()
        return string_tree

def build_BST(permutation):
    """Builds a BST from a list of numbers"""
    if permutation:
        tree = Node(permutation[0])
        for i in range(1, len(permutation)):
            tree.add_node(permutation[i])
        string_tree = tree.print_tree()
        return string_tree
    return False

def build_trees(size_tree):
    """Build BST containing numbers in range 1 to n"""
    if not isinstance(size_tree, int):
        print("Please enter an integer")
        return
    if size_tree <= 0:
        print("Function only prints trees with numbers >= 1")
        return
    permutations = list(itertools.permutations([i for i in range(1, size_tree + 1)],
                                               size_tree))
    set_solutions = set()
    for permutation in permutations:
        string_tree = build_BST(permutation)
        set_solutions.add(string_tree)
    for solution in set_solutions:
        print(solution)
    print(f"==========\n{len(set_solutions)} solutions.")

if __name__ == "__main__":
    build_trees(4)

Some suggestions:

You can use sys.argv or argparse to pass the size on the command line. The latter can check the type and range for you, and print an appropriate error message and usage string. You can specify a validator function in the type argument to check for non-trivial values.
print_tree does not print the tree, it returns a string representation of it. That is the job of __str__, a special internal method you can override to define the string representation of a class.
"node" in add_node is redundant.
Running this code through black will improve the formatting slightly. flake8 and mypy (with a strict configuration) can help make the code more idiomatic.


Save user input in server-side database I have created a site for practice and I have already asked a question on Stack Overflow about XSS.

  Someone tell me to show your php functions on this site that i created using filter_Var

These are two PHP functions I created that sanitize/validate user input and after passing the the user input in these function I save the user input in my server (storage).

function Meg_selfvalidator_html($data){//Validator
    if(is_array ($data)){

        $arrayLen = count($data);

        for($i=0;$i<$arrayLen;$i++){
            $data[$i] = trim($data[$i]);
            $data[$i] = stripslashes($data[$i]);
            $data[$i] = htmlentities($data[$i], ENT_QUOTES);
            $data[$i] = filter_var($data[$i], FILTER_SANITIZE_STRIPPED, FILTER_FLAG_STRIP_HIGH );
            $data[$i] = filter_var($data[$i], FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_HIGH);
            $data[$i] = filter_var($data[$i], FILTER_SANITIZE_STRIPPED, FILTER_SANITIZE_STRING);

        }
        return $data;

    }else{
        $data= trim($data);
        $data= htmlentities($data, ENT_QUOTES);
        $data = stripslashes($data);
        $secured_output = filter_var($data, FILTER_SANITIZE_STRIPPED, FILTER_FLAG_STRIP_HIGH );
        $secured_output = filter_var($secured_output,  FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_HIGH );
        $secured_output = filter_var($secured_output, FILTER_SANITIZE_STRIPPED, FILTER_SANITIZE_STRING);
        return $secured_output;
    }

}

This is my second PHP function:

function selfValidator($data) {//Validator

    if(is_array ($data)){

        $arrayLen = count($data);

        for($i=0;$i<$arrayLen;$i++){
            $data[$i] = trim($data[$i]);
            $data[$i] = stripslashes($data[$i]);
            $data[$i] = strip_tags($data[$i]);
            $data[$i] = htmlspecialchars($data[$i]);
            $data[$i] = filter_var($data[$i], FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_HIGH);
        }
        return $data;

    }else{
        $data = trim($data);
        $data = stripslashes($data);
        $data = strip_tags($data);
        $data = htmlspecialchars($data);
        return $data;
    }
}

Which one is the best function or what is bad in these functions? How do I add a filter to validate the link?

Neither is all that good. It seems that you just apply functions at random to your data in the hope that some of them will protect you, which is not the correct approach to security.

How to properly defend against XSS

First of all, the correct approach to XSS is to HTML encode data when outputting it. That is where the vulnerability is create, and that is where it should be prevented.

There are three important reasons for this:

When outputting, you know the context, which is important to defend against XSS (is it inside script tags? inside a HTML attribute? What attribute? A JavaScript context attribute? Are there quotes around it? What quotes? ...)
You really can't be sure when outputting data that it was previously secured (can data get into the database in other ways than GET/POST? etc).
Your data will not be clean anymore, which may break your website (eg when sending an email, or when using passwords (with your function, "a<super!secure^password" would become a)).

Note that there are contexts in which HTML encoding may not be enough to prevent XSS.

Improving your Code

Why are you using different cleaning functions for array values and non-array values in selfValidator? It doesn't make any sense to treat these differently.

For that matter, if you would use the same functions, you would have duplication in your code, which should be removed (either by using recursion or by extracting the cleaning to a separate function).

Also:

Use more spaces, eg around <, =, etc (just use any IDE to fix formatting issues for you).
Your variable names are not consistent. Sometimes you use secured_output and sometimes data. When things are named differently, you make a reader of the code wonder why that is (eg is data maybe not secured)?
Your Validator comment doesn't really add any information, I would just remove it (and give the functions a more fitting name instead). You could create PHPDoc style comments instead, documenting the behavior of your function in-depth

The cleaning functions you use

I think it would be a good idea to look at the functions you do use in-depth, to get a real understanding what each of them does. Ideally, you take a look at the PHP documentation for this, but here is a short overview:

trim: remove whitespace, may be useful, depending on the situation
stripslashes: ONLY use this when magic quotes are enabled. Stripping slashes does nothing for security, and it makes your data unclean if magic quotes are disabled (which is the default).
htmlentities: This is what you generally want to use to prevent XSS when outputting data in most contexts.
strip_tags: Not really all that useful. Doesn't prevent XSS in as many contexts as htmlentities, and makes your data dirtier than htmlentities (see my password example above).
filter_var: These functions are great when first accessing GET/POST values, to restrict what characters are allowed (as defense in depth, NOT as only defense). If you know that you need only alphanum, it's good to restrict the characters, etc (just in case that you do have vulnerabilities in other parts of your application, this will make it harder to exploit them). But: You can't just apply filter_var to all values that you will ever receive. It's again context dependent. If you expect an id, only accept integer. If you expect an email address, use that filter. But if you eg expect a password, filtering it is a bad idea, as it reduces the complexity of the password.


SQL view, better way? I there a better way to write this query?  I feel sure it can be done in one statement with joins, but I couldn't get it, and I'm afraid I've made it too slow.  What suggestions do you have?  I'm running it in SQL Server 2000 (yeah, I know, upgrade is coming).

Basically, I want to match estimated and actual costs, but sometimes the estimate is done on 1 cost center, and the actual costs are set to another cost center (hence the possibility of having null in est or act.  I want to get all possible combinations for that job.

ALTER view [dbo].[JobCost_EstVsAct]     --SELECT * FROM JobCost_EstVsAct Where jobnumber = '122773'

as

SELECT JobNumber, CostCenter, sum(Amount) as Est, sum(cost) as Act
FROM
(
SELECT JobNumber, CostCenter, Amount, 0 as cost
FROM Avanti_ActiveJobBudgets AJB

UNION

SELECT jobnumber, costcentre as CostCenter, 0 as Est, cost as Act
FROM Avanti_ActiveCostDetails
) temp
--where cost + Amount > 0     This line is a bug
GROUP by JobNumber, CostCenter
HAVING sum(Amount) + sum(cost) > 0   --bug correction

First off, it's not a stored procedure, it's a view.

That said, the main change I would make is to use a CTE, once you have upgraded.

ALTER view [dbo].[JobCost_EstVsAct]
    --SELECT * FROM JobCost_EstVsAct Where jobnumber = '122773'
AS
WITH cte1 as (
    SELECT JobNumber, CostCenter, Amount, 0 as cost
    FROM Avanti_ActiveJobBudgets AJB
),
cte2 as (
    SELECT jobnumber, costcentre as CostCenter, 0 as Amount, cost as Act
    FROM Avanti_ActiveCostDetails
),
cte3 as (
    Select * From cte1
    Union
    Select * From cte2
)
SELECT JobNumber, CostCenter, sum(Amount) as Est, sum(cost) as Act
FROM cte3
--where cost + Amount > 0     /* This line is a bug */
GROUP by JobNumber, CostCenter
HAVING sum(Amount) + sum(cost) > 0   --bug correction

Replacing the cteX with more appropriate names.


Find the first occurrence of a substring in a string This is a simple version from the function strstr. It returns the address of the first occurrence of the substring s2 in s1. I want to know possible problems in the code, and how to improve it, in general.

#include <stdio.h>

/* Version of strstr, which returns the adress of
   the first occurrence of s2 in s1, else, NULL
*/
char *strs(char *s1, char* s2)
{
    char *ptr = NULL;
    int i, n;

    for (i = 0, n = 0; s1[i] != '\0'; i++)
    {
        if (s1[i] == s2[n])
        {
            if (n == 0)
                ptr = &(s1[i]);
            if (s2[n + 1] == '\0')
                return ptr;
            ++n;
        }
        else
            n = 0;
    }
    return NULL;
}

int main(void)
{
    char s1[] = "I'm waiting in my cold cell\nWhen the bell begins to chime\n";
    char s2[] = "waiting";
    char *ptr = strs(s1, s2);

    if (ptr == NULL)
    {
        printf("There is no occurrence of \"%s\" in the first string!\n",
        s2);
    }
    else
    {
        printf("%s\n", ptr);
    }
}

Comments about your code:

I believe that you would rename it to something like strstr if you could.

You should accept pointers to const char, since you don't modify them.

You should initialize i and n right inside the loop. It will narrow their scope and make the code more maintainable since it won't pollute the scope of the function. On top of that, they will be deallocated whenever the loop ends.

using int for index is a very bad idea. The biggest issues with it that it is signed and not guaranteed to be large enough. You should use size_t instead, because it is guaranteed that size_t will have enough capacity to index array of maximum possible size.

You're increasing overhead by using subscript ([]) operator.

Loop could be rewritten in other, more readable and straightforward form, one of which is shown in suggested implementation. I know that you would like to have high performance, but I don't think that it is that important in this situation. May be with more context of the usage of the algorithm I could suggest particular algorithm (there are quite a few of them). The best in my opinion is Boyer–Moore string search algorithm.

Suggested implementation:

I think that strncmp should be used. I know that including string.h header defeats the purpose, but you could write your own. It increases code reuse in C!

To make it work we just need to precalculate length of the substring.

const char *strs(const char *text, const char *substr)
{
    size_t substrlen = strlen(substr);
    while (*text != 0)
    {
        if (strncmp(text, substr, substrlen) == 0)
        {
            return text;
        }
        ++text;
    }

    return NULL;
}

The performance gain from the data being const depends on physical constness and the compiler. Further reading.

About performance gain..


Search filter for words in dart I need to add search filters for firebase So that I can get search result easily and precisely
I was altering the code from hereThis Medium Post
I would like to optimize this code.
void main() {
  print(setSearchParam("Dr apj abdul Kalam"));
}

setSearchParam(String caseNumber) {
  List<String> caseSearchList = List();
  String temp = "";
  String temp2 = "";
  String temp3 = "";

  if (caseNumber.split(" ").length > 1) {
    String firstWord = caseNumber.split(" ")[0];
    String secondWord = caseNumber.split(" ")[1];
    String thirdWord = caseNumber.split(" ")[2];

    for (int i = 0; i < firstWord.length; i++) {
      temp = temp + firstWord[i];
      caseSearchList.add(temp);
    }
    for (int i = 0; i < secondWord.length; i++) {
      temp2 = temp2 + secondWord[i];
      caseSearchList.add(temp2);
    }
    for (int i = 0; i < thirdWord.length; i++) {
      temp3 = temp3 + thirdWord[i];
      caseSearchList.add(temp3);
    }
  } else {
    for (int i = 0; i < caseNumber.length; i++) {
      temp = temp + caseNumber[i];
      caseSearchList.add(temp);
    }
  }
  return caseSearchList;
}

OutPuts are like List of each word letters = [D, Dr, a, ap, apj, a, ab, abd, abdu, abdul]
I am pushing these lists to firebase so that I can get search results easily...
if there is an alternative for searching in firebase, then pls let me know...
if anyone still didn't understand my question, pls look at the link I provide above.

I have tried make a more optimized version here which also will work for string of any sizes and any number of words:
void main() {
  print(setSearchParam("Dr apj abdul Kalam"));
  // [D, Dr, a, ap, apj, a, ab, abd, abdu, abdul, K, Ka, Kal, Kala, Kalam]

  print(searchParamWord('test'));
}

List<String> setSearchParam(String caseNumber) =>
    caseNumber.split(' ').expand(searchParamWord).toList();

Iterable<String> searchParamWord(String word) sync* {
  final sb = StringBuffer();

  for (var i = 0; i < word.length; i++) {
    yield (sb..writeCharCode(word.codeUnitAt(i))).toString();
  }
}

I have split the logic of your code into two parts.
searchParamWord takes one word and split it into its components like "(t, te, tes, test)" and returns a lazy evaluated iterator. This iterator can then later be used by expand in setSearchParam where we split the long String into each separate word.
By request, example how to upper case first letter in each word
void main() {
  print(setSearchParam("Dr apj abdul Kalam"));
  // [D, Dr, A, Ap, Apj, A, Ab, Abd, Abdu, Abdul, K, Ka, Kal, Kala, Kalam]

  print(searchParamWord('test'));
}

List<String> setSearchParam(String caseNumber) =>
    caseNumber.split(' ').expand(searchParamWord).toList();

Iterable<String> searchParamWord(String word) sync* {
  final sb = StringBuffer();

  for (var i = 0; i < word.length; i++) {
    yield (i == 0)
        ? (sb..write(word[i].toUpperCase())).toString()
        : (sb..writeCharCode(word.codeUnitAt(i))).toString();
  }
}