Using UPSERT to maintain word counts for subtitles I will first explain the context. I want to read serie subtitles contained in .srt files. I have already done the part of extracting the words from the files (and how many times the word appears for a serie) but when it comes to inserting the data in my SQL Server database, it takes more and more time to do the updates or inserts because i am checking if the word exists or not. I call a stored procedure from C# with the following parameters : the word, how many times it appears in my files, the serie name.

ALTER PROCEDURE [dbo].[INSERT_WORD_FOR_SERIE]
    @word nvarchar(50),
    @counter int,
    @serie nvarchar(50)
AS
BEGIN
    DECLARE @idSerie AS int
    DECLARE @idWord AS int

    SELECT @idWord = IDWORD FROM WORD WHERE WORD=@word;

    IF @idWord IS NOT NULL
        BEGIN
            SELECT @idSerie = IDSERIE FROM SERIE WHERE SERIE=@serie;
            IF EXISTS (SELECT counter FROM APPEARS WHERE IDWORD=@idWord AND IDSERIE=@idSerie)
                BEGIN
                    UPDATE APPEARS SET counter = counter + @counter WHERE IDWORD=@idWord AND IDSERIE=@idSerie;
                END
            ELSE
                BEGIN
                    INSERT INTO APPEARS VALUES (@idWord, @idSerie, @counter);
                END
        END
    ELSE
        BEGIN
            INSERT INTO WORD VALUES (@word);
            SELECT @idWord = IDWORD FROM WORD WHERE WORD=@word;
            SELECT @idSerie = IDSERIE FROM SERIE WHERE SERIE=@serie;
            INSERT INTO APPEARS VALUES (@idWord, @idSerie, @counter);
        END
END

I have inserted only two series and I can already see that it is taking more and more time to insert or update one word.

Can you see any improvements that can be done or different way of doing what I want more efficiently ?

Like Pankaj Gupta already mentions, you could move the query for @idSerie outside the IF-statement, as it is exactly the same query on exactly the same table with exactly the same data (it does not change anywhere in this stored procedure).

Secondly, I find the code a bit hard to read because of the casing of the table and field names.  This is, of course, a matter of style, but unless you are not "in charge" of the naming style, I would suggest to case them differently.  E.g. using camelCase for field names as you do for variable names (and the APPEARS.counter column), and use PascalCase for the table names.  This way, using UPPERCASE for reserved SQL keywords makes them stand out, instead of having them look just the same as the rest.

Separation of concerns is probably a bit harder in T-SQL, because writing beautiful T-SQL is not always the same as writing well-performing T-SQL.  But you could split up your stored procedure into three parts: the first part would get the @idSerie value from the SERIE table.  The second part would fetch the @idWord from the WORD table, inserting it if it's not already in there.  The third part then would upsert the correct values into the VALUES table.  Not only will this make the code more readable (you only have to remember one table and one or two variables in each part), but also more maintainable, because the INSERT INTO APPEARS command would be only in one place.

Those three parts have generally the same form: 1) select @id, 2) insert or update that row, and 3) get the real @id.  2 and 3 don't apply to each part, so they are optional (you don't need to insert a SERIE row, and you don't need to update a WORD row).

You didn't mention which version of SQL Server you use, but since version 2008, you could look into the MERGE statement, which combines tho logic of inserting and updating into a single statement.

About performance (of which I don't know whether it is in the scope of code review): I suspect that the WORD table contains a unique entry for each word.  As you mention that the query takes more and more time, the more words are in there, you might want to add a (unique) index on the WORD.WORD column, but I don't really know much about performance, so you might want to experiment.

Finally, it's good practice to specify the columns you are inserting into, for readability and maintenance purposes.  So I'd change

INSERT INTO APPEARS VALUES (@idWord, @idSerie, @counter);

To:

INSERT INTO APPEARS (IDWORD, IDSERIE, counter) VALUES (@idWord, @idSerie, @counter);

Likewise for the other inserts.

By the way, I like that you are ending all statements with a semicolon, which is not mandatory, but a good practice, and can help prevent a kind of errors with changing code, using CTE's or query hints for example.


Angular Modular File Structure My intention is to separate components on a file basis. For example, I want a specific controller to have it's own file (Same goes with services, filters and directives). Of course, files will be group together based on the module they will fall into.

Here's an overview of what I currently have:

Directory

User/
    User/UserModule.js
    User/UserDirective.js
    User/UserService.js
    User/UserFilter.js
    User/UserController.js

UserModules.js

UserModule = angular.module('UserModule', []);

UserModule.controller('userCtrl', ['$scope', 'UserService', UserCtrl])

    .factory('userService', function() {
        return new UserService();
    })

    .filter('userFilter', UserFilter)

    .directive('userDirective', UserDirective);

UserController.js

UserCtrl = function($scope, UserService) {
    // ...
};

UserDirective.js

UserDirective = function() {
    return {
        // ...
    }
};

UserService.js

UserService = function() {
    // ...
};

UserFilter.js

UserFilter = function() {
    return function() {
        // ...
    }
};

Then I'll just push the user module to the app module.

app.requires.push('UserModule');

My concern lies on the registration of the concepts (Such as controllers, services...) to the module. I was wondering if this is the best way to go and if it's correct. Also possible issues on the parameters and the external .js file.

Consider this part:

.controller('userCtrl', ['$scope', 'UserService', UserCtrl])

The UserCtrl above refers to a function defined in a separate file. Will I be able to pass the $scope and UserService dependency as parameters to the UserCtrl?

UserCtrl = function($scope, UserService) { // Pass parameters (UserController.js)

What's the correct way of doing this in terms of Services, Filters and Directives?

Finally, how can I improve the code?

It would be much easier to give feedback if we saw your actual code, for those 5 js files.

Regardless, I tend to organize differently:

controllers/
  user.js
views/
  user.js
models/
  user.js

So anything that does not fit controller/user.js or views/user.js goes into models/user.js.

I want a specific controller to have it's own file (Same goes with services, filters and directives). There is no good enough reason to do this, keep controllers and views together, and do not split up everything.


Speeding Ticket Application I'm looking to get some advice on whether or not I am building/structuring my application the correct way. The program has a few key components:

Calculate the fine based on how fast over the limit a driver was travelling
Generate a fine letter address to the driver
Update a file with a driver's registration details and the fine

So that being said, I have the following classes:

DriverDetails
DriverManager
Fine
FineCalculator
FineType

Here is the idea:

When I want to work out the fine:

FineCalculator object is created
The speed limit is set
The speed of the car is set and then the calculateFine() method is called
The calculateFine() method will return a Fine object, which is a data object which contains an amount, and a string (the result), the points if any, a FineType

This is one side of the application.

The other part is to be done after a fine has been generated. The user will enter the driver details, a DriverDetails object will be created, for the user to then generate any form of documentation it will use the DriverManager object which will take a DriverDetails object when constructed.

The FineCalculator class:

public class FineCalculator {

    private int speedlimit;

    private int carspeed;

    public FineCalculator(int speedlimit, int carspeed) {

        this.speedlimit = speedlimit;
        this.carspeed = carspeed;
    }

    public Fine calculateFine() {

        Fine fine = new Fine();

        int milesover = carspeed - speedlimit;

        if(milesover < 1) {

            fine.result = "The user was not going above the speed limit, no actions will be taken.";
        }

        else if(milesover < 5) {

            fine.result = "No fine issued, no points gained but a warning will be issued to the driver.";
            fine.finetype = FineType.WARNING;
        }

        else if(milesover > 4 && milesover < 10) {

            fine.result = "A fine of £50.00 will be issued to the driver. 0 points gained.";
            fine.amount = 50.00;
            fine.finetype = FineType.FIFTY;

        }

        else if(milesover > 9 && milesover < 15) {

            fine.result = "A fine of £100.00 will be issued to the driver. 0 points gained.";
            fine.amount = 100.00;
            fine.finetype = FineType.ONE_HUNDRED;

        }

        else if(milesover > 14 && milesover < 20) {

            fine.result = "A fine of £150.00 will be issued to the driver. 3 points gained.";
            fine.amount = 150.00;
            fine.points = 3;
            fine.finetype = FineType.THREE_POINTS_ONE_FITY;
        }

        else if(milesover > 19) {

            fine.result = "A fine of 1000.00 will be issued to the driver, and they will be disqualified.";
            fine.finetype = FineType.DISQUALIFIED;
        }

        return fine;

    }
}

The Fine class:

public class Fine {

    public Double amount;
    public String result;
    public int points;
    public FineType finetype;

}

The DriverDetails class:

public class DriverDetails {

    public String firstname, surname, address, city, postcode;
    public int drivernumber;

}

The DriverManager class:

public class DriverManager {

    private DriverDetails driverdetails;
    private Fine fine;

    public DriverManager(DriverDetails driverdetails, Fine fine) {

        this.driverdetails = driverdetails;
        this.fine = fine;
    }

    public void generateFineLetter() {

    }

    public void updateDriverDetailsWithFine() {

    }
}

And finally, the simple enum class:

public enum FineType  {

    WARNING, FIFTY, ONE_HUNDRED, THREE_POINTS_ONE_FITY, DISQUALIFIED

}

My biggest concern is the fact that I'm not using the getters/setters inside the DriverDetails and Fine data classes. I mean, it doesn't feel like the getters and setters are completely necessary. Am I wrong? I know what I'm doing goes completely against encapsulation, but in this scenario is that bad? Can I have any advice here as to what to improve or if I'm structuring this wrong?

It is a question of how much will the project expand beyond what is described here.  For instance, are you going to persist driver details and other data? if you are going to use a database, then you will probably want to use an Object-Relation-Mapping (ORM) framework, and they tend to expect Java Beans to work with. perhaps you will want to develop a web api that will send or receive such data in json format? json parsers/generators also prefer objects that follow Java Bean standard.

for very simple, small, one-man projects it is probably ok to skip the boilerplate code of getters/setters.  it is just that usually projects tend to grow and expand in time.

another point that can and should be addressed now is the calculator: it feels wrong to me to create instances just to do fine calculation. if we make the analogy to windows calculator, it is like openning a new window each time you want to do a new calculation.  the calculator is stateless: it takes arguments and produces a result. it does not carry any state between calculations.  So I would make the calculateFine() method static and have it accept the two arguments.

another issue is the calculation process itself.  all the attributes of a fine are closely related to each other and come in mutually exclusive sets: the result is a textual description of the enum, the amount and points are also linked to it.

I would make these values properties of the enum:

public enum FineType
{
    NONE("The user was not going above the speed limit, no actions will be taken.", 0, 0),
    WARNING("No fine issued, no points gained but a warning will be issued to the driver.", 0, 0),
    FIFTY("A fine of £50.00 will be issued to the driver. 0 points gained.", 50.00, 0),
    // and so on

    private String description;
    private int    fine;
    private int    points;

    FineType(String description, int fine, int points) {
        this.description = description;
        this.fine = fine;
        this.points = points;
    }

    public String getDescription() { return description; }
    public String getFime() { return fine; }
    public String getPoints() { return p[oints; }
}

now you can have the Fine take a type in the constructor and fill the properties by itself. this makes the calculator process clearer:

public static Fine calculateFine(int speedlimit, int carspeed) {

    int milesover = carspeed - speedlimit;

    if(milesover < 1) {
        return new Fine(FineType.NONE);
    }
    else if(milesover < 5) {
        return new Fine(FineType.WARNING);
    }
    ...


Change code to use functional style I tried to solve a programming contest problem in Scala, but I have a feeling, that it could be done in a more functional way. I compared it with the solution written in imperative style and it is not shorter or less complex at all. Do you have any idea how to "impove" or approach the problem in a more functional way?

Here is my solution:

object Sunshine extends App {

  def tanning(bedNumber:Int, part1 : List[Char], part2: List[Char], inBed: List[Char], tanned:Int, walkedAway: Int) : Int  = {

     val actCustomer =part2.head
     val tanningTuple=(part1.indexOf(actCustomer),inBed.indexOf(actCustomer),bedNumber>inBed.length,part2.tail)

     tanningTuple  match {
      case(_,_,_,Nil)  => walkedAway
        // enter,can be tanned
      case (-1,-1,true,_) =>
        tanning(bedNumber,actCustomer::part1,part2.tail,actCustomer::inBed,tanned,walkedAway)
        // enter, can not be tanned
      case (-1,-1,false,_) =>
        tanning(bedNumber,actCustomer::part1,part2.tail,inBed,tanned,walkedAway)
        //leave, not in bed
      case (x ,-1,_,_) if x>=0 =>
        tanning(bedNumber,actCustomer::part1,part2.tail,inBed,tanned,walkedAway+1)
      //leave, in bed
      case (x,y,_,_) if x>=0 && y>=0 =>
        tanning(bedNumber,actCustomer::part1,part2.tail,inBed-actCustomer,tanned+1,walkedAway)
    }
  }

  println(tanning(2,List(),"ABBAJJKZKZ".toList,List(),0,0))
  println(tanning(3,List(),"GACCBDDBAGEE".toList,List(),0,0))
  println(tanning(3,List(),"GACCBGDDBAEE".toList,List(),0,0))
  println(tanning(1,List(),"ABCBCA".toList,List(),0,0))
}

A good way to think about this problem is to list the different cases when a customer is encountered in the list:

The person arrives and there is a bed available => the person gets a tan and the number of available bed decreases by one
The person arrives and there is not a bed available => the person won't tan
The person leaves and they got a tan => the number of available beds increases by one
The person leaves and they didn't get a tan => nothing special happens

When there are no more customers then we need to return the number of people who didn't tan.

This is embodied by the following function:

def step(avail: Int, tanned: Set[Char], not_tanned: Set[Char], customers: List[String]): Int = {
  customers match {
    // case 3 above
    case c::cs if tanned(c) => step(avail+1, tanned, not_tanned, cs)
    // case 4 above
    case c::cs if left(c) => step(avail, tanned, not_tanned, cs)
    // case 1 above
    case c::cs if avail > 0 => step(avail-1, tanned+c, not_tanned, cs)
    // case 2 above
    case c::cs if avail == 0 => step(avail, tanned, not_tanned+c, cs)
    // exit condition, no more customers
    case Nil => not_tanned.size
  }
}

Since the problem statement says that no customer visits more than once and everyone who doesn't get a tan leaves before the people getting tans do it isn't necessary to remove people from the sets or maintain a list of waiting customers in case a bed opens up.

The step function above needs to be called with an initial state which, assuming needs is the number of beds in the salon and customers is the string specified in the problem, is

step(needs, Set(), Set(), customers.toList)

It is possible to transform the step function into a fold which would remove the explicit list traversal, but in my opinion that would hurt the readability of the code.

A complete version of the code is

object tanning {
  def howmanyleft(nbeds: Int, customers: String): Int = {
    def step(avail: Int, tanned: Set[Char], not_tanned: Set[Char], customers: List[Char]): Int = {
      customers match {
        case c::cs if tanned(c) => step(avail+1, tanned, not_tanned, cs)
        case c::cs if not_tanned(c) => step(avail, tanned, not_tanned, cs)
        case c::cs if avail > 0 => step(avail-1, tanned+c, not_tanned, cs)
        case c::cs if avail == 0 => step(avail, tanned, not_tanned+c, cs)
        case Nil => not_tanned.size
      }
    }
    step(nbeds, Set(), Set(), customers.toList)
  }
  def main(args: Array[String]) {
    println(howmanyleft(2, "ABBAJJKZKZ"))
    println(howmanyleft(3, "GACCBDDBAGEE"))
    println(howmanyleft(3, "GACCBGDDBAEE"))
    println(howmanyleft(1, "ABCBCA"))
  }
}


Creating an IUserSession that is used throughout my code I need a bit of help on if I have coded this class correctly.

I want to handle the setting of client cookie/client store.

public class UserSession : IUserSession
{

    private readonly IReadOnlySession _repo;
    public UserSession(IReadOnlySession repo)
    {
        _repo = repo;
    }

    private string _loginId;
    public string LoginID
    {
        get { return HttpContext.Current.User.Identity.Name; }
        set { _loginId = value; }
    }

    private string _companyIdentifier;
    public string CompanyIdentifier
    {
        get { return AuthenticateCookie.GetCompanyIdentifierFromTicket(LoginID); }
        set { _companyIdentifier = value; }
    }

    /// <summary>
    /// when the client store is set, this means the user has been changed
    /// </summary>
    /// <param name="loginId"></param>
    /// <param name="identifier"></param>
    public void SetClientStore(string loginId, string identifier)
    {
        MyProfile = null;
        AuthenticateCookie.AddDetailsToCookie(loginId, identifier);

        LoginID = loginId;
        CompanyIdentifier = identifier;
    }

    public void Logout()
    {
        // we could throw an exception here?
        MyProfile = null;
        HttpContext.Current.User = null;
        FormsAuthentication.SignOut();
        //clear cookie values too?

    }

    private UserProfile _myprofile;
    public UserProfile MyProfile
    {
        get { return SetupProfile(); }
        private set { _myprofile = value; }
    }

    private UserProfile SetupProfile()
    {
        if (_myprofile == null)
        {
            if (string.IsNullOrEmpty(LoginID) || string.IsNullOrEmpty(CompanyIdentifier))
            {
                Logout();
                return null;
            }

            //call repo to get object...

            _myprofile = _repo.All<User>()
                .Where(x => x.Login == LoginID)
                .Join(_repo.All<Profile>().Where(x => x.IsActive),
                      x => x.UserID, y => y.UserID,
                      (x, y) =>
                          new
                            {
                                x.UserID, y.CompanyID, y.RoleID,
                            })
                .Join(_repo.All<Company>()
                        .Where(x => x.IsActive && x.Identifier == CompanyIdentifier),
                    x => x.CompanyID, y => y.CompanyID,
                    (x, y) =>
                        new
                          {
                              x.UserID, y.CompanyID, x.RoleID, x.IsSurveyor,
                              CompanyFriendlyName = y.Name,
                              CompanyType = y.Type
                          })
                .Join(_repo.All<Role>(), x => x.RoleID, y => y.RoleID,
                      (x, y) =>
                          new
                            {
                                x.UserID, x.CompanyID, y.RoleName, x.CompanyFriendlyName, x.CompanyType
                            })
                .Join(_repo.All<Subscription>(),
                      x => x.CompanyID, y => y.CompanyID,
                      (x, y) =>
                      new UserProfile
                          {
                              UserID = x.UserID,
                              CompanyID = x.CompanyID,
                              Role = x.RoleName.Convert<RoleName>(),
                              CompanyFriendlyName = x.CompanyFriendlyName,
                              LoginID = LoginID,
                              Identifier = CompanyIdentifier,
                              PricePlan = y.Name.Convert<PricePlanType>(),
                              CompanyType = x.CompanyType.Convert<CompanyType>()
                          })
                .SingleOrDefault();

            if (_myprofile == null)
                Logout();
        }

        return _myprofile;
    }
}

It's great that you separated the ASP.NET specific implementation from the interface. Two things to watch:

Don't forget to call Session.Abandon();Session.Clear(); in the Logout method.
SetupProfile contains a lot of code. This seems like business logic to me. Try to extract this code to its own class. An IUserService for instance. Or perhaps explicitly define Query Objects.


Two Knockout computed dependent on each other I have 3 fields:

Net Price (ex. tax)
tax amount
Total price (price ex. vat + tax amount)

The NetPrice and the Total are writable (i.e. you can change either of them and the other 2 values must be auto-calculated).

The way I've done it is using 3 observable and 2 computed knockout objects but I thought perhaps someone who knows Knockout a lot better could suggest a more efficient way to achieve this.

Working jsFiddle

HTML:

Net Price:
<input type="textbox" data-bind="value: NetPriceCalc" />

Tax Amount:

Total:
<input type="textbox" data-bind="value: TotalCalc" />

Script:

var viewModel = {
    NetPrice: ko.observable(100),
    TaxAmt: ko.observable(20),
    Total: ko.observable(120),
    TaxRate: 0.2
};

viewModel.updateTaxAmt = function (useNetPrice) {
    if (useNetPrice) {
        return this.TaxAmt(this.NetPrice() * this.TaxRate);
    } else {
        var total = Number(this.Total());
        var taxAmt = total - total / (1 + this.TaxRate);
        return this.TaxAmt(taxAmt);
    }
};
viewModel.updateNetPrice = function () {
    this.NetPrice(Number(this.Total()) - Number(this.TaxAmt()));
};
viewModel.updateTotal = function () {
    this.Total(Number(this.NetPrice()) + Number(this.TaxAmt()));
};

viewModel.NetPriceCalc = ko.computed({
    read: function () {
        console.log("NetPriceCalc read");
        return viewModel.NetPrice();
    },
    write: function (value) {
        console.log("NetPriceCalc write");
        viewModel.NetPrice(value);
        viewModel.updateTaxAmt(true);
        return viewModel.updateTotal();
    }
});
viewModel.TotalCalc = ko.computed({
    read: function () {
        console.log("TotalCalc read");
        return viewModel.Total();
    },
    write: function (value) {
        console.log("TotalCalc write");
        viewModel.Total(value);
        viewModel.updateTaxAmt(false);
        return viewModel.updateNetPrice();
    }
});

ko.applyBindings(viewModel);

I will review and contrast the SO answer that I liked best:

function viewModel() {
    var self = this;

    self.NetPrice = ko.observable(100);

    self.TaxRate = 0.2;

    self.TaxAmt = ko.computed(function() {
        return parseFloat(self.NetPrice()) * self.TaxRate;
    });

    self.Total = ko.computed({
        read: function() {
               return parseFloat(self.NetPrice()) + self.TaxAmt();
        },
        write: function(val){
                var total = parseFloat(val);
                var taxAmt = total - total / (1 + self.TaxRate);
                self.NetPrice(total - taxAmt);
        }
    });
}

It is more standard to create a constructor, then to create an object with object notation and then add functions.
Not to big a fan of using self everywhere instead of this in the SO answer
Do not use console.log in production code
Do not use calc as part of the variable name
Do not use a useNetPrice type of indicator, knockout can figure out the computed values on its own and knows when to recalculate what
write functions do not have to return anything


Efficient mechanism to generate operator overload functions I'm attempting to tidy up a C++ framework (PyCXX).

One particular file contains ~400 lines of operator overload functions, which I've managed to reduce to the following:

#define OP( op, l, r, cmpL, cmpR ) \
    bool operator op( l, r ) { return cmpL op cmpR; }

#define OPS( l, r, cmpL, cmpR ) \
    OP( !=, l, r, cmpL, cmpR ) \
    OP( ==, l, r, cmpL, cmpR ) \
    OP( > , l, r, cmpL, cmpR ) \
    OP( >=, l, r, cmpL, cmpR ) \
    OP( < , l, r, cmpL, cmpR ) \
    OP( <=, l, r, cmpL, cmpR )

    OPS( const Long &a, const Long &b,   a.as_long() , b.as_long() )
    OPS( const Long &a,       int   b,   a.as_long() , b           )
    OPS( const Long &a,       long  b,   a.as_long() , b           )
    OPS( int         a, const Long &b,   a           , b.as_long() )
    OPS( long        a, const Long &b,   a           , b.as_long() )

#ifdef HAVE_LONG_LONG
    OPS( const Long &a, PY_LONG_LONG b,   a.as_long_long() , b                )
    OPS( PY_LONG_LONG a, const Long &b,   a                , b.as_long_long() )
#endif

    //------------------------------------------------------------
    // compare operators
    OPS( const Float &a, const Float &b,   a.as_double() , b.as_double() )
    OPS( const Float &a, double       b,   a.as_double() , b             )
    OPS( double a,       const Float &b,   a             , b.as_double() )

}    // end of namespace Py

However, I wonder if it may be possible to tidy it up further.

It looks as though I can save having to pass all the parameters each time by using:

#define OPS( ... ) \
    OP( !=, ##__VA_ARGS__ ) \
    OP( ==, ##__VA_ARGS__ ) \
    OP( > , ##__VA_ARGS__ ) \
    OP( >=, ##__VA_ARGS__ ) \
    OP( < , ##__VA_ARGS__ ) \
    OP( <=, ##__VA_ARGS__ )

Although I have a hunch this might be making the code less transparent.

Another possible idea would involve storing pairs:

#define FD(op, x) (       Float &x , op, x.as_double()    )
#define L1(op, x) ( const Long &x  , op, x.as_long()      )
#define L2(op, x) ( const Long &x  , op, x.as_long_long() )
:

but I am risking obfuscation through trying too hard.  I think it is commonly accepted that other things being equal, readability trumps concise.

What other viable design patterns could I consider?

I was looking a bit at your PyCXX and want to offer you Barton–Nackman trick using Curiously recurring template pattern together with variadic template recursion (through partial specialization). This way you can solve all your types and conversion operators with one template:

template<typename T> struct Arithmetic;
using Long = Arithmetic<long long>;
using Double = Arithmetic<long double>;

template<class Base, class... Other> struct compare_operators {};

template<class Base, class First, class... Other>
  struct compare_operators<Base, First, Other...>
  : compare_operators<Base, Other...>
{
    friend bool operator == (const Base& a, First b) {
        return a.template as<First>() == b; }
    friend bool operator != (const Base& a, First b) {
        return a.template as<First>() != b; }
    friend bool operator <  (const Base& a, First b) {
        return a.template as<First>() <  b; }
    friend bool operator <= (const Base& a, First b) {
        return a.template as<First>() <= b; }
    friend bool operator >  (const Base& a, First b) {
        return a.template as<First>() >  b; }
    friend bool operator >= (const Base& a, First b) {
        return a.template as<First>() >= b; }

    friend bool operator == (const Base& a, const Arithmetic<First>& b) {
        return a.template as<First>() == b.value; }
    friend bool operator != (const Base& a, const Arithmetic<First>& b) {
        return a.template as<First>() != b.value; }
    friend bool operator <  (const Base& a, const Arithmetic<First>& b) {
        return a.template as<First>() <  b.value; }
    friend bool operator <= (const Base& a, const Arithmetic<First>& b) {
        return a.template as<First>() <= b.value; }
    friend bool operator >  (const Base& a, const Arithmetic<First>& b) {
        return a.template as<First>() >  b.value; }
    friend bool operator >= (const Base& a, const Arithmetic<First>& b) {
        return a.template as<First>() >= b.value; }
};

template<typename T> struct Arithmetic
  : compare_operators<Arithmetic<T>
  , long long, int, short, double, float, long double>
{
    T value;
    Arithmetic(T value): value(value) {}

    template<typename X, typename = enable_if_t<is_arithmetic<decay_t<X>>::value>>
      X as() const { return (X)value; }
};

int main() {
    Long val(3);
    if(val > 2) cout << "val > 2\n";
    if(val <= 3.2) cout << "val <= 3.2\n";
    Double dbl(3.14);
    if(val < dbl) cout << "val < dbl\n";
}

Beware that the comparision operators need some fine-tuning especially when you compare integral vs. floating-point (which to choose as common type) and that dacay_t in conversion operator may be problematic (not sure). (Finally imagine using namespace std, but I use my own header while explicitly using selected std features/classes in the namespace and then using the namespace).

BTW: This would be the same (but the as conversion is altered) in my own syntax, which you may find a bit familiar:

#include "basics.hpp"
using*firda

forward template<typename T> struct Arithmetic
using Long = Arithmetic<long long>
using Double = Arithmetic<long double>

template<class Base, class... Other>
  struct compare_operators

template<class Base, class First, class... Other>
  struct compare_operators<Base, First, Other...>
  : compare_operators<Base, Other...>

    friend bool operator == (const Base& a, First b)
        return a.template as<First>() == b
    friend bool operator != (const Base& a, First b)
        return a.template as<First>() != b
    friend bool operator <  (const Base& a, First b)
        return a.template as<First>() <  b
    friend bool operator <= (const Base& a, First b)
        return a.template as<First>() <= b
    friend bool operator >  (const Base& a, First b)
        return a.template as<First>() >  b
    friend bool operator >= (const Base& a, First b)
        return a.template as<First>() >= b

    friend bool operator == (const Base& a, const Arithmetic<First>& b)
        return a.template as<First>() == b.value
    friend bool operator != (const Base& a, const Arithmetic<First>& b)
        return a.template as<First>() != b.value
    friend bool operator <  (const Base& a, const Arithmetic<First>& b)
        return a.template as<First>() <  b.value
    friend bool operator <= (const Base& a, const Arithmetic<First>& b)
        return a.template as<First>() <= b.value
    friend bool operator >  (const Base& a, const Arithmetic<First>& b)
        return a.template as<First>() >  b.value
    friend bool operator >= (const Base& a, const Arithmetic<First>& b)
        return a.template as<First>() >= b.value

template<typename T> struct Arithmetic
  : compare_operators<Arithmetic<T>
  , long long, int, short, double, float, long double>

    T value
    Arithmetic(T value): value(value) {}

    template<typename X>
      enable_if_t<is_arithmetic<X>::value,
      X> as() const
        return value

int main()
    Long val(3)
    if val > 2; cout << "val > 2\n"
    if val <= 3.2; cout << "val <= 3.2\n"
    Double dbl(3.14)
    if val < dbl; cout << "val < dbl\n"


Comparing an active record result with an array of object in a rspec test Is the test flexible?

  describe "self.sort" do
    before(:each) do
      @tee = FactoryGirl.create :author, nickname: "tee jia hen", user: FactoryGirl.create(:user)
      @jon = FactoryGirl.create :author, nickname: "jon", user: FactoryGirl.create(:user)
      @tee_article1 = FactoryGirl.create :article, author: @tee, title: "3diablo"
      @tee_article2 = FactoryGirl.create :article, author: @tee, title: "1people"
      @jon_article = FactoryGirl.create :article, author:@jon, title: "2game"
    end

    it ", it should sort articles base on title" do
      Article.sort("title", "asc").should == [@tee_article2, @jon_article, @tee_article1]
    end

  end

Provided you sort the active record results (as you have done), comparing to an array works fine.  I'm not sure what you mean by 'flexible', but the test you've written looks pretty good (without knowing the internals of your 'sort' function).

I'd only suggest adding some more tests to cover reverse sorting, sorting by author.nickname (if that's part of the default scope, or pulled in via delegation), and passing invalid values to the sort function.

Being a little pedantic, you don't need to repeat the word "it" in the spec declaration.
The word "it" will be prepended to the message in the case of failures.

  describe "self.sort" do
    before(:each) do
      @tee = FactoryGirl.create :author, nickname: "tee jia hen", user: FactoryGirl.create(:user)
      @jon = FactoryGirl.create :author, nickname: "jon", user: FactoryGirl.create(:user)
      @tee_article1 = FactoryGirl.create :article, author: @tee, title: "3diablo"
      @tee_article2 = FactoryGirl.create :article, author: @tee, title: "1people"
      @jon_article = FactoryGirl.create :article, author:@jon, title: "2game"
    end

    it "should sort articles base on title" do
      Article.sort("title", "asc").should == [@tee_article2, @jon_article, @tee_article1]
    end

    it "should reverse-sort articles base on title" do
      Article.sort("title", "desc").should == [@tee_article1, @jon_article, @tee_article2]
    end
  end


Scala traits for arithmetic expressions I am learning Scala and want to know, what is preferred style of writing a function for a trait.

First:

sealed trait Expression

final case class Addition(left: Expression, right: Expression) extends Expression

final case class Subtraction(left: Expression, right: Expression) extends Expression

final case class Division(left: Expression, right: Expression) extends Expression

final case class SquareRoot(value: Expression) extends Expression

final case class Number(value: Double) extends Expression

object Expression {

  def eval(e: Expression): Calculation =
    e match {
      case Addition(l1, r1) => eval(l1) match {
        case Failure(rea1) => Failure(rea1)
        case Success(res1) => eval(r1) match {
          case Failure(rea2) => Failure(rea2)
          case Success(res2) => Success(res1 + res2)
        }
      }
      case Subtraction(l1, r1) => eval(l1) match {
        case Failure(rea1) => Failure(rea1)
        case Success(res1) => eval(r1) match {
          case Failure(rea2) => Failure(rea2)
          case Success(res2) => Success(res1 - res2)
        }
      }
      case Division(l1, r1) => eval(r1) match {
        case Failure(rea2) => Failure(rea2)
        case Success(res2) => res2 match {
          case 0.0 => Failure("Division by zero")
          case _ => eval(l1) match {
            case Failure(rea1) => Failure(rea1)
            case Success(res1) => Success(res1 / res2)
          }
        }
      }
      case SquareRoot(v) => eval(v) match {
        case Failure(rea) => Failure(rea)
        case Success(res) => res match {
          case x if x < 0 => Failure("Square root of negative number")
          case y => Success(math.sqrt(y))
        }
      }
      case Number(v) => Success(v)
    }

}

Second:

sealed trait Expression {
  def eval: Calculation =
    this match {
      case Addition(l, r) =>
        l.eval match {
          case Failure(reason) => Failure(reason)
          case Success(r1) =>
            r.eval match {
              case Failure(reason) => Failure(reason)
              case Success(r2) => Success(r1 + r2)
            }
        }
      case Subtraction(l, r) =>
        l.eval match {
          case Failure(reason) => Failure(reason)
          case Success(r1) =>
            r.eval match {
              case Failure(reason) => Failure(reason)
              case Success(r2) => Success(r1 - r2)
            }
        }
      case Division(l, r) =>
        l.eval match {
          case Failure(reason) => Failure(reason)
          case Success(r1) =>
            r.eval match {
              case Failure(reason) => Failure(reason)
              case Success(r2) =>
                if(r2 == 0)
                  Failure("Division by zero")
                else
                  Success(r1 / r2)
            }
        }
      case SquareRoot(v) =>
        v.eval match {
          case Success(r) => if(r < 0)
            Failure("Square root of negative number")
          else
            Success(Math.sqrt(r))
          case Failure(reason) => Failure(reason)
        }
      case Number(v) => Success(v)
    }
}
final case class Addition(left: Expression, right: Expression) extends Expression
final case class Subtraction(left: Expression, right: Expression) extends Expression
final case class Division(left: Expression, right: Expression) extends Expression
final case class SquareRoot(value: Expression) extends Expression
final case class Number(value: Int) extends Expression

In terms of you where to put the eval function, both are fine really. I have seen bits of both in libraries likes cats or scalaz. And given how easily you get one from the other, it does not make a massive difference architecturally.

However, I see a few issues on the side of validating your expressions, that I am honestly acking to improve :)

1) Both sides of an expression can result in an error, but you will show only one of them to the user.

2) These nested match-case expressions can be massively simplified by using one of the many functional validation libraries.

Let me attempt to solve both of these by using Scalactic's validation:

import org.scalactic._
import Accumulation._

object Arithmetics extends App {

  sealed trait Expression {
    def eval: Double Or Every[String] = this match {
      case Addition(l, r) => withGood(l.eval, r.eval)(_ + _)
      case Subtraction(l, r) => withGood(l.eval, r.eval)(_ - _)
      case Division(l, r) =>
        def isNonZero(i: Double) = if (i != 0) Pass else Fail(s"divisor evaluated to zero: $r")
        withGood(l.eval, r.eval.when(isNonZero))(_ / _)
      case SquareRoot(v) =>
        def isGreaterThanZero(i: Double) = if (i >= 0) Pass else Fail(s"Expression under square root evaluated to $i (<0): $v")
        v.eval.when(isGreaterThanZero).map(Math.sqrt)
      case Number(v) => Good(v)
    }
  }
  final case class Addition(left: Expression, right: Expression) extends Expression
  final case class Subtraction(left: Expression, right: Expression) extends Expression
  final case class Division(left: Expression, right: Expression) extends Expression
  final case class SquareRoot(value: Expression) extends Expression
  final case class Number(value: Int) extends Expression

  val failing = Addition(SquareRoot(Number(-5)), Division(Number(1), Subtraction(Number(1), Number(1))))
  println(failing.eval)
  // Bad(Many(Expression under square root evaluated to -5.0 (<0): Number(-5), divisor evaluated to zero: Subtraction(Number(1),Number(1))))

  val valid = Addition(SquareRoot(Number(25)), Division(Number(1), Number(2)))
  println(valid.eval)
  //Good(5.5)
}

You can see that you can get multiple errors at once, and much fewer lines of code, plus a human readable syntax.

In case you want to stick with your custom Calculation, it could look like this:

object Arithmetics2 extends App {

  sealed trait Calculation[A] {
    def map[B](f: A => B): Calculation[B] = this match {
      case Success(value) => Success(f(value))
      case Failure(errors) => Failure(errors)
    }
    def zip[B](other: Calculation[B]): Calculation[(A, B)] = (this, other) match {
      case (Success(valueA), Success(valueB)) => Success((valueA,valueB))
      case (Failure(errorsA), Failure(errorsB)) => Failure(errorsA ++ errorsB)
      case (_, Failure(errors)) => Failure(errors)
      case (Failure(errors), _) => Failure(errors)
    }
    def filter(f: A => Boolean, msg: A => String): Calculation[A] = this match {
      case Success(value) if f(value) => Success(value)
      case Success(value) => Failure(List(msg(value)))
      case Failure(errors) => Failure(errors)
    }
  }
  case class Success[A](value: A) extends Calculation[A]
  case class Failure[A](errors: List[String]) extends Calculation[A]

  sealed trait Expression {
    def eval: Calculation[Double] = this match {
      case Addition(l, r) => l.eval.zip(r.eval).map{ case (a,b) => a + b}
      case Subtraction(l, r) => l.eval.zip(r.eval).map{ case (a,b) => a - b}
      case Division(l, r) =>
        l.eval.zip(r.eval.filter(_ != 0, _ => s"divisor evaluated to zero: $r")).map{ case (a,b) => a / b}
      case SquareRoot(v) =>
        v.eval.filter(_ >= 0, i => s"Expression under square root evaluated to $i (<0): $v").map(Math.sqrt)
      case Number(v) => Success(v)
    }
  }
  final case class Addition(left: Expression, right: Expression) extends Expression
  final case class Subtraction(left: Expression, right: Expression) extends Expression
  final case class Division(left: Expression, right: Expression) extends Expression
  final case class SquareRoot(value: Expression) extends Expression
  final case class Number(value: Int) extends Expression

  val failing = Addition(SquareRoot(Number(-5)), Division(Number(1), Subtraction(Number(1), Number(1))))
  println(failing.eval)
  // Failure(List(Expression under square root evaluated to -5.0 (<0): Number(-5), divisor evaluated to zero: Subtraction(Number(1),Number(1))))

  val valid = Addition(SquareRoot(Number(25)), Division(Number(1), Number(2)))
  println(valid.eval)
  //Success(5.5)
}

Notice that it actually is not that lengthy/hard to make some decent validation mechanism.
Once you get a bit more into functional programming, you will see that functions like map, filter or zip can be found in many places. You will find them for example on most collections, like List.


Selecting multiple images I wrote this code to allow users to select multiple images, and each image can be selected multiple times. It feels convoluted, though, and prone to errors, such as forgetting to do something associated with an action.

How can I make it more object-oriented or reusable?

var images = [];

function removeImage(image) {
    while((index = images.indexOf(image)) !== -1) {
        images.splice(index, 1);
    }
}
function decrementImage(image) {
    var index = images.indexOf(image);
    images.splice(index, 1);
}

function countOccurrences(image) {
    var occurrences = 0;
    for (i = 0; i < images.length; i++) {
        if (images[i] === image) occurrences += 1
    }
    return occurrences;
}

$('#image-container').on('click', 'img', function () {
    var image = $(this),
        source = image.attr('src');
        image.data('selected', true).addClass('selected').siblings('.controls').show();
        images.push(source);
        image.siblings('span.count').html(countOccurrences(source));
});

$('#image-container').on('mouseleave', '.image-wrapper', function () {
    $(this).children('.controls').hide();
});

$('#image-container').on('mouseenter', 'img', function () {
    var image = $(this),
        controls = image.siblings('.controls');
    if (image.data('selected') === true) {
        controls.show();
    }
});

$('#image-container').on('click', '[data-action="remove"]', function () {
    var controls = $(this).parents('div.controls'),
        image = controls.siblings('img')
        source = image.attr('src');
        image.data('selected', false).removeClass('selected');
        controls.hide();
        removeImage(source);
        image.siblings('span.count').html(countOccurrences(source));
});

$('#image-container').on('click', '[data-action="increment"]', function () {
    var image = $(this).parents('.controls').siblings('img'),
        source = image.attr('src');
    images.push(source);
    image.siblings('span.count').html(countOccurrences(source));
});

$('#image-container').on('click', '[data-action="decrement"]', function () {
    var image = $(this).parents('.controls').siblings('img'),
        source = image.attr('src');
    decrementImage(source);
    var count = countOccurrences(source)
    image.siblings('span.count').html(count);
    if (count < 1) {
        image.siblings('.controls').hide();
        image.data('selected', false).removeClass('selected');
    }
});

$(document).click(function () {
    console.log(images);
});

HTML

            Remove
            Increment
            Decrement

        0

            Remove
            Increment
            Decrement

        0

            Remove
            Increment
            Decrement

        0

Your code is actually very good.

I'd advice against using OOP there. It's an overused tool which, when not necessary, only makes things more complex. JavaScript works very well without it. I'd recommend a more functional style. So, if you want to make your code more reusable, you could abstract your functions to work with any kind of collection. You could also use existing functions to define others:

function removeFirst(array,obj) {
    array.splice(array.indexOf(obj), 1);
};
function without(array,obj){
    return array.filter(function(element){
        return element !== obj;
    });
};
function count(array,obj){
    return array.length - without(array,obj).length;
};

Then, counting an image would be, for example:

count(images,my_image);

But before I'd suggest checking some functional JavaScript libraries such as underscore.js, which already implement similar functions in very clever manners. Using them in your daily code will make it smaller, simpler, more readable and robust.


LINQ foreach - error handling and general improvement I'm looking for two kinds of feedback:

Is there a better way of doing what I'm attempting to do?
Any obvious weaknesses that could potentially cause problems?

The code in question (I've broken it into lines to prevent horizontal scroll):

items.ForEach(i =>
                i.JobOrderTypeString = Context.JobOrderTypes.Where(x =>
                    x.JobOrderTypeIdentifier == i.JobOrderTypeIdentifier.CodeListItemIdentifier)
                    .FirstOrDefault().Title);

Datatypes:

items is a List of simple ViewModels

My worries:

If FirstOrDefault() returns null (though very unlikely), and I'm "reading" the .Title property, will my code crash?
I'm using Context on each iteration, which I'm suspecting is probably not the best idea.

First, the least important: Personally I don't like using the .ForEach extension method as it looks too much like LINQ query commands which are mainly for projection/filtering. I just use a straight-up foreach loop most of the time. But that's just style points.

You can simplify .Where(lambda).FirstOrDefault() to FirstOrDefault(lambda).

You're referencing context every time, but you don't have to, you're right. You could enumerate over context.JobOrderTypes and store it in memory before iterating. Chances are it's going to be more performant than hammering your db every iteration. You are pulling back an entire table into memory, though, so be aware of the implications of that.

Your code is going to crash but you can fix it either very simply:

var val = ...FirstOrDefault(x => x.JobOrderTypeIdentifier == identifier);
i.JobOrderTypeString = val != null ? val.Title : "SomeDefaultValue";

Or you can flatten to an IEnumerable of Titles first (essentially saying "Give me the first of all Titles of objects that match my criteria" instead of saying "Give me the Title of the first object that matches my criteria, if no matches then null"):

i.JobOrderTypeString = context.JobOrderTypes.Where(x => x.JobOrderTypeIdentifier == identifier).SelectMany(j => j.Title).FirstOrDefault();

I'm not an EF expert so I'm not sure if the following will generate the same SQL, but it's cleaner LINQ:

i.JobOrderTypeString = context.JobOrderTypes.SelectMany(j => j.Title).FirstOrDefault(x => x.JobOrderTypeIdentifier == identifier);

Generally code that has a hydrated collection, but then goes through and iterates through the collection and sets values on each element in the collection means that you can do it better. I don't know where this code lives, but make sure it's in a place that says "right before I write these to the database, calculate!" You could also do this computation whenever the column value on i changes or when i is instantiated.

This may be a sign that your problem could be better solved by using a stored proc or a computed column, or you need to wrap the in a "BeforeInsert"-type event.


isRTL.coffee library to determine if a text is of right-to-left direction I just wrote this tiny library called isRTL.coffee to determine the direction of the text. Is there any better way of doing this?

rtlChars = '\u0600-\u06FF' # Arabic - Range

rtlChars += '\u0750-\u077F' # Arabic Supplement - Range

rtlChars += '\uFB50-\uFDFF' # Arabic Presentation Forms-A - Range

rtlChars += '\uFE70-\uFEFF' # Arabic Presentation Forms-B - Range

reRTL = new RegExp "^[#{rtlChars}]"

window.isRTL = (value) ->
    if value.match reRTL
        true
    else
        false

Yep, it's possible to improve. You can simply do:

window.isRTL = (value) ->
    reRTL.test value

Another way to improve is the way you declare your special characters. Right now, it's confusing between the = and +=. Here is another way:

rtlChars = [
    '\u0600-\u06FF' # Arabic - Range
    '\u0750-\u077F' # Arabic Supplement - Range
    '\uFB50-\uFDFF' # Arabic Presentation Forms-A - Range
    '\uFE70-\uFEFF' # Arabic Presentation Forms-B - Range
].join("")

reRTL = new RegExp "^[#{rtlChars}]"

This way, adding a new character is simply adding a line in an array.


JQuery Class and Display Toggling I know there is a better way to do the following but I havent had any luck trying to clean it up.

This is for a table of messages where the user can click a row, message-head, which will display the message-body-hidden to the user as well as change the class of message-head to message-head-active. It will also change the fontawesome icon. Along with that, any rows that are currently being displayed or are -active will be reverted back to their defaults.

How could I clean this up or more easily perform the same tasks?

$("table").on("click", "tr.message-head", function () {
    var active = $(this)
    active.toggleClass('message-head-active')

    if($("i",this).hasClass("fa-envelope-open")){
        $("i",this).removeClass("fa-envelope-open")
        $("i",this).addClass("fa-envelope")
    }else{
        $("i",this).removeClass("fa-envelope")
        $("i",this).addClass("fa-envelope-open")
    }

    var showMe = active.next().find('.message-body-hidden')
    showMe.toggle()

    $("tr.message-head-active").not(active).removeClass('message-head-active')
    $("tr.message-head-active").not(active).addClass('message-head')

    $("tr.message-head").not(active).find('i').removeClass("fa-envelope-open")
    $("tr.message-head").not(active).find('i').addClass("fa-envelope")

    $("td.message-body-hidden").not(showMe).hide()
})

Move your revert all logic to the top of the function to avoid having to exclude the newly active item.

Use a stateful class such as .is-active instead of message-head-active. Means you only have to toggle 1 class rather than 2.

toggleClass accepts multiple arguments so you can toggle the FontAwesome classes in one line. This also removes the need for the hasClass check.

Store the result of $("tr.message-head-active") to a variable and reuse it when reverting the header and the icon.


Singleton method for Redis connection in Python Here's my class for providing a singleton instance of a Redis connection. What do you think?

from redis import StrictRedis
import os

class RedisConnection(object):
    REDIS_URL = os.environ['REDIS_URL']

    def __init__(self):
        self._redis = StrictRedis.from_url(self.REDIS_URL)

    def redis(self):
        return self._redis

    the_instance = None
    @classmethod
    def singleton(cls):
        if cls.the_instance == None:
            print "assigning!"
            cls.the_instance = cls().redis()
        return cls.the_instance

The Singleton pattern is a way to work around the absence of global variables in Java. But Python has global variables, and it's usually possible to write shorter and simpler initialization code if you use them, for example:

import os

from redis import StrictRedis

_connection = None

def connection():
    """Return the Redis connection to the URL given by the environment
    variable REDIS_URL, creating it if necessary.

    """
    global _connection
    if _connection is None:
        _connection = StrictRedis.from_url(os.environ['REDIS_URL'])
    return _connection

Alternatively, you could memoize the function, for example using functools.lru_cache:

from functools import lru_cache
import os

from redis import StrictRedis

@lru_cache(maxsize=1)
def connection():
    """Return the Redis connection to the URL given by the environment
    variable REDIS_URL, creating it if necessary.

    """
    return StrictRedis.from_url(os.environ['REDIS_URL'])


Remaking std::to_string in C++ with UDLs (User Defined Literals) I would like some feedback on what I should change with my semi remake of std::to_string with UDLs (user-defined literals).

This is compiled and working on linux with C++11 using g++.

#include <deque> // if you use vectors instead you have to add them to the
//string in reverse order (no push_front())
#include <string>

std::string operator"" _str(unsigned long long int num)
{
  std::deque<char> digits;
  for(char i = 0; num != 0;num = num / 10)
  {
      i = '0'+(num % 10);
      digits.push_front(i);
  }
  std::string converternum;
  for(auto i = 0; i< digits.size();i++)
    converternum += digits[i];
  return converternum;
}

char * operator"" _cstr(unsigned long long int num)
{
   std::deque<char> digits;
   for(char i = 0; num != 0;num = num / 10)
   {
     i = '0'+(num % 10);
     digits.push_front(i);
   }
   std::string converternum;
   for(auto i = 0; i< digits.size();i++)
   converternum += digits[i];
   char * string = (char *)converternum.c_str();
   return string;//converternum.c_str();
}

I'm ignoring all the testing stuff, and just focusing on the two conversion functions.

std::string operator"" _str(unsigned long long int num)
{
    std::deque<char> digits;

There's really no need for the deque. You can push_back() all the digits directly into a std::string, and then use std::reverse().

To make it even better, you could use the max value of an unsigned long long and calculate the maximum number of digits you'll need, then use reserve(). But that probably won't net you any real gains in reality, since you'll likely always be under the small-string optimization size where std::string won't actually allocate any memory... another reason to ditch the deque and stick with strings.

for(char i = 0; num != 0;num = num / 10)

Have you tested your conversion functions with zero? Because if num is zero, this loop won't fire at all, and digits will be empty. So "0"_str will result in "".

This is a pretty weird and confusing loop in general. i looks like it's going to be some kind of loop counter, but it's not. All-in-all, this just looks like a case of trying to be too clever, and outsmarting yourself (due to the zero issue).

This could be expressed much more simply and clearly as something like:

do
{
    digits.push_front('0' + (num % 10));
    num /= 10;
} while (num != 0);

and it won't have the zero bug. (do loops are less common than while, but the reason to use one here - and this should be documented in a comment - is to guarantee that even if num is zero, the loop will fire at least once.)

{snipped section about digit character codes}

std::string converternum;

I already mentioned that the deque is unnecessary, but if you're really going to use it, you should take advantage of the fact that you know digits.size(), and use reserve() for this string to prevent unnecessary allocations.

char * operator"" _cstr(unsigned long long int num)

This function is mostly identical to the first one, and thus has the same problems. Again, I would ditch the deque and just work with a string. The digits will be in reverse order, but in this case you can fix that with std::reverse_copy() when you copy them into your output buffer.

char * string = (char *)converternum.c_str();
return string;//converternum.c_str();

Here is where you get into real trouble.

First, never, ever use C-style casts. In this case, you've cast away the constness of c_str()... that's bad. Prior to C++17, if you want a non-const pointer to std::string's data, you have to jump through some hoops - you have to do something like &convertnum[0] or &convertnum.front() - and I agree that's a pain. But casting away constness is a red flag that won't pass most code reviews, because it is extremely dangerous.

Second, you are taking a pointer to convertnum's internal data, and returning that... even though convertnum is being destroyed. In other words, you are returning a pointer to freed memory, which is UB.

How do you solve these problems? Well, the don't answer is: Don't use C-style strings. In other words, don't have a _cstr UDL. It's only going to give you grief (it already has!).

But if you really, really must... one option is to do what @hoffmale suggests, and instead of char*, return std::unique_ptr<char[]>. That might look something like this:

char* operator"" _cstr(unsigned long long int num)
{
    auto digits = std::string{};
    do
    {
        digits += '0' + (num % 10);
        num /= 10;
    } while (num != 0);

    auto p = std::make_unique<char[]>(digits.size() + 1);

    std::reverse_copy(digits.cbegin(), digits.cend(), p.get());
    p[digits.size()] = '\0'; // have to manually add the null terminator

    return p;
}

The problem with that is you're now longer returning a C-string, which makes "_cstr" kind of a lie.

So another option is to have some kind of storage with a lifetime longer than the function that will retain the memory for you. Something like:

char* operator"" _cstr(unsigned long long int num)
{
    // Note: you *can't* use a vector<string>, because string's
    // memory may be invalidated on moves, like if the vector
    // resizes.
    static auto storage = std::vector<std::unique_ptr<char[]>>{};

    auto digits = std::string{};
    do
    {
        digits += '0' + (num % 10);
        num /= 10;
    } while (num != 0);

    auto p = std::make_unique<char[]>(digits.size() + 1);

    std::reverse_copy(digits.cbegin(), digits.cend(), p.get());
    p[digits.size()] = '\0';

    // Get the char*, and then move the unique_ptr into storage
    // to let it be managed there.
    auto result = p.get();
    storage.push_back(std::move(p));

    return result;
}

This will "work", and it's basically what some C libraries do (when they don't simply leak). But with no way to release the memory, it will just grow and grow. You could add a mechanism to release the memory, but you would need a way to be sure it's safe for every string. This solution is possible, but unwieldy.

The last solution - the worst case (other than outright UB) - is to define some sort of contract that client code must follow. In plain English, document the function by saying "you must delete[] the memory when you're done with it". That requires programmers to behave, and is very easy to break, so it is not recommended. But... if you really want a C-string function, it's an option. A bad one, but an option nonetheless.

Summary

You have one critical error in your code:

In ""_cstr(unsigned long long), you return a pointer to the internal memory of a std::string that gets destroyed at the end of the function, meaning you're returning a pointer to invalid memory.

You have a couple of logical errors in your code:

Both functions will return an empty string when the number is zero, instead of "0".

You have a few inefficiencies:

You use deques because you want to add the numbers in reverse order, and reason that makes sense because neither strings nor vectors have push_front(). But that's all unnecessary. You can simply push_back() directly into your result string, and then use std::reverse(). (Or push_back() into a string buffer, and std::reverse_copy() into your final buffer.)

And here are some tips to keep in mind:

It's really good that you're doing testing, but if you're going to do that, you might as well go all in and use something like Google Test. It's header-only, a single include, and it produces very pretty output. A full battery of tests can give you great confidence in your code. Using Google Test might look something like (fair warning, I don't actually use Google Test that often):

TEST(StrTest, HandlesZero) {
    EXPECT_EQ(0_str, "0");
}

To test that the types are right, rather than trying to demangle the names (which, by the way, it's been a long time since I've used abi::__cxa_demangle(), but I'm pretty sure you're not using it correctly), it's so much easier to just use type traits:

TEST(StrTest, ReturnType) {
    EXPECT_TRUE((std::is_same_v<decltype(0_str), std::string>));
}

Another dirty trick you can use to examine types is deliberately generating errors:

template <typename T>
struct TD; // note, never define TD

TD<decltype(0_str)> t; // will trigger an error

The error generated will print the exact type of whatever is in the angle brackets, on every compiler. This is a hackish way to check types, but it can help when you're experimenting.


ListView MultiSelect, MVVM and RoutedCommands As far as I know (and I don't claim to know much about this!), direct binding to ListView.SelectedItems isn't possible in WPF. I've seen work-arounds involving code-behind which I'm not too crazy about, especially since I'm having a hard time with getting a DelegateCommand to work, and decided to use RoutedCommands. For the following XAML command binding...

<UserControl.CommandBindings>
    <CommandBinding Command="{x:Static my:ClientSearchSection.AddSelectionCommand}"
                    CanExecute="CanExecuteAddSelectionCommand"
                    Executed="ExecuteAddSelectionCommand"/>
</UserControl.CommandBindings>

...I have the following code-behind:

public static readonly RoutedCommand AddSelectionCommand = new RoutedCommand();

private void CanExecuteAddSelectionCommand(object sender, CanExecuteRoutedEventArgs e)
{
    if (Commands != null)
    {
        Commands.SelectedClients = SearchResultsList.SelectedItems.Cast<ClientViewModel>().ToList();
        e.CanExecute = Commands.CanExecuteAddSelectionCommand();
    }
    e.Handled = true;
}

private void ExecuteAddSelectionCommand(object sender, ExecutedRoutedEventArgs e)
{
    Commands.SelectedClients = SearchResultsList.SelectedItems.Cast<ClientViewModel>().ToList();
    Commands.OnExecuteAddSelectionCommand();
    e.Handled = true;
}

Where Commands is a get-only private property that returns an interface implemented by the ViewModel, which defines all the CanExecuteXXXX and ExecuteXXXX methods.

Here's how I have it:

private IViewModel _viewModel;
private IClientsSearchSectionCommands Commands { get { return _viewModel as IClientsSearchSectionCommands; } }

public ClientSearchSection()
{
    InitializeComponent();
    DataContextChanged += ClientsSearchSection_DataContextChanged;
}

void ClientSearchSection_DataContextChanged(object sender, DependencyPropertyChangedEventArgs e)
{
    _viewModel = e.NewValue as IViewModel;
}

This allows me to implement the command code in the ViewModel, for example:

public bool CanExecuteAddSelectionCommand()
{
    return SelectedClients.Count > 0;
}

public void OnExecuteAddSelectionCommand()
{
    // whatever needs to happen here, I can access my model as needed
}

The trick that allows the SelectedItems to work is with this property, exposed by the IClientsSearchSectionCommands interface implemented by the ViewModel:

public IList<ClientViewModel> SelectedClients { get; set; }

...and the fact that I'm setting them in the commands' CanExecute and Executed handlers.

This works beautifully... but is it weird in any way? [How] could it be done better?

I don't want to dive into behaviors at this point, firstly because I have no clue about them, second, because I'm dragging a more junior developper into WPF & MVVM, coming from WinForms and inline-SQL-in-the-form's-code-behind, so I'd like to know if this code is easy enough to follow...

Your approach looks fine to me. Except i would probably use prefix-casting, to get cast exceptions straigt away if something goes wrong, instead of using as.

This can be achieved without modifying code-behind tho. You can bind container's IsSelected property to appropriate item's viewmodel property.

<ListView.ItemContainerStyle>
    <Style TargetType="ListViewItem" >
        <Setter Property="IsSelected" Value="{Binding IsSelected}"/>
    </Style>
</ListView.ItemContainerStyle>

Then you can access selected items in your viewmodel by using:

protected IEnumerable<Item> SelectedItems { get { return Items.Where(x => x.IsSelected); } }


Copy, Paste And Format I am currently working with this code to automate some tasks for senior staff members that are not very adept in Excel. Wondering if VBA is simply not a very quick code or if my code is clunky and slow.

For clarity, I would think with how simple this code is it could run in under a second or two. Maybe this is overzealous?

Sub Paste()

'---Paste Macro
'---2016-05-23

Dim sht1 As Worksheet
Dim sht2 As Worksheet
Dim LastRow As Long
Dim LastRow2 As Long
Dim LastColumn As Long
Dim StartCell1 As Range
Dim StartCell2 As Range
Dim rng1 As Range
Dim rng2 As Range

Set sht1 = GetWSFromCodeName("Sheet10")
Debug.Print sht1.Name
Set sht2 = GetWSFromCodeName("Sheet8")
Debug.Print sht2.Name
Set StartCell1 = Range("A2")
Set StartCell2 = Range("B2")

'Find Last Row and Column
  LastRow = sht1.Cells(sht1.Rows.Count, StartCell1.Column).End(xlUp).Row
  LastColumn = sht1.Cells(StartCell1.Row, sht1.Columns.Count).End(xlToLeft).Column
  LastRow2 = sht2.Cells(sht2.Rows.Count, StartCell1.Column).End(xlUp).Row

'Select Range And Copy into Final Formula Sheet
  sht1.Range(StartCell1, sht1.Cells(LastRow, LastColumn)).Copy Destination:=sht2.Cells(LastRow2 + 1, 2)

'Convert Text in Column C of Final Formula Sheet to Numbers to Allow Advisor Code to Apply

 Set rng1 = Range(sht2.Cells(LastRow2, 3), sht2.Cells(LastRow2 + LastRow - 1, 3))
 With rng1
    .NumberFormat = "0"
    .Value = .Value
    End With

'Copy Advisor Function down to meet with new Pasted in Data
    With sht2
        Set rng2 = .Cells(LastRow2, 1)
        End With
    With rng2
        .Copy Destination:=Range(sht2.Cells(LastRow2, 1), sht2.Cells(LastRow2 + LastRow - 1, 1))
        End With

End Sub

'---This Function allows the worksheet name to change in the workbook as it allows the
    'user to set Worksheets to codename variables. By using this function the user can input a
    'codename for a worksheet and the function will call the worksheet name of the corresponding
    'codename, allowing the user to set worksheet variables to codenames without losing
    'functionality usually associated with such variables.
'---2016-05-23

Public Function GetWSFromCodeName(CodeName As String) As Worksheet

    Dim WS As Worksheet
    For Each WS In ThisWorkbook.Worksheets
        If StrComp(WS.CodeName, CodeName, vbTextCompare) = 0 Then
            Set GetWSFromCodeName = WS
            Exit Function
        End If
    Next WS

End Function

The 3 lowest-hanging VBA performance fruit are:

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlManual

Just make sure to restore them at the end of your sub, and/or if your method encounters an error and stops, else your senior people won't be able to use Excel afterwards and will blame you for breaking it.

Used like so:

Sub/Function ()

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlManual

        < Code >

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlAutomatic '/ Assuming it was set to automatic to begin with

End Sub/Function

And with some (very basic) error handling:

Sub/Function ()

    On Error Goto CleanFail

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlManual

        < Code >

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlAutomatic '/ Assuming it was set to automatic to begin with

CleanExit:
    Exit Sub/Function

CleanFail:
    '/ Resets the Application settings, *then* raises the error
    On Error Goto 0
    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlAutomatic '/ Assuming it was set to automatic to begin with
    Err.Raise(Err.Number) '/ Or insert your own error handling here

End Sub/Function


Would this code cause memoryleaks? My question is really simple. Does this code cause memoryleaks? If so, where/how/why?

HDC hDC, memDC = 0;
HBITMAP hBMP = 0;
HBITMAP hOldBMP = 0;
PAINTSTRUCT PS;
HBRUSH hb212121, hb141414, hb070707, hb000, hbF7F7F7, hb989898, hb707070, hb494949, hb984921 = 0;
HPEN hp353535 = 0;

case WM_PAINT:
        hDC = BeginPaint(hWnd, &PS);
        memDC = CreateCompatibleDC(hDC);
        hBMP = CreateCompatibleBitmap(hDC, 450, 450);
        SelectObject(memDC, hBMP);

        hb212121 = CreateSolidBrush(RGB(33, 33, 33));
        FillRect(memDC, &rMainClntNoBorder, hb212121);

        hb494949 = CreateSolidBrush(RGB(73, 73, 73));
        hb984921 = CreateSolidBrush(RGB(152, 73, 33));
        hb000 = CreateSolidBrush(RGB(0, 0, 0));
        switch(tiles)
        {
            setTileRect();
            case 1:
                FillRect(memDC, &rTile, hb494949);
                break;
            case 2:
                FillRect(memDC, &rTile, hb984921);
                break;
            case 7:
                FillRect(memDC, &rTile, hb000);
                break;
        }
        SelectObject(memDC, hOldBMP);
        DeleteObject(hBMP);
        DeleteObject(hb984921);
        DeleteObject(hb494949);
        DeleteObject(hb212121);
        DeleteObject(hb000);

        hp353535 = CreatePen(PS_SOLID, 1, RGB(53, 53, 53));
        SelectObject(memDC, hp353535);
        GetClientRect(hWnd, &rClnt);
        MoveToEx(memDC, rClnt.left, rClnt.bottom - 1, 0);
        LineTo(memDC, rClnt.left, rClnt.top);
        LineTo(memDC, rClnt.right, rClnt.top);
        DeleteObject(hp353535);

        BitBlt(hDC, 0, 0, rMainClntNoBorder.right, rMainClntNoBorder.bottom, memDC, 0, 0, SRCCOPY);
        DeleteDC(memDC);

        EndPaint(hWnd, &PS);
        break;

you select the pen in, but don't select out before delete.

SelectObject(memDC, hp353535);
// snip
DeleteObject(hp353535)

should be:

hOldPen = SelectObject(memDC, hp353535);
// snip
SelectObject( memDC, hOldPen );
DeleteObject(hp353535)

SelectObject(memDC, hBMP); should really be hOldBMP = SelectObject(memDC, hBMP);

For some other comments:

I recommend that you split the WM_PAINT (and all other messages) processing into its own function. It'll be easier to read/debug.
Are rMainClntNoBorder (and rTile, rClnt) globals, or declared and initialised before the code supplied?
Only declare one variable per line. It's easier to read and find where it's declared rather than searching. It also avoids an issue with pointers that you may come across.
Use proper names for the brushes if you can - hbDarkOrange (or even hbCurrentActiveTile) means more than hb984921.
Unless you have hundreds of colours, it may be better to create them at the start of the program and delete them at the end, and store their values in a struct in the 'user-area' of the window structure - You may want to avoid creating and deleting them continually.
If you don't do the above, keep the creation and deletion as close together as possible.

hbDarkGrey = CreateSolidBrush(RGB(33, 33, 33));
FillRect(memDC, &rMainClntNoBorder, hbDarkGrey);
DeleteObject( hbDarkGrey );

Similarly, your switch statement could be simplified. Rather than creating three brushes and using one, just pick the one you want.

setTileRect(); // implies that rTile is a global??
switch(tiles)
{
    case 1:
        tile_colour = RGB( 73, 73, 73 );
        break;
    case 2:
        tile_colour = RGB( 152, 73, 33 );
        break;
    case 7: // allow fallthrough
    default: // you need a default!!
        tile_colour = RGB( 0, 0, 0 );
        break;
}
hbTile = CreateSolidBrush( tile_colour );
FillRect( memDC, &rTile, hbTile );
DeleteObject( hbTile );

Instead of 1 and 2 etc in the case statement, create some meaningful constants for them with enum.


Codeigniter Model Optimization I have created this model for Codeigniter. It's working fine, but I need to know if it can be more optimized.

class Basic_Function_Model extends CI_Model
{

    var $msg_invalid_array;
    var $msg_no_table;

    function __construct()
    {
        parent::__construct();
        $this->msg_invalid_array    =   "Data must be provided in the form of array";
        $this->msg_no_table         =   "Table does not exist in database";
        $this->load->database();
    }

    public function insertInDatabase($table, $insertData, $mode = "single")
    {
        if( !is_array($insertData) ) {
            return $this->msg_invalid_array;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table;
        }

        if( $mode == "batch" ) {
            return $this->db->insert_batch($table, $insertData);
        } else {
            $this->db->insert($table, $insertData);
            return $this->db->insert_id();
        }
    }

    public function updateInDatabase($table, $updateData, $conditionData)
    {
        if( !is_array($updateData) || !is_array($conditionData) ) {
            return $this->msg_invalid_array;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table;
        }

        if( $this->db->update($table, $updateData, $conditionData) )
            return $this->db->affected_rows();
        else
            return false;
    }

    public function deleteFromDatabase($table, $conditionData)
    {
        if( !is_array($conditionData) ) {
            return $this->msg_invalid_data;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table;
        }

        return $this->db->delete($table, $conditionData);
    }

    public function insertOnDuplicateKeyUpdate($table, $tableData)
    {
        if( !is_array($tableData) ) {
            return $this->msg_invalid_data;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table;
        }

        foreach( $tableData as $column => $value )
        {
            $columnNames[] = $column;
            $insertValues[] = "'".$value."'";
            $updateValues[] = $column." = '".$value."'";
        }
        $this->db->query("insert into $table(".implode(', ', $columnNames).") values(".implode(', ', $insertValues).") on duplicate key update ".implode(', ', $updateValues));
        return $this->db->insert_id();
    }

    private function validateTable($tableName)
    {
        $result = $this->db->list_tables();

        foreach( $result as $row ) {
            if( $row == $tableName )    return true;
        }
        return false;
    }
}

First of all, thanks for sharing your code, this is the best way to improve yourself!

The M in MVC

A Model in any MVC framework should provide an easy way to access a specific type of data. It allows you to stop worrying about database details and only access your specific type of data in a meaningful way. For a StackOverflow-like website, this means that your Controller can relax! Your controller doesn't want to know about INSERT INTO Questions(id, title, message, author) VALUES (9624, "Codeigniter Model Optimization", "...", "Shayan"). He just wants to do $questions->add("Codeigniter Model Optimization", "...", "Shayan") and know that all details (including security) are going to be handled by the Model.

The model should be in charge of ensuring everything goes well with the database, but that's not what you're doing here. You're creating a Leaky Abstraction, and this doesn't help you. Try coming up with a model that reduces the work the controller needs to do.

A few comments on the code itself.

You're repeating yourself a lot at the start of each function. One fix would be to put this in a function:
    $checks = $this->check($table, $conditionData);
    if ($checks !== FALSE) return $checks;
But the real fix is to remove those. $this->db->query() already removes FALSE when there's an issue. You should use it to check if there's an issue, and then decide what to do then. The handling of issues is what makes abstractions successful. Remember than "It's easier to ask for forgiveness than it is to get permission".
Please use query bindings (at then end of the page) which will avoid SQL injections. They're using PHP's prepared statements, which you should learn about if you want to write secure applications.


Should I be trying to imitate the control flow in this example R code? I have heard that some patterns in R code are really anti-patterns and thus I am trying to imitate superior examples of R code.

I am confused about control flow.

I have been reviewing some R code from openintro.org;
the original exercise is explained at:

http://www.openintro.org/download.php?file=os2_lab_02A&referrer=/stat/labs.php

One of the very early examples deals with counting "hit" or "miss" streaks within a vector of "H" and "M" characters.  We consider a winning streak to be at least one H, and each streak ends in M.  So H M M H H M counts as a streak of 1, a streak of 0, a streak of 2, etc.

So, for example, we start with a sequence like kobe$basket:

  [1] "H" "M" "M" "H" "H" "M" "M" "M" "M" "H" "H" "H" "M" "H" "H" "M" "M" "H"
 [19] "H" "H" "M" "M" "H" "M" "H" "H" "H" "M" "M" "M" "M" "M" "M" "H" "M" "H"
 [37] "M" "M" "H" "H" "H" "H" "M" "H" "M" "M" "H" "M" "M" "H" "M" "M" "H" "M"
 [55] "H" "H" "M" "M" "H" "M" "H" "H" "M" "H" "M" "M" "M" "H" "M" "M" "M" "M"
 [73] "H" "M" "H" "M" "M" "H" "M" "M" "H" "H" "M" "M" "M" "M" "H" "H" "H" "M"
 [91] "M" "H" "M" "M" "H" "M" "H" "H" "M" "H" "M" "M" "H" "M" "M" "M" "H" "M"
[109] "H" "H" "H" "M" "H" "H" "H" "M" "H" "M" "H" "M" "M" "M" "M" "M" "M" "H"
[127] "M" "H" "M" "M" "M" "M" "H"

and if we call calc_streak on that, we get:

 [1] 1 0 2 0 0 0 3 2 0 3 0 1 3 0 0 0 0 0 1 1 0 4 1 0 1 0 1 0 1 2 0 1 2 1 0 0 1 0
[39] 0 0 1 1 0 1 0 2 0 0 0 3 0 1 0 1 2 1 0 1 0 0 1 3 3 1 1 0 0 0 0 0 1 1 0 0 0 1

The "1 0 2" of the answer indicates the streak of 1, the streak of 0, the streak of 2 mentioned above.

The following function is referred to as "calc_streak" but apparently that name doesn't show up in the code listing.

function(x){
y <- rep(0,length(x))
y[x == "H"] <- 1
y <- c(0, y, 0)
wz <- which(y == 0)
streak <- diff(wz) - 1
return(streak)
}

I think the first line makes a big vector of "0" values, just as long as the input, so in this case it should be 133 entries long.

I think the second line goes through that vector and rewrites the "0" value to "1" if the input has an "H."

The third line might be putting an extra "0" at the front and back of the vector.  I think it's in there to make the "diff" work correctly, so that the first winning streak shows up as a "1" value.

I think the object "wz" is a vector of integers. (I suspect this is a short vector, just containing the indices of the entries in x that satisfy the test.)

So first and foremost, if I am misunderstanding the way this example code works, please correct me.

But my primary question is - Should I regard the example R code as a good example of control flow and imitate it in my other programs?

Thanks.

In my opinion, this function is a nice example of R code. Your interpretation of the code is correct. One unnecessary command is y == 0 since the information is redundant given that x == "H" is already known. In this case, y == 0 is identical to c(TRUE, !(x == "H"), TRUE).

The code can be simplified. The object wz can be created more easily:

calc_streak <- function(x){
  wz <- c(1, which(x == "M") + 1, length(x) + 2)
  streak <- diff(wz) - 1
  return(streak)
}

Alternatively and probabily even more efficiently, you can create a logical index indicating the positions of the "M"s in the vector and calculate streak more directly:

calc_streak <- function(x){
  idx <- x == "M"
  streak <- c(!idx[1], diff(which(idx)) - 1, !tail(idx, 1))
  return(streak)
}

The ! sign indicates logical not. If numeric and logical values are combined in one vector, the logical values are cast into numeric values. TRUE becomes 1, FALSE becomes 0.


Custom function for serialization of form inputs (and selects) Just asking to if following code is ok since i needed didn't find equivalent of .serialize() for vanilla JS. So i invented my own function for plain ol' JS.

    function uzim_vred(phpdoc,parent_id){

    var name = "";
    var val = "";
    var str = phpdoc+"?,";
    var len = ($("#"+parent_id+" [name]").length);
    var i = 0;

    $("#"+parent_id+" [name]").each(function(){

        i++;
        name = $(this).attr('name');
        val = $(this).val();

        str += name+"="+val;

        if(i<len){
            str += "&,"
        }

    });

    i = 0;
    var elems = str.split(",");

    str = "";
    while(i<=len){
        str += elems[i];
        i++
    }

        var xhttp = new XMLHttpRequest();

        xhttp.onreadystatechange = function() {

            if((document.getElementById("raport")!=undefined || document.getElementById("raport")!=null) && this.readyState == 4 && this.status == 200){
                document.getElementById("raport").innerHTML = this.responseText;
            }

        };

        xhttp.open("GET", str, false);
        xhttp.send();

}

How to envoke it:

<button type="button" onclick="uzim_vred('yourPHPscript.php',$(this).parent().attr('id'))">Send</button>

First argument is a path to and name of php script, second is used for id of form element from whom you want to "serialize" values.

You are not really implementing in plain JavaScript in that you are using jQuery inside the function. This seems to be an odd choice. Are you using jQuery or not?  You should make a decision. If you are using jQuery, then I don’t know why this function would exist at all.

The function name itself is meaningless and does not at all convey what the function does.

You are not url encoding your query string.

You are not handling case where your asynchronous fails to return 200 response.

You are using loop counters where you don’t really need to.

It makes no sense how you build a query string then split it apart and then reassemble it.

Code style is problematic:

inconsistent indentation
lines of code that are too long


Blink Text with JavaScript My code blinks text for an interval of time then clears the text.

I plan to use this code in a quiz web app. When the user gets an answer right, "CORRECT!" will blink on the screen for 10 seconds. When the user gets an answer wrong, "INCORRECT!" will blink on the screen for 10 seconds.

I'm looking for feedback on how I could simplify and improve it?

JS Bin: http://jsbin.com/ojetow/1/edit

It doesn't look very complex to me but the variable names could use some attention, as they're not very descriptive. I'd suggest:

textHidden instead of flag
timesBlinked instead of i
targetElement instead of divID (it's not an id anymore, but the element referred to by the id)

I would also toggle the element visibility instead of removing and adding the text for several reasons, one of them being that removing text can also change the element's height and/or width, which can cause problems with the layout.

if( textHidden ) {
    targetElement.style.visibility = 'visible';
} else {
    targetElement.style.visibility = 'hidden';
}

textHidden = !textHidden;  // flip the flag


Adding two sides of an array The purpose of the program is to add 2 sides of an array starting from the bottom and top of both ends and to find the lowest difference at equal decrements of iteration. The program functions but it's slow.

int solution(vector<int> &A) {
    int n = A.size();
    int i, j, rsum, lsum, difference;
    int currentmindifference = 1000;

    for(i = 0; i < n; i++)
    {
        lsum = 0;
        rsum = 0;
        for(j = 0; j <= i; j++) lsum += A[j];
        for(j = i + 1; j < n; j++) rsum += A[j];
        difference = abs (lsum - rsum);
        if (difference < currentmindifference) currentmindifference = difference;
    }

    return currentmindifference;
}

Your code is \$O(n^2)\$. You can improve it to \$O(n)\$ by precomputing a sum array.

int solution(vector<int> &A) {
    int n = A.size();
    int i, j, rsum, lsum, difference;
    int currentmindifference = 1000;

    std::vector<int> pre(n);
    pre[0] = A[0];
    for (i = 1; i < n; i++)
        pre[i] = pre[i - 1] + A[i];

    for(i = 0; i < n; i++)
    {
        lsum = pre[i];
        rsum = pre[A.size() - 1] - lsum;
        difference = abs (lsum - rsum);
        if (difference < currentmindifference) currentmindifference = difference;
    }

    return currentmindifference;
}

Reasoning. By calculating a sum array, we save the need to calculate lsum on each iteration. Furthermore, we notice that we can find rsum by finding the total sum less lsum.

We can simplify this further by noting that lsum and rsum are now superfluous. We will also replace 1000 with std::numeric_limits<int>::max() to ensure we get a true minimum instead of capping it, and check if A is empty before using it.

#include <algorithm>
#include <cmath>
#include <limits>
#include <vector>

int solution(std::vector<int> &A) {
    if (A.empty())
        return -1; // or some other meaningful error value

    int n = A.size();
    int minDifference = std::numeric_limits<int>::max();

    std::vector<int> pre(n);
    pre[0] = A[0];
    for (int i = 1; i < n; i++)
        pre[i] = pre[i - 1] + A[i];

    for (int i = 0; i < n; i++)
        minDifference = std::min(minDifference, std::abs(2 * pre[i] - pre[n - 1]));

    return minDifference;
}

Further reading: Prefix sum array and difference array. Disclosure: I am an administrator on this site. Excuse the formatting.


Print all anagrams together in a sentence in Java I know a lot of memory is being used, but how do I make this more efficient in space(and time) .

   public class AnagramsTogether {

    public static String getAnagramsTogether(String sentence){

        if(sentence == null || sentence.trim().equals("")) return null;

        String[] words = sentence.split(" ");
        HashMap<String,List<String>> map =new HashMap<String,List<String>>();

        for(String word:words)
        {
            char[] wordArray = word.toCharArray();
            Arrays.sort(wordArray);

            List<String> tempList = map.get(new String(wordArray));
            if(tempList == null) tempList = new ArrayList<String>();
            tempList.add(word);
                            map.put(new String(wordArray),tempList);
        }

        StringBuilder result = new StringBuilder();
        for(Map.Entry<String, List<String>> entry : map.entrySet())
        {

            for(String item:entry.getValue()){
                result.append(item+ " ");
            }
        }

        return result.toString();
    }

    public static void main(String[] args) {
         System.out.println(getAnagramsTogether("cat dog act tac god"));
    }

}

Whitespace discipline

While your indentation is good, the placement of whitespace isn't very good yet. In Java, we tend to put whitespace after keywords and before braces. A beautifier with standard settings would make your code more readable already.

Use the most common type that still does the job.

For example, your variable map should be declared as Map, not HashMap.

Smarter Use of Map API

Instead of getting the list, checking whether it's null and if it isn't null, creating it, and then always putting it, you could use Map.computeIfAbsent(), like this:

map
    .computeIfAbsent(normalizedAnagram, s -> ArrayList::new)
    .add(word);

Split your logic

Your function getAnagramsTogether is doing many things. You could split it into multiple methods. For example, you could have a separate method normalizeAnagram() which takes a String and returns a new String with the characters sorted.

Bug in your call to Arrays.sort(char[] wordArray)

It should be noted that char is NOT a character. It is a UTF-16 fragment. For the first Unicode plane, i.e. code points with values that can be represented by 16 bits, char is a character. But for code points with higher values, a char contains only part of the character, and the way how you construct the normalized anagram is tearing characters apart.

It may not matter to you, as probably all relevant words in your use cases would have code points in the 16 bit plane.

Don't use + in StringBuilder.append()

You should be consistent, either use + or append().
But using + inside StringBuilder.append() doesn't make sense.
We use StringBuilder in order to have String-concatenation which is faster and uses less memory than using +. Using + in an argument to StringBuilder.append() is completely contrary to the intention of using StringBuilder in the first place.

Don't construct the same object twice.

You're calling new String(wordArray) twice for no good reason.
You could just create a single String normalizedAnagram and use that.

Consider a smarter way of String concatenation.

The following code concatenates your final Map into a String in a smarter way:

map
    .entrySet()
    .stream()
    .flatMap(entry -> entry.getValue().stream())
    .collect(Collectors.joining(" "))


Node.js Sequelize Promise I am working on sending a reset token via email to a potential user in Node.js, Sequelize, and Express.

The code works, but I still think it could be improved by handling the promises better.

exports.sendResetToken = function(req,res) {
  const username = req.headers.username;
  User.findOne({
    where: {username: username}
  }).then((user) => {
    if(user == null){
      res.sendStatus(404);
    } else {
      let token = user.generateResetToken();
      user.update({
        resetToken: token,
      }).then((user) => {
        email.sendResetToken(user.email,user.resetToken);
        res.sendStatus(200);
      }).catch((err) => {
        console.error(err);
        res.sendStatus(400);
      })
    }
    return null;
  }).catch((err) => {
    console.error(err);
    res.sendStatus(401);
  })
}

Reading your code I can make the following remarks:

I don't like the name User as a service as it looks like it specifies one User object. A better name would be Users or UsersApi or UserService.
Having a User service return null in case of the user not found should return a rejected promise instead of a fulfilled promise as it greatly simplifies the logic of your code. In that case you won't have to do this:

if(user == null){
  res.sendStatus(404);
}

You can wrap all the thenable code in functions so that your code will   become more readable. For example this:

.then((user) => {
    if(user == null){
      res.sendStatus(404);
    } else {
      let token = user.generateResetToken();
      user.update({
        resetToken: token,
      }).then((user) => {
        email.sendResetToken(user.email,user.resetToken);
        res.sendStatus(200);
      })

will become this:

.then(generateResetToken)
.then(sendResetToken)

You do not have to return null as the intent of the function is to send an email here and not return anything. This also makes it more confusing.
Handle all your error cases in the catch function such as this:

function handleErrors(error) {
   console.error(err);
   switch(error.reason) {
     case ERRORS.USER_NOT_FOUND:
        res.sendStatus(404);
        break;
     case ERRORS.USER_UPDATE_FAILURE:
        res.sendStatus(400);
        break;
     default:
        res.sendStatus(417); // Or more cases
   }
}

So lets recap:

exports.sendResetToken = function(req,res) {
   const username = req.headers.username;

   Users
     .findOne({
        where: {username: username}
     }).then(generateResetToken)
     .then(sendResetToken)
     .catch(handleErrors)

    function generateResetToken(user) {
      const token = user.generateResetToken();
      const userUpdatePromise = user.update({
        resetToken: token,
      });

      return userUpdatePromise;
    }

    function sendResetToken(user) {
      EmailService.sendResetToken(user.email,user.resetToken);
      res.sendStatus(200);
    }

    function handleErrors(error) {
      console.error(err);
      switch(error.reason) {
        case ERRORS.USER_NOT_FOUND:
          res.sendStatus(404);
        case ERRORS.USER_UPDATE_FAILURE:
          res.sendStatus(400);
        default:
          res.sendStatus(417) // Or more cases here
    }
  }
}


Finding the number of times a query string appears in a vector of strings c++ I wrote some code that iterates over some queries and counts the number of times each query appears in a vector of words. My first iteration iterated through all the words in the vector for each query which is wasteful. I improved it and come up with this.

unordered_map<string, int> matchingStrings(vector<string>& strings, vector<string>& queries) {
    sort(strings.begin(), strings.end());
    sort(queries.begin(), queries.end());
    unordered_map<string,int> counts;
    auto s = strings.begin();
    bool notfound;
    int count = 0;
    for (auto& q : queries) {
        count = 0;
        auto temp = s;
        notfound = false;
        while (q.compare(*s)) {
            if (s != strings.end()-1) {
                s++;
            }
            else {
                counts[q] = count;
                notfound = true;
                s = temp;
                break;
            }
        }
        if (notfound) continue;
        while (!q.compare(*s)) {
            count++;
            if (s != strings.end()-1) {
                s++;
            }
            else {
                break;
            }
        }
        counts[q] = count;
    }
    return counts;
}

It works on my test cases, so I think it has some place here on code review. I would like to know how I can tidy this up? also! In the case where the query is not found we go back to the string the non-present query started at. Is there a way to get around this?

By "tidy this up" I mean, avoiding code repetition and maybe some alternative ideas? Possibly some modern c++ tools even?

You start by sorting both the queries and the strings to search, but you don't seem to make a whole lot of use of the fact that afterwards they're sorted. Fortunately, parts of the standard library make it fairly easy to take better advantage of that as well.
It looks like you've preceded your code by something like using namespace std;. This is generally considered a poor idea, and should usually be avoided.
When you just want to know if one string equals another, it's often simpler and more readable to use something like if (a == b) than if (!a.compare(b)).
The single biggest thing I see here is that the standard library already provides most of the functionality you want, so using it can make the code quite a bit simpler.

I'd probably write the code something on this general order:

[edit: modified code to optimize for unique queries, as OP stated in comment.]

std::unordered_map<std::string, int> matchingStrings(std::vector<std::string> &strings,
                                                     std::vector<std::string> &queries)
{
    std::sort(strings.begin(), strings.end());
    std::sort(queries.begin(), queries.end());

    std::unordered_map<std::string, int> counts;

    auto start = strings.begin();

    for (auto const &q : queries) {
        auto p = std::equal_range(start, strings.end(), q);
        counts[q] = p.second - p.first;
        start = p.second;
    }
    return counts;
}

std::equal_range looks for a range (in a sorted sequence) of objects that are equal to one that's specified. It can use a binary search to find the beginning and end of the range, so if the number of strings being searched is large, it can potentially save quite a bit of time. It returns an std::pair--the first item is an iterator to the beginning of the range, and the second an iterator to the beginning of the range. So, we use that to find all the strings that match a given query, and count them by subtracting the two.


Find the next number with all different digits, and no 0 or 2 Task
Input - integer number (year)
Print one number - the next year, in which all digits are pairwise different, and there are no digits 2 and 0. If there will never be such a year, print -1.
My solution (correct)
Could you please recommend time / space complexity optimizations?
vector<char> toVec(int n) {
    vector<char> res;
    while (n > 0) {
        res.push_back((n % 10) + '0');
        n /= 10;
    }
    return res;
}

int main(){
    int year;
    cin >> year;
    for (int i = year + 1; i < pow(10, 8); ++i) {
        vector<char> v = toVec(i);
        if (find(begin(v), end(v), '2') == v.end() and
            find(begin(v), end(v), '0') == v.end()) {
            set<char> temp(v.begin(), v.end());
            if (v.size() == temp.size()) {
                cout << i;
                return 0;
            }
        }
    }
    cout << -1;

}

You seem to be missing some headers (<vector>, <iostream>, <cmath> and <set>), and also to be assuming the standard names are in the global namespace, rather than std.
After attempting to read from std::cin, it's important to ensure the read was successful before attempting to use the result.  (E.g. if (!(std::cin >> year)) { std::cerr << "Input failure\n"; return EXIT_FAILURE; }).
A linear search will be inefficient, especially as we may have to skip long runs containing the banned digits 0 and 2.  It would be better to construct the result using the available digits than to consider and test every possible number.  The search is flawed anyway, since std::pow(10, 8) could be larger than the maximum int, leading to overflow (which is Undefined Behaviour).