Combining contents of files by reading them in a human ordering This code review is inspired by the this StackOverflow question and written in Java 8.

The code is supposed to:

Order all files, with syntax jobXXX.script, where XXX is a number, by the number.
Write the contents of the files in that ordering to one file.

I am asking for a general code review and there are two things that are bugging me:

I need to wrap Exceptions inside lambda expressions.
The file that is being written to, has an imperfect unnecessary annoying OCD-triggering newline at the end.

package testproject4;

import java.io.BufferedWriter;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Comparator;
import java.util.stream.Stream;

/**
 *
 * @author Beheerder
 */
public class TestProject4 {
    private void init() throws IOException {
        Path directory = Paths.get("C:\\Users\\Frank\\Downloads\\testjob");
        try (BufferedWriter writer = Files.newBufferedWriter(directory.resolve("masterjob.script"))) {
            Files.list(directory)
                    .filter(path -> Files.isRegularFile(path))
                    .filter(path -> path.getFileName().toString().matches("job\\d+.script"))
                    .sorted(Comparator.comparingInt(this::pathToInt))
                    .flatMap(this::wrappedLines)
                    .forEach(string -> wrappedWrite(writer, string));
        }
    }

    private int pathToInt(final Path path) {
        return Integer.parseInt(path.getFileName()
                .toString()
                .replaceAll("job(\\d+).script", "$1")
        );
    }

    private Stream<String> wrappedLines(final Path path) {
        try {
            return Files.lines(path);
        } catch (IOException ex) {
            //swallow
            return null;
        }
    }

    private void wrappedWrite(final BufferedWriter writer, final String string) {
        try {
            writer.write(string);
            writer.newLine();
        } catch (IOException ex) {
            //swallow
        }
    }

    public static void main(String[] args) throws IOException {
        new TestProject4().init();
    }
}

NewLine

The newline at the end is correct... at least according to your code.

Files.lines(...) ignores the last line of input files if it has no characters. This is a standard system to have for most operating systems (the last character in a text file is a newline).

Your code is perpetuating this process, and it always adds a newline after each line printed.

Thus, your file always ends up with a newline at the end. This is a good thing.

Exceptions

Java Lambdas cannot contain expressions that throw caught exceptions. This is a weakness. The weakness is so sever that as part of Java8, there is the new Exception UncheckedIOException so that IO-based operations can throw an unchecked exception.

The easiest way to use it is to change your catch blocks to:

private Stream<String> wrappedLines(final Path path) {
    try {
        return Files.lines(path);
    } catch (IOException ioe) {
        throw new UncheckedIOException(ioe);
    }
}

The alternative is to wrap the exception inside the lambda with:

    try (BufferedWriter writer = Files.newBufferedWriter(directory.resolve("masterjob.script"))) {
        Files.list(directory)
                .filter(path -> Files.isRegularFile(path))
                .filter(path -> path.getFileName().toString().matches("job\\d+.script"))
                .sorted(Comparator.comparingInt(this::pathToInt))
                .flatMap(try { this::wrappedLines } catch(IOException ioe) {throw new UncheckedIOException(ioe)})
                .forEach(string -> try {wrappedWrite(writer, string)} catch(IOException ioe) {throw new UncheckedIOException(ioe)});
    }

General

The Java8 side of this otherwise looks OK, and works fine for me too.


Haskell function to format a floating-point number with some precision I have not yet developed a satisfying coding style in Haskell. In the code snippet below, there is a lot wrong to my taste:

Helper functions which should be local to showEngFloat are defined outside.
let ... in indentation madness
More than 80 columns

Can you show me how this same code can be written in a less indentation and pollution prone way?

I am aware that the name I chose for my function is not well chosen. (show... functions should return a ShowS type, right?)

import Numeric

normalize :: Int -> (Int,Int)
normalize dp =
    loop dp 0
    where
        loop dp e
            | dp > 3 = loop (dp - 3) (e+3)
            | dp < 1 = loop (dp + 3) (e-3)
            | otherwise = (dp,e)

toString :: [Int] -> [Char]
toString ia = concat (fmap show ia)

padZeroes nz v  = v ++ take nz (repeat '0')

showEngFloat :: RealFloat a => Int -> a -> [Char]
showEngFloat precision x
    | x >= 0.0 = calc x
    | otherwise = "-" ++ calc (-x)
    where
        calc n =
            let (digits,dotPos0) = floatToDigits 10 n in
                let (ndigits,(dotPos1,e)) = (length digits,normalize dotPos0) in
                    let a
                            | ndigits >= dotPos1 = toString (take dotPos1 digits)
                            | otherwise = padZeroes (dotPos1 - ndigits) (toString (take ndigits digits))
                            in
                            let b = padZeroes (precision - max dotPos1 ndigits) (toString (drop dotPos1 digits))
                                in
                                    let c = toString [e] in
                                        a ++ "." ++ b ++ "E" ++ c

Here's another way to write it (without changing too much). The functions toString and padZereos do sensible things on their own, so I kept them in the global namespace. Everything else is local. Having one big where-clause where you define everything local to a function is something you'll see often. Note that due to lazy evaluation you can define a_trimmed and a_padded, even if only one is used, so the other is never calculated. That further cleans up the code.

In a next step, I'd probably get rid of calc entirely, and fold sign handling into its body. Also, normalize looks like it should be calculated with divMod or quotRem.

showEngFloat :: RealFloat a => Int -> a -> [Char]
showEngFloat precision x = result where
  result = if x >= 0 then calc x else '-' : calc (-x)
  normalize :: Int -> (Int,Int)
  normalize dp = loop dp 0 where
    loop dp e
      | dp > 3 = loop (dp - 3) (e+3)
      | dp < 1 = loop (dp + 3) (e-3)
      | otherwise = (dp,e)
  calc n = a ++ "." ++ b ++ "E" ++ c where
    (digits,dotPos0) = floatToDigits 10 n
    ndigits = length digits
    (dotPos1,e) = normalize dotPos0
    a_trimmed = toString (take dotPos1 digits)
    a_padded  = padZeroes (dotPos1 - ndigits) (toString (take ndigits digits))
    a = if ndigits >= dotPos1 then a_trimmed else a_padded
    b = padZeroes (precision - max dotPos1 ndigits) (toString (drop dotPos1 digits))
    c = toString [e]


Simple registration form with Backbone I am trying to create simple registration form with Backbone.  I need to validate fields on blur, and whole form on submit.

HTML:

<form>

<input name="name" type="text" placeholder="Name"/>

<input name="surname" type="text" placeholder="Surname"/>

<input name="email" type="text" placeholder="Your mail"/>

<input name="password" type="password" placeholder="Password"/>

<input name="pagename" type="text" placeholder="Page name"/>

        <input type="submit" value="Go"></input>
</form>

Model:

app.__definitions.models.Signup = Backbone.Model.extend({
    url: '/app/signup',

    defaults: {
            name: '',
            surname: '',
            email: '',
            password: '',
            pagename: ''
    },

    validateOne: validator.validateOne,

    validate: function(fields) {
            var result = validator.validateAll(fields);
            if (result !== true) return result;
    }
 });

View:

app.__definitions.views.Signup = Backbone.View.extend({
    el: $('form'),

    events: {
        'submit': 'onFormSubmit',
        'change input[type!="submit"]': 'onInputChange',
        'blur input[type!="submit"]': 'onInputChange',
        'focus input': function(e) {
                this.resetInputErrors(e.target);
        }
    },

    templates: {
            'error': _.template('<%=error%>')
    },

    initialize: function() {
            this.model = new app.__definitions.models.Signup();

            this.model.on('invalid', this.onModelInvalid, this);
    },

    getInput: function(name) {
            return this.$el.find('[name="' + name + '"]');
    },

    onModelInvalid: function(model, errors) {
            var _this = this;
            _.each(errors, function(error, name) {
                    _this.showInputErrors(_this.getInput(name), error);
            })
    },

    onFormSubmit: function(e) {
            e.preventDefault();
            var model = this.model;

            this.$el.find('input[name]').each(function() {
                    model.set(this.name, this.value);
            })
            this.model.save();
    },

    onInputChange: function(e) {
            this.model.set(e.target.name, e.target.value);
            var result = this.model.validateOne(e.target.name, e.target.value);
            if (result !== true) this.showInputErrors(e.target, result)
    },

    resetInputErrors: function(target) {
            var $target = $(target);
            $target.removeClass('incorrect')
            $target.parent().find('.error').remove();
    },

    showInputErrors: function(target, errors) {
            var $target = $(target);
            var errorsHTML = '';

            this.resetInputErrors(target);

            for (var i = 0; i < errors.length; i++) {
                    errorsHTML += this.templates.error({error: se.errors.get(errors[i])});
            }

            $target.addClass('incorrect');
            $target.parent().append(errorsHTML);
    }
    });

Init:

 app.views.Signup = new app.__definitions.views.Signup();
 app.routers.Router = new app.__definitions.routers.Main({hashChange: true});

Questions:

Do I understand right how to make Backbone application?
Where is the mistakes? What can I do better or clearer?
Is there a better way to connect model with inputs on my form, or my getInput is ok?
In my case I don't need to use the collection? Am I right?

Generally it looks like your know what your doing, maybe you could be more efficient by removing the blur event since you wouldn't need to save/validate if the input is already populated or left empty, this will also save you calling onInputChange twice when an input is changed.

Backbone will automatically create this.$el from the el property so you can just do.

el: 'form' and then this.$el would be created for you.

Finally i would use listenTo rather than on for example.

this.model.on('invalid', this.onModelInvalid, this);

would be done like this using listenTo.

this.listenTo(this.model, 'invalid', this.onModelInvalid);

This allows you to keep track of events on the object and then remove them all at once later on.


Testing numbers using 'switch' My task is:

Write a program that reads integers until 0 is entered. After input terminates, the
program should report the total number of even integers (excluding the 0)
entered, the average value of the even integers, the total number of odd integers
entered, and the average value of the odd integers.

But I had to use switch for this...

My code is:

#include <stdio.h>

int main(void)

{
    int oddIndex = 0, evenIndex = 0, evenTotal = 0, evenSum, oddTotal = 0, oddSum;
    int userInput;
    int oddArray[oddIndex], evenArray[evenIndex];

    printf("please enter some numbers: \n");
    while (((scanf("%d", &userInput)) == 1) && (userInput != 0))
    {

        switch (userInput%2)
        {
            case 0:
                evenArray[evenIndex] = userInput;
                evenIndex++;
                evenTotal++;
                evenSum += userInput;
                break;

            default:
                oddArray[oddIndex] = userInput;
                oddIndex++;
                oddTotal++;
                oddSum += userInput;
                break;
        };
    }
    if (evenTotal > 0)
    printf("even total: %d, even average: %d\n", evenTotal, evenSum/evenTotal);
    else
        printf("there is no even numbers!\n");
    if (oddTotal > 0)
    printf("odd total: %d, odd average: %d", oddTotal, oddSum/oddTotal);
    else
        printf("there is no odd numbers!");
}

Is this code ok?

I haven't been a C guy for years, but a few observations:

You have int oddArray[oddIndex], evenArray[evenIndex]; declared, but the only time you use it is to add the entered number, and even that will fail because you have basically declared them as size[0], so oddArray[1] will throw an exception.

I think [even|odd]Index and [even|odd]Total can be combined into one variable.

I'm not sure what the standard is for C, but I really don't like the multiple declarations on one line.  There should be a single declaration per line.  Its less confusing that way.

You could also put some error handling around the input.  What happens if somebody enters "A"?  Your program would crash.  I also think your instruction should be a little more clear.  What does "please enter some numbers." really mean?

Minor thing, you forgot to indent the line after the if's at the bottom.

I would also move the reporting to a different function and reduce duplication.

Your code could turn out like this:

#include <stdio.h>

void DisplayResults(char numberType[], int total, int sum);

int main(void)
{
    int oddCount= 0;
    int evenCount= 0;
    int evenSum = 0;
    int oddSum = 0;
    int userInput;

    printf("Please enter a number followed by <enter>.  Enter \"0\" to exit: \n");

    while (((scanf("%d", &userInput)) == 1) && (userInput != 0))
    {

        switch (userInput%2)
        {
            case 0:
                evenCount++;
                evenSum += userInput;
                break;

            default:
                oddCount++;
                oddSum += userInput;
                break;
        };
    }

    DisplayResults("Even", evenCount, evenSum);
    DisplayResults("Odd", oddCount, oddSum);
    return 0;
}

void DisplayResults(char numberType[], int count, int sum)
{
    if (total > 0)
    {
        printf("%s total: %d, %s average: %d\n", numberType, count, numberType, sum/count);
    }
    else
    {
        printf("There are no %s numbers!\n", numberType);
    }
}

Please excuse me if I have the syntax wrong, I haven't used C in about 20 years.


Adding click handler to multiple IDs I'm kinda new at this jQuery stuff and I've gathered up this much to make my code functional, but I would really want to shorten it. I've already tried $('#Monday','#Tuesday'....) to group them, but with no success.

$('#Monday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});
$('#Tuesday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});
$('#Wednesday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});
$('#Thursday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});
$('#Friday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});
$('#Saturday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});
$('#Sunday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});

You should add a class to all elements, something like .day, and then use that class in your code:

$('.day').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});

Another way would be:

$('#Monday, #Tuesday, #Wendnesday').click(function() {
    if ($(this).hasClass("active")){
        $(this).switchClass( "active", "inactive", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', true);
    } else {
        $(this).switchClass( "inactive", "active", 200, "easeInOutQuad" );
        $(this).parent().find('.miniTime').prop('disabled', false);
    }
    $(this).find('input').prop('checked', function( foo, oldValue ) {return !oldValue});
});


Determining whether a provided credit card number is valid according to Luhn’s algorithm I have been working on credit problem for edx CS50 and got it working as intended. an issue I find with it is the way I have to reinitialise the cardNumber variable each time it enters a loop, and there are plenty of loops in my solution. Same goes for my bool value. Is there a better, cleaner way to go around this?

#include <stdio.h>
#include <cs50.h>

// Implement a program that determines whether a provided credit card number
// is valid according to Luhn’s algorithm.

int main(void)
{
    long long userInput;

    // Prompt for user input as long long.
    do
    {
        userInput = get_long_long("Enter credit card number to validate: ");

        // Ensure user inputs integer otherwise prompt "Retry: "
        if (userInput < 0)
        {
            printf("Retry: \n");
        }
    } while (userInput < 0);

    long long cardNumber = userInput;
    int checkSum = 0;
    int secDig, lastDig, multDig;

    // start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
    while (cardNumber > 0)
    {
        // sum digits that weren't multiplied.
        lastDig = (cardNumber % 10);
        // add the sum to the sum of the digits that weren't multipied by 2.
        checkSum += lastDig;

        secDig = (cardNumber / 10) % 10;
        cardNumber /= 100;

        // multiply every other digit
        multDig = secDig * 2;

        // add those products (separate /10, return modulus of 10) digits together.
        if (multDig > 10)
        {
            checkSum += (multDig % 10) + 1;
        }
        else if (multDig == 10)
        {
            checkSum += 1;
        }
        else
        {
            checkSum += multDig;
        }
    }

    // validate checksum
    // if sum of digits ends in 0 ergo % 10 = 0 then number is valid.
    bool valid = false;
    if (checkSum % 10 == 0)
    {
        valid = true;
    }

    int numOfDigits = 0;
    cardNumber = userInput;

    // validate number's length
    // track number's length
    while (cardNumber > 0)
    {
        cardNumber /= 10;
        numOfDigits++;
    }

    cardNumber = userInput;

    // validate company's identifier
    // if 15 digits, starts with 34 or 37 - AMEX

    if (valid == true && numOfDigits > 12)
    {
        if (numOfDigits == 15)
        {
            for (int i = 0; i < numOfDigits - 2; i++)
            {
                cardNumber /= 10;
            }
            if (cardNumber == 34 || cardNumber == 37)
            {
                printf("AMEX\n");
                valid = true;
            }
            else
            {
                valid = false;
            }
        }
        // if 16 digits, starts with 51-55 - MASTERCARD
        cardNumber = userInput;
        if (numOfDigits == 16)
        {
            for (int i = 0; i < numOfDigits - 2; i++)
            {
                cardNumber /= 10;
            }
            if (cardNumber >= 51 && cardNumber <= 55)
            {
                printf("MASTERCARD\n");
                valid = true;
            }
            else
            {
                valid = false;
            }
        }
        // if 13 OR 16 digits, starts with 4 - VISA
        cardNumber = userInput;
        if (numOfDigits == 13 || numOfDigits == 16)
        {
            for (int i = 0; i < numOfDigits - 1; i++)
            {
                cardNumber /= 10;
            }
            if (cardNumber == 4)
            {
                printf("VISA\n");
                valid = true;
            }
            else
            {
                valid = false;
            }
        }
        if (valid == false)
        {
            printf("INVALID\n");
        }
    }
    else
    {
        printf("INVALID\n");
    }
}

All comments are part of pseudocode for this solution. I left them there to satisfy style50. I would appreciate advice so I can write cleaner next time and make it into a habit. I am a beginner starting CS degree next year with only experience with doing C# courses with Mosh Hamedani.

First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:

long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum { invalid, amex, mastercard, visa } company;
company getCompany(long long cardnumber);

int main()
{
    long long cardnumber = readCardnumber();
    bool valid = validateCardnumber(cardnumber);
    if(!valid) {
        printf("INVALID\n");
        exit(0);
    }
    company comp = getCompany(long long cardnumber);
    switch(comp) {
        case visa: printf("VISA\n"); break;
        case mastercard: printf("MASTERCARD\n"); break;
        case amex: printf("AMEX\n"); break;
        default: printf("INVALID\n");
    }
}

Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).

One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)

There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.


Implement `flatten` for `Cons` Given the following definition of Cons:

data Cons a = Cons a (Cons a)
             | Empty
             deriving Show

I implemented a flatten function.

It's mean to work exactly like concat :: [[a]] -> [a], but for Cons.

flatten :: Cons (Cons a) -> Cons a
flatten Empty                = Empty
flatten (Cons (Empty) ys)    = flatten ys
flatten (Cons (Cons x xs) ys) = Cons x (flatten (Cons xs ys))

Test data:

test1 :: Cons (Cons Int)
test1 = Cons (Cons 5 Empty) Empty

test2 :: Cons (Cons Int)
test2 = Cons (Cons 5 (Cons 10 Empty)) Empty

test3 :: Cons (Cons Int)
test3 = Cons (Cons 5 (Cons 10 (Cons 20 Empty))) test2

Some tests:

ghci> flatten test1
Cons 5 Empty

ghci> flatten test2
Cons 5 (Cons 10 Empty)

ghci> flatten test3
Cons 5 (Cons 10 (Cons 20 (Cons 5 (Cons 10 Empty))))

Well, unless you wanted to add instances to allow for folding and so, or conversions to Data.List I'd say that's perfect as it is.


Creating an instance of an unit of work (NHibernate) I'm trying to figure out how to create a NHibernate UnitOfWork using .NET Core Dependency Injection. I was just hard-coding the connection string before moving it to appsettings.json configuration file. Is this form of creating a UOW in the service.scope valid?

It works, but I'm not sure if this is the intended way to do it.

Startup.cs

services.AddScoped<IUnitOfWork, UnitOfWorkV>( x=> { return new UnitOfWorkV(Configuration); });

UnitOfWorkV.cs

public UnitOfWorkV(IConfiguration configuration)
        {
            _sessionFactory = Fluently.Configure()
                .Database(MsSqlConfiguration.MsSql2008.ConnectionString(configuration.GetSection("ConnectionStrings:ConexionV").Value))
                .ExposeConfiguration(ConfigureNhibernateValidator)
                .Mappings(m =>
                {
                    m.FluentMappings.AddFromAssembly(Assembly.GetExecutingAssembly());
                    m.HbmMappings.AddFromAssemblyOf<JobQueue>();
                })
                .BuildSessionFactory();
            Session = _sessionFactory.OpenSession();
            Session.FlushMode = FlushMode.Auto;
        }

appsettings.json

{
  "ConnectionStrings": {
    "ConexionV": "Some connection string"
  }
}

In some camps it is not advised to tightly couple your code to framework specific dependencies like IConfiguration.

Based on the shown constructor it looks like the UnitOfWorkV really needs the connection string and not the configuration. That late bound constructor dependency will only be realized when the class is being resolved.

Refactor the  class to depend on what is actually needed.

public UnitOfWorkV(string connectionString) {
    _sessionFactory = Fluently.Configure()
        .Database(MsSqlConfiguration.MsSql2008.ConnectionString(connectionString)
        .ExposeConfiguration(ConfigureNhibernateValidator)
        .Mappings(m => {
            m.FluentMappings.AddFromAssembly(Assembly.GetExecutingAssembly());
            m.HbmMappings.AddFromAssemblyOf<JobQueue>();
        })
        .BuildSessionFactory();
    Session = _sessionFactory.OpenSession();
    Session.FlushMode = FlushMode.Auto;
}

And get the connection string at start up,

var connectionString = Configuration.GetConnectionString("ConexionV");

injecting it explicitly into the class.

if(connectionString == null) //Fail early
    throw new Exception("Missing connection string");

services.AddScoped<IUnitOfWork, UnitOfWorkV>(_ => new UnitOfWorkV(connectionString));

That way, any validation can be done immediately at startup instead of deferring to when resolving the actually class


Method naming and violating single responsibility I need your opinion. Which way do you think is right?

The definition of message in this code bothers me because it is a single method that has two separate behaviors.  Depending on how many arguments are passed the method will do different things--that stinks.

On the other hand, calling message in the block just feels right. It is subtle. But the difference of assigning the message with or without the = seems more significant than it should.  I feel guilty for really wanting to code this way.

Here's a code example:

class Queue
    attr_accessor :id

    def initialize(id, &block)
      if block_given?
        instance_eval(&block)
      end
    end

    def message=(value)
        @message = value
    end

    # if no value is given then it reports the value
    def message(value=nil)
        value.nil? ? @message : @message = value
    end
end

Queue.new :test_queue do
    message "This is a test"
end

Alternatively, I could use another method for it.

set_message "This is a test"

This also doesn't have the same effect.

Opinions? Other options?

It looks like you're building a small DSL, in which event message "Hello" is analogous to, say, Rails' belongs_to :user. The difference is that while belongs_to is only, in a sense, a setter (it can't be called without an argument, and doesn't return any useful value), you've built message as a getter and a setter.

I think this is about context. In your example:

Queue.new :test_queue do
  message "This is a test"
end

..the use of message feels "right." But in this example, it doesn't:

q = Queue.new :test_queue

q.message "This is a test"

..at least not as much. q.message = "..." makes more sense here.

You could just leave it as-is, i.e. allow assignment both ways, using whichever you prefer and letting others use whichever they prefer. The other option would be to make it work one way inside the constructor block and another way on an instance object. Here's an article that's relevant to this approach, with an apropos shout-out to Factory Girl, which itself contains a great DSL.

Hope that helps!


Scale the ingredients of a recipe up/down I wrote a React app to adjust the serving size of a recipe. For example, if the recipe serves 4 people and requires 4 eggs, scaling it down to serve only 1 person should only require 1 egg. I try to also keep the units singular/plural accordingly while converting units when they become too big or too small. These features are written in plain JS, but I'd be curious to hear if it would be more appropriate to do this in React and how.

Relevant code snippets:

const RECIPES = [{
  id: 'pasta-dough',
  name: 'Pasta Dough',
  serves: 4,
  ingredients: [
    {quantity: '4', unit: 'eggs'},
    {quantity: '4', unit: 'cups', item: 'flour'},
    // Note that the quantity below would normally be '1/4'.
    {quantity: '.25', unit: 'cup', item: 'olive oil'},
    {quantity: '1', unit: 'tablespoon', item: 'salt'}
  ]
}];

const Recipe = React.createClass({
  getInitialState: function() {
    return {
      serves: this.props.serves,
      scale: 1
    };
  },
  onChange: function(e) {
    const serves = Math.floor(Math.abs(e.target.value));
    this.setState({
      serves: serves,
      scale: serves / this.props.serves
    });
  },
  render: function() {
    return (

{this.props.name}

          Serves
          <input
            type="number" min="0"
            value={this.state.serves}
            /* TODO: Smarter default step. */
            step={this.props.serves / 4}
            onChange={this.onChange} />

        <Ingredients ingredients={this.props.ingredients} scale={this.state.scale} />

    );
  }
});

const Ingredients = React.createClass({
  render: function() {
    return (

        {this.props.ingredients.map((ingredient, i) =>
          <Ingredient key={i} scale={this.props.scale} {...ingredient} />
        )}

    );
  }
});

const Ingredient = React.createClass({
  render: function() {
    // Commenting out the code that depends on other code libraries.
    // Want to keep this code review as simple as possible.
    // const quantity = new Quantity(this.props.quantity).multiply(this.props.scale);
    const unit = this.props.unit;
    // const measurement = new Measurement(quantity, unit).convertUnits();
    const quantity = this.props.quantity * this.props.scale;
    const measurement = `${quantity} ${unit}`
    return(

{measurement.toString()} {this.props.item}

    );
  }
});

RECIPES.forEach((recipe, i) => {
  ReactDOM.render(
    <Recipe key={recipe.id} index={i} {...recipe}/>,
    document.getElementById(recipe.id));
});

document.querySelector('h2 input').focus();

Demo: https://rviscomi.github.io/scale-recipe-react/
Source: https://github.com/rviscomi/scale-recipe-react

I'm new to React and would really appreciate advice on writing more semantic React. Some specific areas I'm interested in:

use of state vs props
mounting
component organization
static methods

Things I'm not interested to get feedback on:

general JS style
ES6, unless it directly affects React
markup semantics

I have edited your little code snippet and wrote it in ES6, cause sooner or later you will need it. Most tutorials are written in ES6.
Mostly you will use a build tool like webpack in building react apps.

Example structure:

dist/

js/

bundle.js

css/

style.css

index.html

src/

libs/ (js libs)
components/
  (one folder for every component to have all files together e.g. css with the use of css modules)

Ingredient/

Ingredient.jsx
Ingredient.css

Ingredients/

Ingredients.jsx
Ingredients.css

App.jsx
style.css

webpack.config.js

Some useful links:

https://github.com/srn/react-webpack-boilerplate
https://github.com/gaearon/react-hot-boilerplate
http://survivejs.com/
https://webpack.github.io/
https://github.com/css-modules/webpack-demo

const RECIPES = [{
    id: 'pasta-dough',
    name: 'Pasta Dough',
    serves: 4,
    ingredients: [
        {quantity: '4', unit: 'eggs'},
        {quantity: '4', unit: 'cups', item: 'flour'},
        // Note that the quantity below would normally be '1/4'.
        {quantity: '.25', unit: 'cup', item: 'olive oil'},
        {quantity: '1', unit: 'tablespoon', item: 'salt'}
    ]
}];

class Ingredient extends React.Component {
    render() {
        let quantity = this.props.quantity * this.props.scale;

        return (

              {quantity} {this.props.unit} {this.props.item}

        );
    }
}

class Ingredients extends React.Component {
    render() {
        return (

                {this.props.ingredients.map((ingredient, i) =>
                    <Ingredient key={i} scale={this.props.scale} {...ingredient} />
                )}

        );
    }
}

class Recipe extends React.Component {

    constructor(props) {
        super(props);

        this.state = {
            serves: this.props.serves,
            scale: 1
        }

        this.onChange = this.onChange.bind(this);
    }

    onChange(e) {
        const serves = Math.floor(Math.abs(e.target.value));

        this.setState({
            serves: serves,
            scale: serves / this.props.serves
        });
    }

    render() {
        return (

{this.props.name}

                    Serves
                    <input
                        type="number" min="0"
                        value={this.state.serves}
                        /* TODO: Smarter default step. */
                        step={this.props.serves % 4}
                        onChange={this.onChange}/>

                <Ingredients ingredients={this.props.ingredients} scale={this.state.scale}/>

        );
    }
}

class App extends React.Component {
    render() {
        return

            {RECIPES.map((recipe, i) => {
                return <Recipe {...recipe} key={i}/>;
            })}

;
    }
}

ReactDOM.render(
    <App />,
    document.getElementById('recipes'));
<!DOCTYPE html>

  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title>JS Bin</title>


Fizzbuzz some Fibonacci numbers I was teaching some new people in office a bit of COBOL, so I threw together this exercise where they had to write a program that would generate the first 10 numbers of a Fibonacci suite and then test them against the rules of fizzbuzz.

I was hoping to get an honest review of this code to see how I could improve.

   IDENTIFICATION DIVISION.
   PROGRAM-ID. PROG6.
   AUTHOR.        SAGGING RUFUS.
   DATE-WRITTEN.  JUNE 5 2017.
   DATE-COMPILED.
   TITLE "FIZZBUZZONACCI"

   ENVIRONMENT DIVISION.
   CONFIGURATION SECTION.
   SOURCE-COMPUTER.   AMDAHL-580.
   OBJECT-COMPUTER.   AMDAHL-580.

   DATA DIVISION.
   WORKING-STORAGE SECTION.
   01  WS-STORAGE.
       05 WS-SUB PIC 9(8) COMP.
       05 WS-LIMIT PIC 9(4) VALUE 10.
       05 WS-F.
          10 WS-FX PIC 9(8) COMP.
          10 WS-F1 PIC 9(8) COMP VALUE 0.
          10 WS-F2 PIC 9(8) COMP VALUE 1.
       05 WS-F-ARRAY      PIC 9(8) COMP OCCURS 10 TIMES.
       05 WS-F-SUB        PIC 9(8) COMP VALUE 0.

   01  COUNTERS.
       05 CURRENT-NUM PIC 9(3) COMP.

   01  REMAINDERS.
       05 MOD-THREE PIC 99 COMP.
          88 FIZZ VALUE 0.
       05 MOD-FIVE PIC 99 COMP.
          88 BUZZ VALUE 0.

   PROCEDURE DIVISION.

   0000-MAINLINE.
       PERFORM VARYING WS-SUB
                  FROM 1 BY 1
                 UNTIL WS-SUB > WS-LIMIT
          COMPUTE
             WS-FX = WS-F1 + WS-F2
          END-COMPUTE

          ADD 1 TO WS-F-SUB

          MOVE WS-F2 TO WS-F1
          MOVE WS-FX TO WS-F2
          MOVE WS-FX TO WS-F-ARRAY(WS-F-SUB)
       END-PERFORM

       PERFORM VARYING WS-SUB
                  FROM 1 BY 1
                 UNTIL WS-SUB > WS-LIMIT
          MOVE WS-F-ARRAY(WS-SUB) TO CURRENT-NUM

          COMPUTE
             MOD-THREE = FUNCTION MOD(CURRENT-NUM 3)
          END-COMPUTE

          COMPUTE
             MOD-FIVE = FUNCTION MOD (CURRENT-NUM 5)
          END-COMPUTE

          EVALUATE TRUE
             WHEN FIZZ AND BUZZ
                DISPLAY "FIZZBUZZ"
             WHEN FIZZ
                DISPLAY "FIZZ"
             WHEN BUZZ
                DISPLAY "BUZZ"
             WHEN OTHER
                DISPLAY CURRENT-NUM
          END-EVALUATE
       END-PERFORM

       STOP RUN.

My suggestions focus on style issues and pointless code and assume you're using a modern compiler.

In the identification division and environment division:

Delete the comment paragraphs. They were deleted from the standard 15 years ago and have been obsoleted by version control systems.
Rather than use the TITLE statement, name your program FIZZBUZZONACCI. All contemporary compilers support program-names longer than six characters.
Delete the non-functional SOURCE- and OBJECT-COMPUTER paragraphs.

In the data division:

WS-LIMIT is a constant, so it's good idea to declare it as such. Change its declaration to

01  WS-LIMIT CONSTANT AS 10.

or, if your compiler doesn't support that, try the common non-standard extension:

78  WS-LIMIT VALUE 10.

Some of the variables begin with WS- and some don't. I suggest getting rid of the WS- prefix: there's nothing to gain here from indicating a variable is in working-storage.
Similarly, there's nothing to gain from putting variables in the WS-STORAGE, COUNTERS and REMAINDERS group (except for the OCCURS items).

In the procedure division:

Get rid of the redundant END-COMPUTE's. They're only needed for the SIZE ERROR handlers and clutter the code otherwise.

Quuxplusone raises a good point that FIZZ and BUZZ aren't strictly necessary. You could rewrite the EVALUATE like so, and not hide the meaning of FIZZ and BUZZ in the data division:

EVALUATE mod-three ALSO mod-five
    WHEN 0 ALSO 0
        DISPLAY "FizzBuzz"
    WHEN 0 ALSO ANY
        DISPLAY "Fizz"
    WHEN ANY ALSO 0
        DISPLAY "Buzz"
    WHEN OTHER
        DISPLAY current-num
END-EVALUATE


JDBC query with limit and offset I have this method that lists database objects:

public List<Person> getPersons(int offset, int limit) {
    return jdbcTemplate.query("SELECT * FROM `Person`" + (limit > 0 ? " LIMIT " + (offset > 0 ? offset + "," : "") + limit : "")+" ORDER BY `created` DESC");
}

It looks confusing, so how do I clean up the method?

If you want to clean up a bit the method, then I think that you should try another, more object oriented way of writing the method (currently the method is written procedurally).

Implementation 1

First lets create few interfaces, which will represent the Limit and the Offset:

public interface Offset {

    public String getOffset();

}

public interface Limit {

    public String getLimit();

}

Next we will need an implementations of this interfaces (I supposed that you are using MySQL as database, since its shorter syntax for limitation and offset is the same):

public MySQLOffset implements Offset {

    private final int offset;

    public MySQLOffset(int offset) {
        this.offset = offset;
    }

    @Override
    public String getOffset() {
        if (offset < 0) {
            throw IllegalArgumentException("Offset couldn't be a negative number.")
        }

        return offset + ",";
    }
}

public MySQLLimit implements Limit {

    private final MySQLOffset offset;
    private final int limit;

    public MySQLLimit(MySQLOffset offset, int limit) {
        this.offset = offset;
        this.limit = limit;
    }

    @Override
    public String getLimit() {
        if (limit <= 0) {
            throw IllegalArgumentException("Limit should be a strictly positive number.");
        }

        return String.format("LIMIT %s %s", offset.getOffset(), limit);
    }
}

Finally we will need one more interface, which will serve us as a glue between this two interfaces (I called it Pagination, since most of the times, offset and limit is used in combination for pagination of some API):

public interface Pagination {

    public String getPagination();

}

public MySQLPagination {

    private final MySQLLimit limit;
    private final MySQLOffset offset;

    public MySQLPagination(MySQLLimit limit) {
        this.limit = limit;
        this.offset = offset;
    }

    public String getPagination() {
        return limit.getLimit();
    }
}

Now, suppose that we have some data access object called PersonDAO, which should retrieve all the persons in a given range (bounded by limit and offset), then the code would look like that (I`ve change the name of getPersons method to retrievePersons, since we actually retrieves the person entries from the database, rather than getting them from some POJO):

.. code ommited ..

    PersonDAO.retrievePersons(new MySQLPagination(new MySQLLimit(new MySQLOffset(offset), limit)));

.. code ommited ..

public List<Person> retrievePersons(Pagination pagination) {
    return jdbcTemplate.query("SELECT * FROM `Person` " + pagination.getPagination());
}

Implementation 2

Suppose that you want to you use the long syntax of limitation and offset. Then the code will look like this:

public interface Offset {

    public String getOffset();

}

public interface Limit {

    public String getLimit();

}

public MySQLOffset implements Offset {

    private final int offset;

    public MySQLOffset(int offset) {
        this.offset = offset;
    }

    @Override
    public String getOffset() {
        if (offset < 0) {
            throw IllegalArgumentException("Offset couldn't be a negative number.")
        }

        return " OFFSET " + offset;
    }
}

public MySQLLimit implements Limit {

    private final int limit;

    public MySQLLimit(int limit) {
        this.limit = limit;
    }

    @Override
    public String getLimit() {
        if (limit <= 0) {
            throw IllegalArgumentException("Limit should be a strictly positive number.");
        }

        return "LIMIT " + limit;
    }
}

public interface Pagination {

    public String getPagination();

}

public MySQLPagination {

    private final MySQLLimit limit;
    private final MySQLOffset offset;

    public MySQLPagination(MySQLLimit limit, MySQLOffset offset) {
        this.limit = limit;
        this.offset = offset;
    }

    public String getPagination() {
        return limit.getLimit() + offset.getOffset();
    }
}

.. code ommited ..

    PersonDAO.retrievePersons(new MySQLPagination(new MySQLLimit(limit), new MySQLOffset(offset)));

.. code ommited ..

public List<Person> retrievePersons(Pagination pagination) {
    return jdbcTemplate.query("SELECT * FROM `Person` " + pagination.getPagination());
}

If you want you could also move the creation of the Pagination object into some factory method like this one:

public Pagination createPagination(int limit, int offset) {
   return new MySQLPagination(new MySQLLimit(limit), new MySQLOffset(offset))
}

Which also comes with some benefits, like more testable code etc.

Benefits of this implementation:

Implemented in OOP way
The validation of the input parameters is moved into the relevant classes, so the single responsibility principle is respected.
Easier to change the implementation
Easier for testing (suppose that you use one database for production and one for testing and they have different syntaxis for the limitation and the offset, then you should only mock the createPagination method and everything will work)
There is no if/else (or ternary operator) clauses in the getPersons method
Also now the getPersons method doesn't know how the limit and the offset of the query is constructed, which is the right way, since it`s work is to retrieve the entries, not to know how to construct them (which means little responsibilities for getPersons method)


Styles: "object initializer" vs "constructor" object creation Question: What are peoples preferences between the two methods of creating an object (in C# in particular) and setting other fields (that may not be given values during construction)? Are either of them considered best practice? or is it down to the programmer and any styling conventions in place at the company?

My preference is absolutely for the object initializer method. It looks much cleaner and more concise to me.

Object initializer

Type T = new Type(Param1, Param2)
{
    Field1 = Value1,
    Field2 = Value2,
    Field3 = ValueFromFunction()
};

Constructor

Type T = new Type(Param1, Param2);
T.Field1 = Value1;
T.Field2 = Value2;
T.Field3 = ValueFromFunction();

These are both techniques born of different goals.

The constructor style is something you will design your classes for when you want them to be faux-immutable, that is, you do not want them changed after construction so you make the setters on your properties protected or private.

The initializer style comes from you ceding control of the object's data members to the object's consumer, which you will do when you do not care about the order or timeliness of them being filled in.

One of the largest decision making factors here is this: Is the object safely usable if the properties are null?

If not, the constructor format allows you to ensure they will never be null by requiring them be set at construction by parameter and disallowing them to be set by the consumer.

Example of this would be a connection string object, without a database catalog it cannot be safely used as anything that attempted to use it would try to use the database catalog and may throw an exception if it is null.

The other reason for the constructor format vs. initialization is dependencies, this falls into the same question as null safety but can be thought about a little differently: If an object is useless without a particular dependency object or piece of data, then there is little reason to allow its construction without that parameter.

Example of this would be a file stream, if you do not give it a path or some way of knowing about a file, it becomes a useless object.

Edit:
All that said, as far as using initializer syntax when available, it's really just sugar and I think everyone can agree that you should use it if you know a number of values at construction time.


Watermarking an image I have a small bit of code and I wonder if I can make it more readable. I would appreciate it if some people could take a look at it and give me some feedback. I especially wonder if there is a beter way of doing this:
cv::Mat image = loadImage(path.string());
if (image.data == NULL) { continue; }

This is the bigger code:
cv::Mat Watermarker::loadImage(const std::string& path)
{
    if (!validFile(path))
    {
        if (!std::filesystem::is_directory(path))
        {
            std::cout << path << " is invalid\n";
        }

        return cv::Mat{};
    }

    cv::Mat image = cv::imread(path, cv::IMREAD_UNCHANGED);
    if (image.data == NULL)
    {
        std::cout << "Not enough permissions to read file " << path << "\n";
        return cv::Mat{};
    }

    return image;
}

void Watermarker::mark(const std::string& targetDirectory, const std::string& watermarkPath)
{
    cv::Mat watermarkImage = loadImage(watermarkPath);
    if (watermarkImage.data == NULL) { return; }

    // Loop through all the files in the target directory
    for (const std::filesystem::path& path : std::filesystem::directory_iterator(targetDirectory))
    {
        cv::Mat image = loadImage(path.string());
        if (image.data == NULL) { continue; }

        cv::resize(watermarkImage, watermarkImage, image.size());

        // Give the images 4 channels
        cv::cvtColor(image, image, cv::COLOR_RGB2RGBA);
        cv::cvtColor(watermarkImage, watermarkImage, cv::COLOR_RGB2RGBA);

        // Add the watermark to the original image
        cv::addWeighted(watermarkImage, 0.3, image, 1, 0.0, image);

        saveImage(targetDirectory + "/watermarked_images/" + path.filename().string(), image);
    }
}

Don't ignore errors
You are just skipping any file that cannot be read correctly. But probably, if the user tried to process the images in a given target directory, it wants all of them to be watermarked, and if something cannot be watermarked, the user probably wants to hear about it. So you should not just continue after loading an image failed, you should stop and report an error. There are several ways to do this, for example:
Consider throwing exceptions
I would recommend throwing exceptions here. I assume that not being able to open a file or parsing it correctly is an exceptional event for your program. The advantage of throwing an exception is that the caller of a function that throws doesn't have to handle the exception, it can just let it propagate up the stack until something does handle it, or if nothing handles the exception, the program will abort with an error. The latter can even be desirable. Here is an example of how to throw exceptions:
#include <stdexcept>

cv::Mat Watermarker::loadImage(const std::string& path)
{
    cv::Mat image = cv::imread(path, cv::IMREAD_UNCHANGED);

    if (!image.data)
        throw std::runtime_error("Failed to read image");

    return image;
}

I have removed the check for path being valid as well, since if it isn't valid, trying to call cv::imread() on it will fail as well.
Inside Watermarker::mark(), you can now assume that when loadImage() returns, image.data is always non-NULL, so you don't need a check there. If you really want to continue parsing images, then you can catch the exception, and perhaps print a warning so the user does get to know what was wrong:
try {
    cv::Mat image = loadImage(path.string());
    cv.resize(...);
    ...
} catch (std::runtime_error &e) {
    std::cerr << "Could not load " << path << ": " << e.what() << "\n";
}

Write error messages to std::cerr
Don't write warnings, errors and debug messages to std::cout, but use std::cerr. This is especially important if the program is printing normal output to std::cout, so that if standard output is redirect to a file for example, it will not contain error messages, and the error messages will still be printed to the console.
Avoid converting std::filesystem::paths to std::strings too early
Use std::filesystem::path as the type to pass paths in consistently. Only convert them to/from std::strings when really necessary. You can also make use of implicit conversions between std::string and std::filesystem::path. So for example:
cv::Mat Watermarker::loadImage(const std::filesystem::path& path)
{
    cv::Mat image = cv::imread(path, cv::IMREAD_UNCHANGED);
    ...
}

And:
void Watermarker::mark(const std::fileystem::path& targetDirectory, const std::filesystem::path& watermarkPath)
{
    auto watermarkImage = loadImage(watermarkPath);

    for (auth& path : std::filesystem::directory_iterator(targetDirectory))
    {
        auto image = loadImage(path);
        cv::resize(watermarkImage, watermarkImage, image.size());
        ...
        saveImage(targetDirectory / "watermarked_images" / path.filename(), image);
    }
}


TypeScript Generics: Create a generic map Exercise from a TypeScript course:

Create a map using TypeScript Generics. The key is always a string.
The value can be a string or a number. It has to work with both types.
It's got to have the following methods:

Creating a new key-value pair : setItem(key: string, item: T)

Retrieving values : getItem(key: string)

Remove all items : clear()

Display all key-value items : printMap() "

Here my uncompiled TypeScript code:

// -- THE ACTUAL IMPLEMENTATION --------------------------
class GenericMap < T extends number | string > {
  private items: object[]

  constructor() {
    this.items = [];
  }

  setItem(key: string, item: T): void {
    let tmp = {};
    tmp[key] = item;

    this.items.push(tmp);
  }

  getItem(key: string): T {
    let ret = null;

    for (let i = 0; i < this.items.length; i++) {
      let curr = this.items[i];

      if (curr[key]) {
        return curr[key];
      }
    }

    return null;
  }

  clear() {
    this.items.length = 0;
  }

  printMap() {
    this.items.forEach((item) => {
      let key: string[] = Object.keys(item);

      console.log('{ "' + key[0] + '": ', item[key[0]], '}');
    });
  }
}

// ------- JUST TESTING --------------------
const numb = new GenericMap < number > ();
numb.setItem('alpha', 7);
numb.setItem('beta', 14);
numb.printMap();
numb.clear();
numb.printMap();

const strMap = new GenericMap < string > ();
strMap.setItem('color', 'Blue');
strMap.setItem('height', '54 cm');
strMap.printMap();
// ---------------------------------------------

The compiled code as a live-demo:

class GenMap {
  constructor() {
    this.items = [];
  }
  setItem(key, item) {
    let tmp = {};
    tmp[key] = item;
    this.items.push(tmp);
  }
  getItem(key) {
    let ret = null;
    for (let i = 0; i < this.items.length; i++) {
      let curr = this.items[i];
      if (curr[key]) {
        return curr[key];
      }
    }
    return null;
  }
  clear() {
    this.items.length = 0;
  }
  printMap() {
    this.items.forEach((item) => {
      let key = Object.keys(item);
      console.log('{ "' + key[0] + '": ', item[key[0]], '}');
    });
  }
}
const numb = new GenMap();
numb.setItem('alpha', 7);
numb.setItem('beta', 14);
numb.printMap();
numb.clear();
numb.printMap();
const strMap = new GenMap();
strMap.setItem('color', 'Blue');
strMap.setItem('height', '54 cm');
strMap.printMap();

It works fine. But I guess there a better ways to solve these exercise.
Especially my getItem-method has become rather long.
Has someone an idea to solve the exercise in a more elegant way?
What do you think about my idea of storing objects within an array?
Looking forward to reading your answers and comments.

It took me a while to understand the way getItem() works in its current implementation. I even thought that there's a bug (after rereading the code, I see I was wrong about it).

Let's start with obvious. The ret variable; can be safely removed.
The whole search loop is counter-intuitive to me, because each element in the items array is an object which stores a value: T under a different key: string. If you really want to use items: object[], you'd rather have it's elements declared as { key: string, value: T }. And this immediately moves us to the next point that answers your question about the choice of the type of the backing object.
An entity that stores key-value pairs in TypeScript and JavaScript is Object. Why not use it instead? I don't know if using Object is disallowed by your real problem definition.

If it is not disallowed, it would be very natural to define items as an Object and wrap it into a GenericMap which exposes the API your problem requires.

However, if it is disallowed, you should not use it in the setItem()'s code (tmp[key] = item;) as well. Because it's technically same kind of a "cheat" anyway.

Below is the code that is built around an Object instead of an Array. As you can see, pretty much all of the operations are now "pass-through" one liners. Including the getItem() which you said got pretty long in the original code.

class GenericMap<T extends number | string> {
  private items: { [key: string]: T } = { };

  setItem(key: string, item: T): void {
    this.items[key] = item;
  }

  getItem(key: string): T {
    return this.items[key];
  }

  clear(): void {
    this.items = {};
  }

  printMap(): string {
    const itemsAsString = Object
      .getOwnPropertyNames(this.items)
      .map(key => `'${key}': ${this.getItem(key)}`)
      .join(", ");
    return `{ ${itemsAsString} }`;
  }
}

A few TypeScript style side notes:

private items: { [key: string]: T } = { };

If we initialize our fields in-place (as a part of their definition), we don't need an explicit constructor() which makes code shorter.
{ [key: string]: T } is a compiler hint that will -- at least in some cases -- help detecting incorrect usages of the field

I personally have never seen the whitespaces around < and > when used with generics, e.g. GenericMap < T extends number | string >. It's not TypeScript idiomatic I believe.

Hope, it helps.


Improving handling of data structure used in parallel I have a class extending the Dictionary class. This class is used for storing some information (modeled in CustomClass) and accessing it through an integer ID.

To extend this class I have added a TryAdd method, specific to my workflow, which implement different behaviours for the cases of trying to add a new or a already existing CustomClass object.

public class CustomDictionary : Dictionary<int, CustomClass>
    {
        private void TryAdd(int ID, CustomClass customObject)
        {
            if (this.ContainsKey(ID))
            {
                //some operations
            }
            else
            {
                //some others operations
                this.Add(customObject);
            }
        }
    }

There will be only one instance object for this class. I want to add locks to provide thread safety and syncronisation for the object of type CustomDictionary.
TryAdd and data accessing operation will frequently occur in parallel for this structure.

Have in mind that at the moment I can't use framework 4.0 so I can't use ConcurrentCollections.

To ensure thread safety I put this.Add(customObject); within a lock(this) but I have read that this is very bad.

Then I read about locking using a private object.

public class CustomDictionary : Dictionary<int, CustomClass>
    {
        private object lockObject = new Object();

        private void TryAdd(int ID, CustomClass customObject)
        {
            if (this.ContainsKey(ID))
            {
                //some operations
            }
            else
            {
                //some others operations
                lock(lockObject)
                {
                    this.Add(customObject);
                }
            }
        }
    }

Is this the good way of doing it? Should I also lock the CustomDictionary object when I read data from that object?

Any improvements for the implementation of this class would be helpfull.

First of all, it's very good that you read about the evilness of lock(this). Avoid it at all costs :)

As for your solution, it's not thread safe, in terms of functionality:

Thread A tries to add an object to id = 123.
Since this object doesn't exist, the ContainsKey returns false, so the false part of the condition takes place.
Then Thread A acquires the lock and starts to add its object.

However, Thread A hasn't yet completed its processing, and the system now switches to Thread B.

Strangely enough, Thread B wants to add its own object with the same id = 123 (!).

And, what do you know, since Thread A hasn't finished its processing, Thread B asks if the dictionary ContainsKey(123) and gets false. That's the problem.

Even worse, when Thread B starts its processing of adding the object, an exception would be thrown, since there's already a key assigned with 123.

So, no, the suggested code is not thread safe. That's a classic race condition.

How to resolve this?

Option 1: Double check on read operations

Inside the lock, you can call again to the ContainsKey, which is a read operation. MS provided a cheering statement, that read operations on its data structures (dictionary, list, etc.) are all thread safe.

Update: from MSDN: A Dictionary can support multiple readers concurrently, as long as the collection is not modified (my emphasis). So please ignore this option.

Option 2: Use ReaderWriterLockSlim Class

This class is optimized to the scenario of multiple reads / seldom writes. So when you want to write, you upgrade your lock to have "write" scope, and you continue your code.

Option 3: Use a simple lock on the entire TryAdd method. This is very straight forward.

All options are fine, and my personal bias is towards option 2.

Update: see also a related post at Ayende: Why is this not thread safe?

Good luck!


Copy object properties without using many if statements I have two class:

InputForm.java

public class InputForm {
    private String brandCode;
    private String caution;

    public String getBrandCode() {
        return brandCode;
    }

    public void setBrandCode(String brandCode) {
        this.brandCode = brandCode;
    }

    public String getCaution() {
        return caution;
    }
    public void setCaution(String caution) {
        this.caution = caution;
    }
}

CopyForm.java

public class CopyForm {
    private boolean brandCodeChecked;
    private boolean cautionChecked;

    public boolean isBrandCodeChecked() {
        return brandCodeChecked;
    }

    public void setBrandCodeChecked(boolean brandCodeChecked) {
        this.brandCodeChecked = brandCodeChecked;
    }

    public boolean isCautionChecked() {
        return cautionChecked;
    }

    public void setCautionChecked(boolean cautionChecked) {
        this.cautionChecked = cautionChecked;
    }
}

I want to copy values from an InputForm to another if its corresponding property in CopyForm is true.
This is what I do:

if(copyForm.isBrandCodeChecked()) {
    inputForm.setBrandCode(otherInputForm.getBrandCode());
}
if(copyForm.isCautionChecked()) {
    inputForm.setCaution(otherInputForm.getCaution());
}

The problem is I have many many properties. Writing many if statements seems ugly and bad programming practice.
How to solve it? (I know reflection is not a good choice so I don't think about it)

I think Apache commons BeanUtils might help you there. It uses reflection. Take this into consideration if performance is an issue...

We need 2 Steps:

1st create a method that copies a property if a condition is met

2nd create a copy method that passes all fields to the first method

Here we go...

public class InputForm {
    [...]

    private void copyProperty(InputForm srcForm, String field, boolean condition) {
        if(condition) {
            PropertyUtils.setProperty(this, field, PropertyUtils.getProperty(srcForm, field));
        }
    }

    public void copyFromOtherForm(InputForm srcForm, CopyForm conditions) {
        copyProperty(srcForm, "brandCode", conditions.isBrandCodeChecked());
        copyProperty(srcForm, "caution", conditions.isCautionChecked());
        [...]
    }
}

if you're still not pleased with that you could use reflection to invoke "is"+field+"Checked" on your CopyForm instance and change the calls to:

copyProperty(srcForm, conditions, "field");


Generating the maximum number possible from an array I have the following code in Scala as a solution to the problem:

Find the maximum number possible from the given Seq of Integers while keeping the individual numbers intact

e.g.

Seq(2,5,9,4) should result in 9542
Seq(4, 98, 10, 110, 91) should result in 9891411010

Note that in case of numbers with more than one digit, the entire number should remain together.
Solution in Scala:
import scala.util.Sorting
object MatheMagic {
  def getLargestPossibleNumber(numArray: Seq[Int]) = {
    numArray.sorted[Int](IntOrdering).mkString.toLong
  }
  object IntOrdering extends Ordering[Int] {
    def compare(x: Int, y: Int) = {
      -(x.toString.concat(y.toString).toInt compare y.toString.concat(x.toString).toInt)
    }
  }
}

Sample invocation:
MatheMagic.getLargestPossibleNumber(Seq(4, 98, 10, 110, 91))

Your algorithm is good, but the implementation is a bit wasteful:

In IntOrdering.compare, x and y are converted to String twice, and then converted to Int

It's especially bad since the numbers in the sequence may pass through compare multiple times in different pairs, and get converted to String even more times

You could reduce the casts:

Convert all numbers to String once in the beginning
Compare as strings. The algorithm will work with alphabetical comparison too
Convert to Long once at the end

Creating a custom Ordering seems a bit overkill

This one-liner will do the job:

nums.map(_.toString).sortWith(
  (a, b) => (a concat b).compareTo(b concat a) > 0).mkString.toLong


Handling conditional logic inside controller actions I am looking for best practice in handling conditions inside the controller actions in ASP.NET MVC:

public ActionResult Edit(int Id = 0)
{
   var Item = _todoListItemsRepository.Find(Id);
   if (Item == null)
      return View("NotFound");
   if (!Item.IsAuthorized())
      return View("NotValidOwner");

   return View("Edit", Item);
}

The above two ifs are used in other actions inside the controller. So, in order not to repeat these conditions in all the actions. I have used the below approach:

[HttpGet]
[Authorize]
[ModelStatusActionFilter]
public ActionResult Edit(int Id = 0)
{
    var Item = _todoListItemsRepository.Find(Id);
    return View("Edit", Item);
}

public class ModelStatusActionFilterAttribute : ActionFilterAttribute
{
    private readonly ITodoListItemsRepository _todoListItemsRepository;
    public ModelStatusActionFilterAttribute()
        : this(new TodoListItemsRepository())
    {

    }
    public ModelStatusActionFilterAttribute(ITodoListItemsRepository     todoListItemsRepository)
    {
        _todoListItemsRepository = todoListItemsRepository;
    }
    public override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        try
        {
            var Id = Convert.ToInt32(filterContext.RouteData.Values["Id"]);
            var Item = _todoListItemsRepository.Find(Id);
            if (Item == null)
            {
                filterContext.Result = new ViewResult() { ViewName = "NotFound" };
            }
            else if (!Item.IsAuthorized())
            {
                filterContext.Result = new ViewResult() { ViewName = "NotValidOwner" };
            }
        }
        catch
        {

        }
    }
}

I am unsure if this is the best practice in handling such scenarios. Could someone please advise?

This is just copy of my answer to your question on SO.

Usually you don't use action filters for so-called business logic of your web application - this is what the controllers are for. Action filter are rather for the whole stuff which is external to the actual logic - common case is logging, performance measurement, checking if user is authenticated / authorized (I don't think this is your case, although you call IsAuthorized method on the "Item").

Reducing code is generally good thing but in this case, I don't think putting the logic to action is a good way, because you've actually made it a bit unreadable, and unreadable code is in my opinon much worse than repeated code. Also, specifically in your case, for all valid items you actually call the _todoListItemsRepository.Find() twice (for each valid item), which might be costly if this is some webservice call or db lookup.

If the code is just repeated throughout the actions, make a method out of it like:

private View ValidateItem(Item) {
    if (Item == null)
      return View("NotFound");
   if (!Item.IsAuthorized())
      return View("NotValidOwner");
return null; }


Refactoring String manipulating method with TDD I'd like to refactor my extractSimplePromptName test so it does the same but it's "prettier", hence less code. As far as looking at it, it seems like it could be refactored so it does the same thing, but it's just less code. I'm just fairly new to programming so having a hard time seeing where I should make the changes.

Guessing it has something to do with putting some stuff out of the first if sentence or perhaps just removing something that doesn't do anything.

I just ain't that certain, and I'd really like some help so I can get better at doing decent code.

@Test
public void makeSimpleName1() {
    String promptNameOrg = "20129142\\1234";

    String simplePromptName = extractSimplePromptName(promptNameOrg);
    Assert.assertEquals("1234", simplePromptName);
}

@Test
public void makeSimpleName2() {
    String promptNameOrg = "80808080\\159;20129142\\1234";

    String simplePromptName = extractSimplePromptName(promptNameOrg);
    Assert.assertEquals("1234", simplePromptName);
}

@Test
public void makeSimpleName3() {
    String promptNameOrg = "159;1234";

    String simplePromptName = extractSimplePromptName(promptNameOrg);
    Assert.assertEquals("1234", simplePromptName);
}

private String extractSimplePromptName(String promptNameOrg) {
    String simplePromptName = "";
    if (promptNameOrg != null) {
        if (promptNameOrg.contains(";") || promptNameOrg.contains("\\")) {
            List<String> splitPrompts = Arrays.asList(promptNameOrg.split(";"));

            String promptName = splitPrompts.get(splitPrompts.size() - 1);
            if (promptName.contains("\\")) {
                String[] split = promptName.split("\\\\");
                List<String> splitPromptName = Arrays.asList(split);
                simplePromptName = splitPromptName.get(splitPromptName.size() - 1);
            } else {
                simplePromptName = promptName;
            }

        } else {
            simplePromptName = promptNameOrg;
        }

    }
    return simplePromptName;
}

Often, when performing text manipulation, Regular Expressions can do the work for you. Your solution is essentially manually building a text parser and stripper, when a regular expression will do the work in a much more concise way.

The regular expression would be to remove all text up to the last \ or ; character, which would be written as:

private String extractSimplePromptName(String promptNameOrg) {
    return promptNameOrg == null ? "" : promptNameOrg.replaceAll("^.*(\\\\|;)", "");
}


JavaScript code for countdown function is maxing CPU usage I have a script for a countdown function, which is using massive amounts of CPU power on my MBP.

The script updates the countdown display every second and is updating all piecharts as well.

    var options = {
        scaleColor: false,
        trackColor: 'rgba(255,255,255,0.3)',
        barColor: '#E7F7F5',
        lineWidth: 6,
        lineCap: 'butt',
        size: 95
    };

    $('#days').easyPieChart(options);
    $('#hours').easyPieChart(options);
    $('#minutes').easyPieChart(options);
    $('#seconds').easyPieChart(options);

function countdown(endT,callback) {
        var first_load = false;
        var days,hours,minutes,sec,timer;

        end = new Date(endT);

        end = end.getTime();                     //Get initial Date in Milliseconds
        if (isNaN(end)) {
            return;

        }
        var tot_current = new Date();
        var tot_remain = parseInt((end - tot_current.getTime())/1000);
        var tot_days = parseInt(tot_remain/86400);

        timer = setInterval(calculate,1000);
        function calculate(){
            var current = new Date();
            var remaining = parseInt((end - current.getTime())/1000); //remaining seconds

            if (remaining <= 0){
                clearInterval(timer);
                days=0;
                hours=0;
                minutes=0;
                sec=0;
                display(days,hours,minutes,sec);
                if (typeof callback === 'function' ) {
                    callback();
                }

            }else{

                days = parseInt(remaining/86400);
                remaining = (remaining%86400);
                hours = parseInt(remaining/3600);
                remaining = (remaining%3600);
                minutes = parseInt(remaining/60);
                remaining = (remaining%60);
                sec = parseInt(remaining);
                display(days,hours,minutes,sec);

            }
        }
        function display(days,hours,minutes,sec) {
            var dl = days.toString().length;
            if (dl == "1") {
                sl = 2;
            }else{
                if (isNaN(dl)) {
                    sl = 3;
                }
                sl = dl;
            }

            days_rem = ("00"+days).slice(-sl);
            hrs_rem = ("0"+hours).slice(-2);
            min_rem = ("0"+minutes).slice(-2);
            sec_rem = ("0"+sec).slice(-2);
            $("#days span").text(days_rem);
            $("#hours span").text(hrs_rem);
            $("#minutes span").text(min_rem);
            $("#seconds span").text(sec_rem);
            $("#days").data('easyPieChart').update((100/tot_days)*days_rem);

            // Disable animation for the first load
            if(hrs_rem == 23 && first_load) { $('#hours').data('easyPieChart').disableAnimation(); }
            $("#hours").data('easyPieChart').update((100/23)*hrs_rem);
            if(hrs_rem == 23 && first_load) { $('#hours').data('easyPieChart').disableAnimation(); }

            if(min_rem == 59 && first_load) { $('#minutes').data('easyPieChart').disableAnimation(); }
            $("#minutes").data('easyPieChart').update((100/59)*min_rem);
            if(min_rem == 59 && first_load) { $('#minutes').data('easyPieChart').enableAnimation(); }

            if(sec_rem == 59 && first_load) { $('#seconds').data('easyPieChart').disableAnimation(); }
            $("#seconds").data('easyPieChart').update((100/59)*sec_rem);
            if(sec_rem == 59 && first_load) { $('#seconds').data('easyPieChart').enableAnimation(); }

            first_load = true;
        }

    }

    var d = new Date(2015, 6, 10, 12, 7, 0, 0);
    countdown(d,null);

The code can be seen in action here.

First, I'll get to the fix for your problem.  The problem is more in the CSS than in your JavaScript.  Because you are updating the graph every second, that area of the page has to be repainted each time.  Due to the way browsers handles these things by default, it has to also check the surrounding area to see if it now needs to be changed as well.  So the idea is to minimize this.  And the simplest way to do this is to add the following lines into your CSS:
.chart {
  transform: translateZ(0);
  z-index:1;
  /* future proofing */
  will-change: transform;
}

What this code does is move the charts onto there own rendering layer. This prevents the browser from having to check the surrounding areas because each chart is now separated from the rest of the content.  In Chrome, you can see the performance difference this makes by watching the Timeline in the Dev tools.  In the original, it spends ~180 ms repainting the screen and ~100 ms performing calculations.  In the updated version, it spends ~20 ms for each.  Add this over four graphs and you can see why the cpu is pegged.  The will-change is a new CSS property that will basically do the same thing as the two lines above in the somewhat-near-future.  I have updated the fiddle with just this change and you can see the difference.
Now, the code review:
As always, the first thing I recommend is to remove your code from the global scope.  The easiest way to do this is to use an IIFE.  This will basically create a private scope for all your code which reduces the amount of code you place in the global scope, which in turn, reduces the chance of collisions with other peoples code.  Since you are using jQuery, this also provides a way to make sure $ always points to it.
(function( $, window, document, undefined ) {
  //your code here
})( jQuery, window, document );

So in the above statement, we are passing jQuery as $, as well as the window and document objects (for a slight performance boost) and, to deal with older browsers, making sure undefined is actually undefined.
The next thing you could do is cache your jQuery selectors.  This will improve the overall performance because selecting an element in the DOM is one of jQuery's slowest performing functions.  Your are using ID's which help but caching them is even quicker.  We want to do this as soon as the DOM is ready.  I usually create a single function for everything that has to happen there and then call the single function.  Also, since we will be referring to these selectors we will declare them immediately inside of our IIFE. I have also included the reference to create the piecharts:
(function( $, window, document, undefined ) {

  var $days, $daysText, $hours, $hoursText,
      $minutes, $minutesText, $seconds, $secondsText;

  function init() {

    var options = {
      scaleColor: false,
      trackColor: 'rgba(255,255,255,0.3)',
      barColor: '#E7F7F5',
      lineWidth: 6,
      lineCap: 'butt',
      size: 95
    };

    getSelections();

    $days.easyPieChart( options );
    $hours.easyPieChart( options );
    $minutes.easyPieChart( options );
    $seconds.easyPieChart( options );

  }

  function getSelections() {

    $days = $('#days'),
    $daysText = $days.find('span'),
    $hours = $('#hours'),
    $hoursText = $hours.find('span'),
    $minutes = $('#minutes'),
    $minutesText = $minutes.find('span'),
    $seconds= $('#seconds'),
    $secondsText = $seconds.find('span');

  }

  $(function() {   //shortcut for document.ready event
    init();
  });

})( jQuery, window, document );

One other big change you need to make is to not update all four pie charts every second.  There is only one chart that needs updating that often (seconds obviously).  You should only update the minutes chart every minute, the hour every hour and the days every day.  That way, on almost every interaction you are only updating the one chart.
Another way you could improve this code is to DRY it out a bit.  One place that would greatly benefit from this is the display function.  We need to make changes here anyway to help out with the problem mentioned above.
Let's take this code:
days_rem = ("00"+days).slice(-sl);
hrs_rem = ("0"+hours).slice(-2);
min_rem = ("0"+minutes).slice(-2);
sec_rem = ("0"+sec).slice(-2);
$("#days span").text(days_rem);
$("#hours span").text(hrs_rem);
$("#minutes span").text(min_rem);
$("#seconds span").text(sec_rem);

Notice how that is pretty much the same thing repeated over and over?  That means it needs to be it's own function:
updateText( $daysText, days );
updateText( $hoursText, hours );
updateText( $minutesText, minutes );
updateText( $secondsText, seconds );

function updateText( $el, val) {
  var len = val.length;
  val = ( len === 1 ) ? '0'+val : val;
  $el.text( val );
}

With this function, val will always be at least 2 characters long.
Now before updating the charts, check to see what are current value is.  If it's the same as the new value (which will be typical for minutes, hours, days), then there is no reason to update them (or there text for that matter). Adding a data element with jQuery is a great way to keep track of this.
if ( $days.data('remaining') !== days_rem) {
  // update it
  updateText( $daysText, days );
  $days.data('remaining', days_rem);
  $days.data('easyPieChart').update( ( 100 / tot_days) * days_rem );
}

The first time through, the remaining data element is not set so it will update the chart.  Each other time through, it will check that existing value and only update the chart when the value actually changes.  Once the code is added for each chart, it becomes obvious that this can be DRY-ed out as well. Maybe with something like this:
updateChart( $days, $daysText, days_rem, ( 100 / tot_days ) * days_rem );
function updateChart( $chart, $chartText, newValue, formula ) {
  //code here
}

I will leave that code up to you.
At this point, you should see a noticeable increase in the performance of the page.
Some other things to consider:
In your calculate function you have a bunch of Magic Numbers such as 86400. Although this is fairly straight forward when you know what you are looking at, it might be easier to set up some variables to hold those that provide some explanation for them:
var SECONDS_IN_A_DAY = 86400;
days = parseInt( remaining / SECONDS_IN_A_DAY , 10 );

Also notice, the radix parameter for the parseInt function.  It isn't required but it's one of those things you should use anyway.  That way the parser doesn't take the extra time to figure this out on it's own.
I hope that helps! Feel free to post any comments or questions.
Updates based on comments/fiddle
I have created a fiddle with all of the updated code, including some of the things you were trying to do on your own fiddle.  To show you the performance improvements, I created a couple of screen captures.  Note these are both showing when the seconds chart goes from 1 to 59.
This first one is your original fiddle linked in your post.
Performance Before
This is the updated fiddle I linked to above.
Performance After
By reducing the number of times each of the charts have to update and using the CSS above, you can see the execution time has improved significantly.  These samples were taken with Chrome dev tools.  If you are seeing significant CPU thrashing, try running in an incognito window to see if it is something else (tab or extention) interfering with your performance.


Generating (presumably) unique `nonce` fields for Twitter OAuth This is code that's part of a library (closed source) that I have that generates a unique nonce value for Twitter OAuth. Essentially, it generates a random number, combines it in string form with an extra string separated by a pipe, computes a Sha-1 hash, and then repeats the process if the Sha-1 hash (when converted to Base64) contained any non-alpha-numeric characters, which are subsequently stripped.

/// <summary>
/// Generates a (presumably) unique NONCE string for use in Twitter OAuth requests.
/// </summary>
/// <param name="extra">Extra data to add during generation.</param>
/// <returns>
/// A (presumably) unique NONCE string.
/// </returns>
/// <remarks>
/// While we can assume the generated NONCE strings will be somewhat unique, there is a small possibility that they will not be, and in such cases a new NONCE should be generated.
///
/// The generated NONCE values are 32 characters in length, and are generated by a Sha-1 hash and a Random generator, which generates four random numbers to be hashed until the generated string is 32 characters in length.
/// </remarks>
public string GenerateNonce(string extra = "")
{
    string result = "";
    SHA1 sha1 = SHA1.Create();

    Random rand = new Random();

    while (result.Length < 32)
    {
        string[] generatedRandoms = new string[4];

        for (int i = 0; i < 4; i++)
        {
            generatedRandoms[i] = rand.Next().ToString();
        }

        result += Convert.ToBase64String(sha1.ComputeHash(Encoding.ASCII.GetBytes(string.Join("", generatedRandoms) + "|" + extra))).Replace("=", "").Replace("/", "").Replace("+", "");
    }

    return result.Substring(0, 32);
}

The idea is to generate somewhat unique nonce strings for use with Twitter OAuth (et al.).

I use the term "presumably," because, while it's not considered a bug that the nonce strings will not be guaranteed to be unique, collisions are expected to be fairly rare. I do welcome any and all suggestions that would lead to more uniqueness.

If I understand the requirements correctly, Guid.NewGuid().ToString("N") would work for generating a nonce.

  A GUID is a 128-bit integer (16 bytes) that can be used across all computers and networks wherever a unique identifier is required. Such an identifier has a very low probability of being duplicated.

The "N" format specifier will return the GUID as 32 (lowercase) hexadecimal digits.


JavaScript animation effect stack I am building a library that uses animations in a canvas.

I need to apply effects to some objects in the canvas and I came up with an effect stack that contains each effect to be executed on a object. Each object has it's own effect stack, because each effect has some random values to them.

As each effect needs to be executed on a set of frames, I need those to get back in the effect stack to be continued in the next frame.

Here is what I want:

Note that each effect in the stack is consumed once. If the effect is not completed, it is reinserted on the stack.

I came up with a couple of solutions:

I pop() the effect, consume it and unshift() it in the stack. Reading only the top of the stack n times (n -> nb of effects).
The opposite is also possible: shift() -> consume() -> push()

The problem with these 2, is the shift() and the unshift() as it copies all the values in memory to another location. I'd prefer to use pop() or push() only.
I use 2 stacks. One for the current effects to be consumed and the other for the effects that are consumed. And each frame, I copy the values from #2 to #1, then reset the #2.

But now, I have to create 2 stacks for each object and add a function to process those, which complicates all the code that needs access to these effects.

A basic implementation for the second solution is:

var EffectStack = function() {
    this.stack = [];
    this.queued = [];
}

EffectStack.prototype.length = function() {
    return this.stack.length;
};

EffectStack.prototype.push = function( fx ) {
    this.stack.push( fx );
};

EffectStack.prototype.pop = function( fx ) {
    return this.stack.pop( fx );
};

EffectStack.prototype.update = function() {
    this.stack = this.queued.slice(0);
    this.queued = [];
};

EffectStack.prototype.continue = function( fx ) {
    this.queued.push( fx );
};

And for the animated object:

/**
 * Applies each effect and updates the Effect Stack by queuing
 * the effects that weren't complete.
 */
AnimObject.fn.consumeEffects = function() {
    var fx
    while( this.eStack.length() > 0 ) {
        // fx = {
        //     func: function(){ ...core of the effect... },
        //     data: { ... }
        // }
        fx = this.eStack.pop();
        fx.func( this, fx );
    }

    this.eStack.update();
}

Effect Example

/**
 * This is an Effect (fx).
 */
changeOpacity: function( value, step ){
    return {

        // Basic information.
        data: {
            step: step ? step : Math.random()*0.06 + 0.04,
            targetValue: value ? value : Math.random()*0.7 + 0.2
        },

        // The core of the effect.
        func: function( particle, fx ){
            if(!FX.opacity_FUNC( animObj, fx.data )) {
                // **IMPORTANT**
                // Add to stack if the effect isn't over.
                animObj.eStack.continue({
                    data: fx.data,
                    func: fx.func
                });
            }
        }
    }
}

/**
 * -- THIS IS A SUB_EFFECT. IT CONTAINS THE CORE LOGIC --
 * -- SUB_EFFECTS CAN BE MIXED TOGETHER. --
 * Changes the opacity of a AnimObject that is floating.
 *
 * @param animObj   : Affected AnimObject.
 * @param data      : Transformation data.
 */
opacity_FUNC: function( animObj, data ) {

    // Distance to targetValue.
    var _delta = (animObj.opacity - data.targetValue);

    // If the <animObj> still validates the conditions,
    // continue the effect till it's done.
    if ((animObj.isFloating || animObj.isMoving) && Math.abs(_delta) > 0.1)
    {
        animObj.opacity += data.step * (_delta > 0 ? -1 : 1);
        return false;
    }

    // Effect is over.
    else
    {
        animObj.opacity = data.targetValue;
        return true;
    }
},

Here is a little jsfiddle of what my sandbox looks like.

But, I still need to copy the values. Any better ideas?

You want a data structure that supports efficiently manipulating head and tail. Usually we call that a deque, a double-ended queue, sometimes implemented with linked lists.

As I look at your diagram, the containers on top and bottom in some sense hold the same R,B,Y sequence of items, just in cyclic permutation. So why don't you consider using a simple array of size N, with head and tail indices that you increment mod N ? Then you only incur expensive memory copies when an effect finally completes.


Making sure initialization code is executed before other code using Lazy Initialization The Problem

Recently I was told to refactor i.e. move some initialization logic from the constructor. I came up with something similar to the posted code.
In actual there are bunch of loader classes inheriting from ILoader and doing all kind of stuff (TPL, DB access,XML parsing, CSV Writing)
One of our UBER Developers reviewed my code and give some really appreciative comments particularly with the line of code marked with //<--:

  Wow – I have seen a lot but that would make the
  http://thedailywtf.com/ for sure. HOW can you think of having a Getter
  method have a initialization SIDE EFFECT??

What is your opinion guys. What should I've done to avoid such appreciative comments?

The Solution

using System;

public class Program
{
    public static void Main()
    {
        var obj = new TestLoader("testing");
        Console.WriteLine("Calling Run() first time....");
        obj.Run();
        Console.WriteLine("Calling Run() again...");
        obj.Run();
        Console.WriteLine("Calling SomeOtherMethod()...");
        obj.SomeOtherMethod();
    }
}

//Class to make sure initialize will only be called once
public abstract class DataInitializer<T>
{
    public bool IsInitialized { get { return _isInitialized.Value; } }//<--
    readonly Lazy<bool> _isInitialized;
    protected DataInitializer(T param)
    {
        Console.WriteLine("Inside DataInitializer constructor");
        _isInitialized = new Lazy<bool>(Initialize);
    }
    protected abstract bool Initialize();
}

//Some loader class
public class TestLoader :DataInitializer<string>, ILoader
{
    public TestLoader(string test):base(test)
    {
    }

    protected override bool Initialize()
    {
        Console.WriteLine("Inside Initialize()");
        return true;
    }

    public void Run()
    {
        if(IsInitialized)
        {
            Console.WriteLine("Inside Run()");
        }
    }

    public void SomeOtherMethod()
    {
        if(IsInitialized)
        {
            Console.WriteLine("Inside SomeOtherMethod()");
        }
    }

}

interface ILoader
{
    void Run();
    void SomeOtherMethod();
}

Your code does feel like a misuse of lazy evaluation. Some alternative approaches to consider:

MakeInitialize method public and throw if it was not called:

public override void Initialize()
{
    //do stuff
    _isInitialized = true;
}

public void Run()
{
    if(!_isInitialized) throw new InvalidOperationException();
    //do stuff
}

IMHO, it is all-around best option, as it allows you to control when to do initialization.
Call initialization automatically on every Run call:

protected override void Initialize()
{
    if (_isInitialized) return;
    //do stuff
    _isInitialized = true;
}

public void Run()
{
    Initialize();
    //do stuff
}

This approach is good in some cases. For example, when you have an unstable network connection, it makes sense to check the connection before every method call, and reconnect if needed.
Use Lazy objects to initialize actual services:

public CsvWriter CsvWriter { get { return _csvWriter.Value; } }
readonly Lazy<CsvWriter> _csvWriter;

public void Run()
{
    CswWriter.Write(...)
}

I'm not a fan of lazy initialization, but it is fine, if you use it to instantiate one or two actual services. Laziness is justified in scenarios when there are high chances that Run method will not be called at all by your software.

P.S. Note, that Lazy is thread-safe, while options #1 and #2 are not and require synchronization if thread-safety is required.