XKCD Password Generator I'd like to verify that the security of this JavaScript code is correctly doing what I believe it's doing.  Its intended purpose is to generate xkcd-style passwords randomly in the visitor's browser.

When the page loads, the full wordlist is loaded into the browser synchronously (>350K words) and then generates a secure seed.

  $(document).ready(function() {
    wordlist = [];

    $.ajax({ url: '../xkcd_wordlist.txt',
      async: false,
      success: function(data) {
        wordlist = data.split('\n');
      }
    });

    Math.seedrandom();
    updatePasswordField();
  });

There are various other UI things happening before the main updatePasswordField function is called, but I've omitted those for brevity.  Once the updatePasswordField is called, it randomly chooses from the list that was fetched earlier.

wordlist[Math.floor(Math.random()*wordlist.length)]

I can't see anything wrong with this setup, but would like to make sure I can get a second set of eyes (or as many as would like) before I declare it secure.

From a position of security, is this sufficiently random?

Relevant code sections:

  function updatePasswordField() {
    // Generate and Return the XKCD style password
    var step;
    var final_xkcd_password = "";
    var number_of_words = $('#word-count').val();

    for (step = 0; step < number_of_words; step++) {
      final_xkcd_password = final_xkcd_password + " " + wordlist[Math.floor(Math.random()*wordlist.length)];
    }
  };

  $(document).ready(function() {
    wordlist = [];

    $.ajax({ url: '../xkcd_wordlist.txt',
      async: false,
      success: function(data) {
        wordlist = data.split('\n');
      }
    });

    Math.seedrandom();
    updatePasswordField();
  });

Let's get the security aspect out of the way first.

The idea of the XKCD password is a set of characters that is hard to guess, but easy to remember (in the form of words that only you know). This code fails on both one criteria.

The word list trims possibilities to a known set of words. See discussion in comments.
Random words doesn't mean easy to remember.

Your code did conform to the concept. I just wouldn't use it to generate my password. It would be a good password suggestion tool though.

Now over to your code.

$.ajax({ url: '../xkcd_wordlist.txt',
  async: false,
  success: function(data) {
    wordlist = data.split('\n');
  }
});

You gain nothing from making this request synchronous. This will make the UI freeze while waiting. If your goal was to prevent that input from being updated while loading, you could just set the readonly property until the request succeeds.

In addition, I suggest you use promises and the method then instead of the success option to set the callback. It's better practice as promises are standard. Even if you don't use native promises, getting used to using then will just reinforce your muscle memory when using promises.

The jQuery function also doubles as $(document).ready() when given a function. Also, since you're just doing a GET request, just use the $.get shorthand.

Consider splitting off the password generator into its own function so that it becomes reusable as well as split from the UI logic that is updating the password field. Also, since I suggested async fetching of the word list, this means your UI logic can be called with an empty word list. Make the code throw an error when the word list is empty and handle it accordingly.

$(function(){

  var wordList = [];
  var passwordInput = $('#password-input');
  var stepsInput = $('#word-count');

  function generatePassword(wordList, words){
    var password = '';

    for(var i = 0; i < words; i++){
      password += wordList[Math.floor(Math.random() * wordlist.length)];
    }

    return password;
  }

  function updatePasswordField(){
    if(!wordList.length) throw new Error('Word list not populated');
    passwordInput.val(generatePassword(wordlist, stepsInput.val()));
  }

  passwordInput.prop('readonly', true);

  $.get('../xkcd_wordlist.txt').then(function(data){
    wordList = data.split('\n');
    passwordInput.prop('readonly', false);
    updatePasswordField();
  });

});


Simplify and prettify function looking for max value in a list and modifying some underlying values within that list This function is used to modify values in list List<GoogleUsageMapping> and prepare them for entering into billing system.

I'm hoping this function can be more simplified and prettified, logic works.
Function needs to keep track of maxValue and if the maxValue is of type LOGICAL_SIZE. Would love to eliminate one of the for-loop if possible for instance.

private void checkForOverage(List<GoogleUsageMapping> records) {
    BigDecimal highestQuantity = new BigDecimal(0);
    GoogleUsageMapping recordHighestQuantity = null;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getQuantity().compareTo(highestQuantity) > 0) {
            highestQuantity = record.getUsage().getQuantity();
            recordHighestQuantity = record;
        }
    }

    if (recordHighestQuantity.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
        s_logger.info("Overage scenario accord LOGICAL_SIZE: " + recordHighestQuantity.getUsage().getQuantity() + " greater than ALLOCATED_SIZE");
        for (GoogleUsageMapping record : records) {
            if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
                continue;
            }
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

modified version:

private void checkForOverage(List<GoogleUsageMapping> records) {
    GoogleUsageMapping recordUsageHighestQuantity = Collections.max(records, Comparator.comparing(c -> c.getUsage().getQuantity()));

    if (recordUsageHighestQuantity.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
        s_logger.info("Overage scenario accord LOGICAL_SIZE: " + recordUsageHighestQuantity.getUsage().getQuantity() + " greater than ALLOCATED_SIZE");
        for (GoogleUsageMapping record : records) {
            if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
                continue;
            }
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

If you want to get rid of a for-loop just for code clarity, you could use a Stream and the max() function. (Assuming you're using Java 8+). You could also use Streams for the second for-loop.

Note however, it's not logically possible to avoid iterating through the list twice. You have to check every element to get the largest value, and again to set values on every item in the list.

I don't see you using highestQuantity, it's not really necessary since you have a pointer to recordHighestQuantity already.


zsh system status script The following script is being used to generate the status line in wmii. I'm curious if there are faster/more efficient ways to find the various info I'm grabbing. I'm also a relative newbie when it comes to zsh (and shell scripting in general) so I'd also suspect that there are better ways to process some of the data.

#!/bin/zsh
cpu_freq=$(grep "cpu MHz" /proc/cpuinfo | sed 's/[\t ]*//g; s/cpuMHz://g')

cpu0=$(echo "scale=2;" `echo $cpu_freq | head -n 1 | tail -n 1` / 1000 | bc)
cpu1=$(echo "scale=2;" `echo $cpu_freq | head -n 2 | tail -n 1` / 1000 | bc)
cpu2=$(echo "scale=2;" `echo $cpu_freq | head -n 3 | tail -n 1` / 1000 | bc)
cpu3=$(echo "scale=2;" `echo $cpu_freq | head -n 4 | tail -n 1` / 1000 | bc)

stat=$(mpstat | tail -n 1 | tr -s ' ')
usr=$(echo $stat | cut -d ' ' -f 4)
nice=$(echo $stat | cut -d ' ' -f 5)
sys=$(echo $stat | cut -d ' ' -f 6)
cpu=$(echo $usr + $nice + $sys | bc)

memline=$(free | head -n 2 | tail -n 1 | tr -s ' ')
cached=$(echo $memline | cut -d ' ' -f 7)
free=$(echo $memline | cut -d ' ' -f 4)
cachedfree=$(echo $cached + $free | bc)
mem=$(echo "scale=3;" $cachedfree / 1000000 | bc)

network=$(nmcli nm | sed 's/  */\;/g' | cut -d \; -f 2 | tail -n 1)

tun0=$(ifconfig tun0 2>/dev/null)
vpn=$(if [[ -n $tun0 ]]; then; echo "(vpn:" $(cat /tmp/current-vpn)")"; else; echo ""; fi)

battery=$(acpi -b | sed 's/Battery 0: //; s/ until charged//; s/ remaining//; s/Charging/C/; s/Discharging/D/; s/Full, //; s/Unknown, //')

load=$(uptime | sed 's/.*://; s/,//g')

date=$(date +%a\ %b\ %d\ %H:%M)

echo -n $vpn $network '||' $mem GB '||' $cpu% '('$cpu0 $cpu1 $cpu2 $cpu3') ||' $load '||' $date '||' $battery

Extracting a column

It's a lot easier to extract columns from text using awk.
Instead of this:

grep "cpu MHz" /proc/cpuinfo | sed 's/[\t ]*//g; s/cpuMHz://g'

This is shorter, and uses a single awk process instead of grep + sed:

awk '/cpu MHz/ {print $4}' /proc/cpuinfo

Extracting a line

This is a really awkward method to extract the 4th line:

echo $cpu_freq | head -n 4 | tail -n 1

This is simpler and better:

sed -ne 4p <<< "$cpu_freq"

Using arrays

Instead of getting the values of cpu0..cpu3 one by one by running separate command(s) for each, you can get it in one go:

cpus=($({ echo scale=2; awk '/cpu MHz/ {print $4 " / 1000"}' /proc/cpuinfo; } | bc))

Explanation:

The awk command appends the " / 1000" to the CPU frequency, so it's ready for piping to bc
Pipe all lines (commands) to bc at once, and get all the output at once
Group the echo scale=2 and the awk for bc by enclosing in { ...; }
Store the output of bc in an array

From this array, you can extract the individual CPU values

cpu0=${cpus[0]}
cpu1=${cpus[1]}
cpu2=${cpus[2]}
cpu3=${cpus[3]}

Next steps

I don't want to spoil all the fun for you :-)
Following the techniques above,
you should be able to extract the other values more efficiently (using fewer processes) in a similar fashion.


Unquoting booleans, numbers, etc. in a JSON string I have a method that takes a JSON string as input and "fixes" it. By that, I mean, that the JSON string comes in with all values quoted. My method un-quotes the values that shouldn't be quoted (i.e. true, false, numbers, {}, etc). The method was originally doing it by doing 4 string replaces (for the true / false, etc) and regex replace for the numbers. Kind of slow. So I re-wrote it with StringBuilder:

    protected static string FixupJson(string json)
    {
        if (json == null)
            throw new ArgumentNullException(nameof(json));

        int nLen = json.Length;

        StringBuilder sb = new StringBuilder(nLen);

        for (int i = 0; i < nLen; i++)
        {
            char ch = json[i];

            if (ch == ':' && json[i+1] == ' ' && json[i + 2] == '\"')
            {
                int k = json.IndexOf('\"', i + 3);

                sb.Append(": ");

                string g = json.Substring(i + 3, k - i - 3);

                bool bQuote = true;

                if (g == "true" || g == "false" || g == "null" || g == "{}")
                    bQuote = false;
                else if (g == "NaN")
                    bQuote = true;
                else if (Double.TryParse(g, out double d))
                    bQuote = false;

                if (bQuote)
                    sb.Append('\"');

                sb.Append(g);

                if (bQuote)
                    sb.Append('\"');

                i = k;
            }
            else
            {
                sb.Append(ch);
            }
        }

        return sb.ToString();
    }

This got me a 35% improvement, but I want to see if there is a better approach or if I can get this faster. My first StringBuilder approach, instead of appending char by char would append chunks pulled out with SubString(). That was actually a lot slower then this version.

Yes, I understand the right way to do it would be to fix it at the source :), which I do control, but the way the json gets serialized out at the source, I don't know if the value needs to be quoted until the json is fully spit out (or at least until the value is fully spit out) as a single value can be a formatted string from multiple sources (i.e. "myVal": "(this part from one place)|(this part from another place))". So "fixing it there" would mean going from a single String.Format() for the entire json to a bunch of them for each value. Right now the JSON is "pre-rendered" (once) and passed into a String.Format() to fill in the values, so by fixing it there I'd have to go to a bunch of little String.Format()'s to format each value then pass it in to the big String.Format()… so I figure this is a decent compromise.

EDIT: Should handle any old json, but this was the one I was using in my test app in C# string format ready to paste :).

"{\r\n  \"xxx\": \"TRACE|1\",\r\n  \"Timestamp\": \"2019-01-01 12:44:36.529\",\r\n  \"Message\": {\r\n    \"a\": \"bbb\",\r\n    \"b1\": {\r\n      \"b2\": \"test2\",\r\n      \"b3\": \"Test1\",\r\n      \"b4\": \"true\",\r\n      \"b5\": \"178\"\r\n    }\r\n  },\r\n  \"ThreadId\": \"1\",\r\n  \"Exception\": \"{}\"\r\n}"

I find your method for fixing invalid JSON is the worst possible because it treats JSON as a pure string and not a tree of properties/keys and values. This increases the possibility of changing values that might not be values at all. IMO the optimal approach would be to parse it with JSON.NET by using JToken.Parse and recreate the resulting tree with correct values.

You can take a look how it could be done here where I'm using the Visitor Pattern to transform JSON too.

But honestly, unless you need to be able to read some old data, you should really fix the serializer so that it outputs valid JSON instead of trying to fix its bugs with even more dirty workarounds.


Extended Euclidean Algorithm in modern and readable C++ In Python the Extended Euclidean Algorithm (egcd) could be written as follows:

def egcd(a, b):
  if b == 0:
    return (a, 1, 0)
  else:
    (d, tmp, s) = egcd(b, a%b)
    return (d, s, tmp - (a//b) * s)

I want to write a native and modern C++ version of the egcd. This is what I have so far:

template<typename A, typename B, typename T = std::common_type_t<A, B>>
constexpr std::tuple<T, T, T> egcd(A a, B b) {
  static_assert(std::is_integral<A>::value, "arguments to egcd are integers");
  static_assert(std::is_integral<B>::value, "arguments to egcd are integers");

  if (b == 0) {
    return std::make_tuple(a, 1, 0);
  } else {
    auto triple = egcd(b, a%b);
    return std::make_tuple(std::get<0>(triple), std::get<2>(triple), std::get<1>(triple) - (a/b) * std::get<2>(triple))
  }
}

As you can see using <tuple> makes the code very long and hard (compare with the Python version..) to read.

Any suggestions to make this code more readable?

The code is rather tiny so there isn't much to say, but I still have a couple of remarks:

Since you're using C++17, you can take advantage of variable templates to simplify your static assertions a bit:

static_assert(std::is_integral_v<A>, "arguments to egcd are integers");
static_assert(std::is_integral_v<B>, "arguments to egcd are integers");

The LWG issues 2733 and 2759 highlight the fact that any possibly cv-qualified bool satisfies the std::is_integral trait, but the GCD and LCM operations don't make much sense for boolean types. Consequently, the extended operations don't make much sense for boolean types eithers. You could add the following static assertions to make it clear:

static_assert(not std::is_same_v<std::decay_t<A>, bool>, "arguments to egcd are not booleans");
static_assert(not std::is_same_v<std::decay_t<B>, bool>, "arguments to egcd are not booleans");

Since you're always computing both a / b and a % b, you can compute std::div instead which computes both at once and returns a structure with quot and rem members. Actually, the algorithm used to compute the remainder of a and b also computes the quotient of a and b, so functions like std::div exist in several programming languages so that users can take advantage of it instead of discarding a computed value and computing it again.
They're not widely available at the moment, but if you intend to keep returning an std::tuple, a C++17 decomposition declaration would probably make your code more readable by providing explicit names instead of the opaque std::get<position>:

if (b == 0) {
    return std::make_tuple(a, 1, 0);
} else {
    auto divmod = std::div(a, b);
    auto [gcd, bezout_x, bezout_y] = egcd(b, divmod.rem);
    return std::make_tuple(gcd, bezout_y, bezout_x - divmod.quot * bezout_y)
}


Determining if there is data left after fetching a page of data I just faced a problem where I needed to know if extra data was present in a given collection after a Take operation took place. Specifically, this is related to generating @odata.nextLink values in an OData-enabled API only if there is remaining data on the server: the user should not receive a "nextLink" if no more data is available, so that he can properly rely on this value for paging purposes.

The common strategy used in these cases AFAIK is to take one more element on the target, and then check if the results "went past" the limit or not.

For example, when we want to return 10 results:

const int pageSize = 10;
var data = dataSource
    .Take(pageSize + 1)
    .ToList();

var hasRemainingData = data.Count > pageSize;

return new PageResult(
    data: data.Take(pageSize),
    nextLink: hasRemainingData ? CreateNextLink(pageSize) : null);

Now this all felt a bit convoluted to me, so I created an extension method to abstract part of the logic away:

public static (IEnumerable<T> Data, bool HasRemainingData) TakeWithRemainder<T>(this IEnumerable<T> sequence, int count)
{
    if (sequence == null)
        throw new ArgumentNullException(nameof(sequence));

    if (count < 0)
        throw new ArgumentOutOfRangeException(nameof(count));

    var data = sequence.Take(count + 1).ToArray();

    return (new ArraySegment<T>(data, 0, count), data.Length > count);
}

This allows me to clean up the original code significantly, like this:

const int pageSize = 10;
var results = dataSource.TakeWithRemainder(pageSize);

return new PageResult(
    data: results.Data,
    nextLink: results.HasRemainingData ? CreateNextLink(pageSize) : null);

I have a few problems with the extension though, and was wondering if you've got any ideas to make this better:

It materializes the collection

Not sure if there is a way to avoid this since we need to count the elements anyways, but it sounds unexpected to have a Take overload that materializes the results vs the normal one that does not. Right now it seems like I'm violating the Principle of Least Astonishment here. Should I consider an approach that does not materialize the collection, or should I rename it to something else? Other options?

.
It relies on ArraySegment to avoid unneeded iteration

The original code had 2 Take calls in it, which is kinda bad in and of itself. I decided to try using something more decent and went with ArraySegment. Is that intuitive enough to you? I found the code somewhat hard to follow with that in place. Any other options that would still allow me to avoid multiple enumeration are more than welcome.

.
It uses a value Tuple to get the results out

Would also want to see your take on this aspect. The named tuple seemed like the most straightforward way to get both the data and the boolean indicator. This of course "breaks" the fluent chain, as you can't immediately chain extra LINQ methods on top of the whole tuple (which could again be seen as a Principle of Least Astonishment violation). Should I consider something else, like a custom iterator class with an extra property that still implemented IEnumerable<T>? That would allow callers to access the boolean, but still chain more LINQ calls as needed.

At the same time, I wonder if it wouldn't be extra misleading due to the materialization of the collection.

.
HasRemainingData and TakeWithRemainder seem like poor names to me

I'm not liking these 2 names, but I'm failing to thinking of something better for them if I am to keep this approach.

You should be aware that

return (new ArraySegment<T>(data, 0, count), data.Length > count);

throws an exception if count > data.Count()

The normal Take() doesn't behave that way - it takes min(count, data.Count())

One way to go could be:

public static IEnumerable<T> TakeWithRemainder<T>(this IEnumerable<T> sequence, int count, out bool hasMoreData)
{
  bool moreData = false;

  IEnumerable<T> Iter()
  {
    int i = 0;
    foreach (T value in sequence)
    {
      if (i < count)
      {
        yield return value;
      }
      else
      {
        moreData = true;
        yield break;
      }
      i++;
    }
  }

  var result = Iter().ToList();
  hasMoreData = moreData;
  return result;
}

Another way could be:

public static IEnumerable<T> TakeWithRemainder<T>(this IEnumerable<T> sequence, int count, out bool hasMoreData)
{
  hasMoreData = sequence.Skip(count).Any();
  return sequence.Take(count);
}

Or if you want to take a sub sequence not at the start:

public static IEnumerable<T> TakeWithRemainder<T>(this IEnumerable<T> sequence, int skip, int count, out bool hasMoreData)
{
  sequence = sequence.Skip(skip);
  hasMoreData = sequence.Skip(count).Any();
  return sequence.Take(count);
}

Or if you want to take page number n of m:

public static IEnumerable<T> TakeWithRemainder<T>(this IEnumerable<T> sequence, int page, int count, out bool hasMoreData)
{
  sequence = sequence.Skip(page * count);
  hasMoreData = sequence.Skip(count).Any();
  return sequence.Take(count);
}


Finding unique values in an array My objective is to make an array of unique keys from a list of objects.

Example:

Input:

var input = [{"a":1, "b":2}, {"a":3, "c":4}, {"a":5}]

Output:

[ "a", "b", "c" ]

Current approach:

var outputObj={};
input.map( function( obj ) {
    Object.keys( obj ).map( function(key) {
        outputObj[key] = 1;
    })
});
// output obj is now { a : 1, b: 1, c: 1 }
var output = Object.keys( outputObj );

This works, but it just smells funny, partly because the map function is relying on side-effects. Is there a better way to do this?

In Java, I would create a java.util.Set and simply add all keys to it.

You could clean it up a bit by using reduce and forEach instead of map (especially as you don't need map's return value, but are just using it as an iterator)

function uniqueKeys(objects) {
  var keys = objects.reduce(function (collector, obj) {
    Object.keys(obj).forEach(function (key) {
      collector[key] = true;
    });
    return collector;
  }, {});
  return Object.keys(keys);
}

Alternatively, you could combine all the objects, and then use Object.keys on the result. Just be careful, as extend-type functions usually work on the passed objects directly (as the one below does), rather than returning a new object, so pass an empty object to use as collector:

function extend(target, source) {
  var key;
  for(key in source) {
    target[key] = source[key];
  }
  return target;
}

function uniqueKeys(objects) {
  var combined = objects.reduce(function (target, obj) {
    extend(target, obj);
    return target;
  }, {});
  return Object.keys(combined);
}

If you've got an extend function that takes an arbitrary number of arguments, you could do something like

function uniqueKeys(objects) {
  var collector = {};
  extend.apply(null, [collector].concat(objects));
  return Object.keys(collector);
}

Here are demos of each one


Insert Model object effectively using entity framwork I have a WPF MVVM Application.  I have a Customer Model which is generated by an entity framework (Database first approach) and one CustomerViewModel where I created an instance of a Customer Model and used Model members inside the ViewModel property get and set.

Here is my Customer Model:

public partial class TblCustomer
{
    public TblCustomer()
    {
        this.TblSalesInvoices = new HashSet<TblSalesInvoice>();
        this.TblSalesOrders = new HashSet<TblSalesOrder>();
    }

    public int CustomerId { get; set; }  //Primary key Identity
    public string CustomerName { get; set; }
    public string Address1 { get; set; }
    public string Address2 { get; set; }
    public string City { get; set; }
    public string State { get; set; }
    public string Country { get; set; }
    public string Phone { get; set; }
    public string Email { get; set; }

    public virtual TblUser TblUser { get; set; }
    public virtual ICollection<TblSalesInvoice> TblSalesInvoices { get; set; }
    public virtual ICollection<TblSalesOrder> TblSalesOrders { get; set; }
}

Here is my CustomerViewModel:

    private TblCustomer_customerModel;
    public TblCustomer CustomerModel
    {
        get
        {
            return _customerModel?? (_customerModel= new TblCustomer());
        }
        set { _customerModel= value; }
    }

    #region Bindable Properties

    public string CustomerName
    {
        get
        {
            return CustomerModel.CustomerName;
        }
        set
        {
            CustomerModel.CustomerName= value;
            RaisePropertyChanged("CustomerName");
        }
    }

    public string Email
    {
        get
        {
            return CustomerModel.Email;
        }
        set
        {
            CustomerModel.Email = value;
            RaisePropertyChanged("Email");
        }
    }

    private ICommand _saveCommand;
    public ICommand SaveCommand
    {
        get
        {
            if (_saveCommand == null)
            {
                _saveCommand = new RelayCommand(() => Save());
            }
            return _saveCommand;
        }
    }
   #Endregion Bindable Properties

    private void SaveCustomer()
    {
        using(var context=new R_MaizeEntities())
        {
            context.TblCustomers.Add(CustomerModel);
            context.SaveChanges();
        }
    }

Is this a correct way create a Model class instance in ViewModel class and set and get model class members inside the ViewModel Property get and set?  By doing so, does the ViewModel class have a dependency on the Model class?  Is there any other efficient way to handle this situation?

Entity

public partial class TblCustomer

The Tbl prefix belongs in the database, if it belongs anywhere at all. You have plenty of ways with Entity Framework, to map an entity type to a specific table, there's no need to carry database design mistakes into the code. Also entity types don't need to be partial.
One way could be to use annotations:
using System.ComponentModel.DataAnnotations.Schema;

[Table("TblCustomer")]
public class Customer
{
}

Another way could be to use the fluent API in the OnModelCreating method override of your DbContext:
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Customer>().ToTable("TblCustomer");
}

The most verbose way would be to create an EntityTypeConfiguration class:
public class CustomerMap : EntityTypeConfiguration<Customer>
{
    public CustomerMap()
    {
        ToTable("TblCustomer");
    }
}

And then to load that configuration into the model builder:
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Configurations.Add(new CustomerMap());
}

You'll probably want to prefer convention over configuration, so simple data annotations should do the trick.

This comment:

public int CustomerId { get; set; }  //Primary key Identity

Doesn't need to exist. Entity Framework understands [EntityName]Id as your primary key (also just Id works), by convention - the mapping is automagic.
If you prefer it to be explicit (/by configuration) you can, again, use data annotations:
[Key()]
public int CustomerId { get; set; }

..or configure it in OnModelCreating:
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Customer>().ToTable("TblCustomer");
    modelBuilder.Entity<Customer>().HasKey(e => e.CustomerId);
}

..or configure it in an EntityTypeConfiguration class:
public class CustomerMap : EntityTypeConfiguration<Customer>
{
    public CustomerMap()
    {
        HasKey(e => e.CustomerId);
        ToTable("TblCustomer");
    }
}

And then to load that configuration into the model builder:
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Configurations.Add(new CustomerMap());
}

The comment doesn't add anything new, and at worst it could be just a lie (because if you're using configurations the entity type itself doesn't know which property is a key). I'd just remove it.

ViewModel

#region Bindable Properties

Don't. Any property that is public is bindable. This comment/region can only turn into an eventual lie. Remove it.

using(var context=new R_MaizeEntities())

R_MaizeEntities is an awful name for a type. Syntax highlighting doesn't even pick it up as a C# type name, that's a sign. I don't know what the R_ stands for, and I'm pretty sure it's meaningless - the type should be named MaizeEntities or MaizeContext.
One problem you have in your ViewModel, is that you can't test it without hitting the database and actually performing a save: your ViewModel is strongly coupled with everything you're newing up in that class.
Think of it: anything the ViewModel is newing up, is a dependency. If it makes sense for the ViewModel to be tightly coupled with specific commands, being tightly coupled with a specific data context makes your code pretty much untestable.
I would either constructor-inject the context (as an abstraction), or constructor-inject a context factory (as an abstract factory) whose job would be to instantiate a data context - and unit tests can inject a stub factory that instantiates a mock context for testing purposes.

The CustomerName property should be using _customerModel - it being null wouldn't result in a NullReferenceException at runtime, only as a silent binding error that you wouldn't be getting if _customerModel was constructor-injected instead of being new'd up in the property getter.

I think there might be a bug here:
_saveCommand = new RelayCommand(() => Save());

Looks like the command should be calling the SaveCustomer() method. Or there's a Save() method somewhere else?

RaisePropertyChanged doesn't follow naming conventions: if the method does what it says it does, it should be named OnPropertyChanged.
Raising a PropertyChanged event is surely something that's common to all your ViewModel classes. Why not write a ViewModelBase abstract class, and shove the property-changing logic in there? See this post for some ideas, more specifically about referring to property names in a strongly-typed manner.


Counting the most frequently appearing character in a string (i.e. the statistical mode) This is my sample code to find maximum occurrence of characters in string in C#.

How can I make it better?

public static int GetMaximumOccurence(string str)
{
    int _maxcount = 0;

    foreach (char c in str)
    {
        int ix = str.IndexOf(c);
        int count = 0;
        while (ix!=-1)
        {
            StringBuilder _s = new StringBuilder();
            _s.Append(str);
            _s[ix] = ' ';
            str = _s.ToString();

            count++;

            ix = str.IndexOf(c);
        }

        if (count > _maxcount)
        {
            _maxcount = count;
        }
    }

    return _maxcount;
}

I'm worried this is turning into Code Golf, so I'll preface my alternative with some actual review.

There is a pretty big bug in your code, as the following will not terminate:

Console.WriteLine(GetMaximumOccurence(" "));

Can you see why?

The basic structure of your code is good. That is, the part that effectively goes

var maxCount = 0;
foreach (var c in input)
{
    // TODO: Count occurrences of c in input.
    maxCount = Math.Max(count, maxCount);
}

return maxCount;

Now how to we complete that TODO? Using IndexOf like you do seems like a good idea.

var maxCount = 0;
foreach (var c in input)
{
    var count = 0;
    var i = input.IndexOf(c);
    while (i != -1)
    {
        // TODO: Keep counting occurrences of c.
    }

    maxCount = Math.Max(count, maxCount);
}

return maxCount;

Now, string has an overload of IndexOf that takes a starting position. But where do we start searching? We start searching one position after our last found occurrence.

var maxCount = 0;
foreach (var c in input)
{
    var count = 0;
    var i = input.IndexOf(c);
    while (i != -1)
    {
        count++;
        i = input.IndexOf(c, i + 1);
    }

    maxCount = Math.Max(count, maxCount);
}

return maxCount;

If you prefer, this can be written as a for-loop.

var maxCount = 0;
foreach (var c in input)
{
    var count = 0;
    for (var i = input.IndexOf(c); i != -1; i = input.IndexOf(c, i + 1))
    {
        count++;
    }

    maxCount = Math.Max(count, maxCount);
}

return maxCount;

However, this solution is not terribly efficient. For each character in the string, we're iterating over the string again with our calls to IndexOf, so our time complexity is \$O(n^2)\$.

In fact, we've basically just written this:

var maxCount = 0;
foreach (var c in input)
{
    var count = 0;
    foreach (var d in input)
    {
        if (c == d)
        {
            count++;
        }
    }

    maxCount = Math.Max(count, maxCount);
}

return maxCount;

@BlueTrin's answer shows how you can trade space to reduce the time complexity to \$O(n)\$, so I won't go over that here.

All that said, there is an elegant way to express this using LINQ:

return input.Length == 0
    ? 0
    : input.GroupBy(c => c).Max(group => group.Count());


Here are a few suggestions:

1. Don't write this

Many before you have needed, and gotten, a fast Base64 decoder. Why should you write this yourself? You don't seem to have a special twist on the decoding/encoding - unless you want "base85 decoding", a term I'm not quite sure I understand.

Specifically, here is a nice treatment of the subject (although still far from the optimum):

Fast Base64 encode and decode

(and see also the suggestion in the comments there - by now only available on archive.org)

2. Don't work on 8-bit values

The fact that base64 encoding has a 1-byte (or 1-ASCII-character) resolution the way it's defined does not mean that's how you need to decode it. Your machine is (probably) a 64-bit machine. So work on in 64-bits units unless you have reason to use another size (which you very well might - but probably not 8 bits). Work out the math for the larger sizes.

Now you might say "but what if the length of my output isn't divisible by 8?" - well, just handle the slack at the end, outside your main tight loop.

Similarly, you shouldn't use 4 distinct 8-bit variables for your input - use something bigger.

3. Don't mix decoding and allocation + don't force the use of std::vector

You should not be using an std::vector. Take a pointer to a nicely-aligned output buffer, or better yet - a gsl::span. (You could also take a templated iterator but I wouldn't bother) with sufficient memory pre-allocated, and write there.

If you insist on doing the allocation yourself for some reason, have a wrapper function do it. Right now you're allocating \$log(n)\$ times for decoding \$\frac{n}{6}\$ bits! C-R-A-Z-Y!.

At any rate, there's no reason to force the client code for your decoder to take an std::vector.

4. Are both lookup tables really helping you?

Using a look-up table means doing less computational work. But is the trade-off worth it? Not always. Now, if you be making your lookup table larger (which you may very well do, see the linked blog post and remember we want to work with more than 8 bits at a time) you might hit cache limits. And even if you aren't - maybe in-register arithmetic is good enough?

Now, I might be worrying you for nothing here, and maybe lookup tables are the best thing to do - but at least profile to make sure. In fact,

5. Profile profile profile

If @DJuracu wrote already - profile to find out what needs optimization. You can speed up something a zillion times, but if it's only 5% of the execution time you haven't saved much (Amdahl's law). Of course, after you've improved something else, those 5% may now become 50%, making it worthwhile to work on that part of the code after all. Still, you need to know what worked and what didn't.

6. You have to look at the compiled code

When you're writing code that's this low-level, you can't allow yourself the slack of ignoring the assembly. Learn to read it and do so. I know that's a tall order but you're the one who said you want to write something that's "fast"...

7. Multi-thread

If this is part of an application which already has multiple threads running, then forget this point. Otherwise - why use only one core, and with one thread on it? Assuming you're on a typical modern PC, you have 2 or 4 cores, and with 'HyperThreading', they can execute instructions from 4 or 8 threads overall. While you could theoretically be bottlenecked on memory bandwidth or some other resource at full 4 or 8 thread capacity, you might not. Especially if you're not well-enough SIMDized

8. Use SIMD for parallelism

Your processor supports vectorized instructions. Your decoding (and your decoding) does not have "dependency chains" between the values of consecutive characters, except at most when composing a larger word from fewer bits. So - you should be able to SIMDize all or almost all of this work. I'm a GPU guy more than a CPU guy, so I can't tell you which instructions you'll want to be using, but that would definitely be necessary for maximum speed. Probably best to work on this before going multi-threaded.


If ComparisonIsTrue(thisPost, ComparisonOperator.NotEqualTo, goodCode) Then Me.Answer This is a follow-on to a global enum for comparison operators. I wanted to pass a logical expression E.G. >= someValue as a parameter to a function, so I made an Enum and an accompanying function to analyse logical expressions.

Is my approach a good way to implement this functionality in VBA?

Public Enum ComparisonOperator
    NotEqualTo = 0
    LessThan = 1
    LessThanOrEqualTo = 2
    EqualTo = 3
    GreaterThanOrEqualTo = 4
    GreaterThan = 5
End Enum

Public Function ComparisonIsTrue(ByVal sourceValue As Variant, ByVal operator As ComparisonOperator, ByVal comparisonValue As Variant) As Boolean

    If IsNull(sourceValue) Or IsNull(comparisonValue) Then GoTo CleanFail
    If IsObject(sourceValue) Or IsObject(comparisonValue) Then GoTo CleanFail

    Dim isTrue As Boolean

    Select Case operator

        Case NotEqualTo
        isTrue = (sourceValue <> comparisonValue)

        Case LessThan
        isTrue = (sourceValue < comparisonValue)

        Case LessThanOrEqualTo
        isTrue = (sourceValue <= comparisonValue)

        Case EqualTo
        isTrue = (sourceValue = comparisonValue)

        Case GreaterThanOrEqualTo
        isTrue = (sourceValue >= comparisonValue)

        Case GreaterThan
        isTrue = (sourceValue > comparisonValue)

        Case Else
        '/ Error Handling
        Stop

    End Select

CleanExit:
    ComparisonIsTrue = isTrue
    Exit Function

CleanFail:
    isTrue = False
    GoTo CleanExit

End Function

I really don't like your GoTos...

If IsNull(sourceValue) Or IsNull(comparisonValue)
Or IsObject(sourceValue) Or IsObject(comparisonValue) Then
    ComparisonIsTrue = False
    Exit Function
End If

And gone are your gotos.


Setting defaults in a Ruby options hash I have a specific method that I like because it lets me decide whether or not I want to use the default. If I want anything different I enter in :option => value otherwise I get the default. Here's a concrete example that works, but is a little ugly.

How can I accomplish the same thing in a more elegant manner?

def connect_to_oracle opts = {}
  host_name = opts[:host_name]
  host_name ||= 'a_default_host_name'
  db_name = opts[:db_name]
  db_name ||= 'a_default_db_name'
  userid = opts[:userid]
  userid ||= 'a_default_userid'
  password = opts[:password]
  password ||= 'a_default_password'
  url = "jdbc:oracle:thin:#{userid}/#{password}@#{host_name}:1521:#{db_name}"
  $db = Sequel.connect(url)
end

You don't need the ||= you can use ||:

def connect_to_oracle( opts = {} )
  host_name = opts[:host_name] ||'a_default_host_name'
  db_name = opts[:db_name] || 'a_default_db_name'
  userid = opts[:userid] || 'a_default_userid'
  password = opts[:password] ||'a_default_password'

  url = "jdbc:oracle:thin:#{userid}/#{password}@#{host_name}:1521:#{db_name}"
  $db = Sequel.connect(url)
end

Another approach is Hash.merge:

DEFAULT = {
  host_name: 'a_default_host_name',
  db_name:  'a_default_db_name',
  userid: 'a_default_userid',
  password: 'a_default_password',
}

def connect_to_oracle( opts = {} )
  opts = DEFAULT.merge(opts)

  host_name = opts[:host_name]
  db_name = opts[:db_name]
  userid = opts[:userid]
  password = opts[:password]

  url = "jdbc:oracle:thin:#{userid}/#{password}@#{host_name}:1521:#{db_name}"
  $db = Sequel.connect(url)
end

or:

DEFAULT = {
  host_name: 'a_default_host_name',
  db_name:  'a_default_db_name',
  userid: 'a_default_userid',
  password: 'a_default_password',
}

def connect_to_oracle( interface_opts = {} )
  opts = DEFAULT.merge(interface_opts )

  url = "jdbc:oracle:thin:%s/%s@%s:1521:%s" % [
    opts[:userid],
    opts[:password],
    opts[:host_name],
    opts[:db_name],
    ]
  $db = Sequel.connect(url)
end
connect_to_oracle()
connect_to_oracle(:host_name => :xxxxxxxx)

I prefer the version with merge. So I get a constant with all default parameters in my documentation.

Another advantage: I can easily add checks if my interface contains correct keys.

Example:

DEFAULT = {
  host_name: 'a_default_host_name',
  db_name:  'a_default_db_name',
  userid: 'a_default_userid',
  password: 'a_default_password',
}

def connect_to_oracle( myopts = {} )
  (myopts.keys - DEFAULT.keys).each{|key|
    puts "Undefined key #{key.inspect}"
  }
  opts = DEFAULT.merge(myopts)

  url = "jdbc:oracle:thin:%s/%s@%s:1521:%s" % [
    opts[:userid],
    opts[:password],
    opts[:host_name],
    opts[:db_name],
    ]
  #~ $db = Sequel.connect(url)
end
connect_to_oracle(:host_nam => :xxxxxxxx)

My method call contains an error (forgotten 'e'), but when you call it, you get a warning Undefined key host_nam. This often helps to detect errors. (in a real scenario I replace the puts with a logger-warning/error).


jquery checking nested input with repetitive code I have a table with 'nested' inputs that need to be checked or unchecked when a button 'select all' is pressed.  There are several levels that can be toggled with their respective 'select all' button.  In my code I have a repetitive .each() statement where the second one tests if there are differing numbers of checked/unchecked inputs and if there are it overwrites what the first .each() statement did.  How can I write the following code better?

$(document).ready(function(){
    $(".checkAll").click(function(e){
        e.preventDefault();
        var thisRow = $(this).parents("tr");
        var thisRowNumber = parseInt(thisRow.attr("id").replace("row",""));
        var thisLevel = parseInt(thisRow.children().first().attr("class").replace("textlevel",""));
        var checked = 0;
        var unchecked = 0;
        $(".tablefull tr").each(function(){
            if($(this).attr("class") !== "heading"){
                r = parseInt($(this).attr("id").replace("row",""));
                if(r > thisRowNumber){
                    n = parseInt($(this).children().first().attr("class").replace("textlevel",""));
                    if(n === thisLevel){
                        return false;
                    }
                    if(n > thisLevel){
                        $(this).find("input[name='ci_id']").prop("checked",function(index, oldAttr){
                            if(oldAttr){
                                checked++;
                            }
                            else {
                                unchecked++;
                            }
                            return !oldAttr;
                        });
                    }
                }
            }
        });
        if(checked > 0 && unchecked > 0){
            $(".tablefull tr").each(function(){
                if($(this).attr("class") !== "heading"){
                    r = parseInt($(this).attr("id").replace("row",""));
                    if(r > thisRowNumber){
                        n = parseInt($(this).children().first().attr("class").replace("textlevel",""));
                        if(n === thisLevel){
                            return false;
                        }
                        if(n > thisLevel){
                            $(this).find("input[name='ci_id']").prop("checked",true);
                        }
                    }
                }
            });
        }
    });
});

here's the working code in a fiddle: http://jsfiddle.net/KnU4X/1/

here's the code without my lengthy fix in a fiddle: http://jsfiddle.net/KnU4X/2/

To see the problem, click 'select all' with level 1, then 'select all' with level 2, then 'select all' with level 1 again. You will notice it unchecks everything except the level 2 which becomes checked.

Is there a way to remove all that seemingly redundant code and keep that functionality?

In general, you would have an easier time doing this if your "nested rows" were actually nested. However - that leads to a variety of other layout problems, etc.

That said - here is a simpler way to accomplish what I think you're looking for:

jQuery code:

$(".checkAll").click(function (e) {
    e.preventDefault();

    // Find the row whose button was clicked.
    var $row = $(this).closest('tr');

    // Capture the associated "level" class.
    var cls = $row.children('td:first-child').attr('class');

    // Create a collection of rows, in which you can store the child-rows
    var $children = $row;

    // Start with the next row, and...
    var $current = $row.next();

    // ... for every subsequent row which is *not* on the same level as the
    // one you started with...
    while ($current.length && ($current.children('td:first-child').attr('class') !== cls)) {
        // Add the row to the collection.
        $children = $children.add($current);
        // Then move on to the next row.
        $current = $current.next();
    }

    // Capture all of the *checkboxes* within the matching rows
    var $checks = $($children).find('input[type="checkbox"]');

    // If ALL checkboxes are checked, uncheck them.
    // Otherwise, check them all.
    if ($checks.length == $checks.filter(':checked').length)
        $checks.prop('checked', false);
    else
        $checks.prop('checked', true);
});

Here is a working jsFiddle showing the code in-action.


Generating complementary DNA sequence I am still relatively new to Python but I have been actively doing sample examples to try and get used to problem solving aspect of programming.

With that said, I stumbled upon a question that asks you to write a code to convert DNA strand to its complementary form.

What I have written works, but I don't believe that my approach is efficient enough. Could you please review my code and guide me as to how I might improve on it? I feel like there will be a faster way than writing multiple if, elif statements.

import sys

def check_input(user_input):
    if len(user_input) == 0:
        sys.exit("No strand provided.")
    return user_input

def complementary(strand):
    complementary_strand = ''
    for dna in strand:
        if dna == 'A':
            complementary_strand += 'T'
        elif dna == 'a':
            complementary_strand += 't'
        elif dna == 'T':
            complementary_strand += 'A'
        elif dna == 't':
            complementary_strand += 'a'
        elif dna == 'G':
            complementary_strand += 'C'
        elif dna == 'g':
            complementary_strand += 'c'
        elif dna == 'C':
            complementary_strand += 'G'
        elif dna == 'c':
            complementary_strand += 'g'
        else:
            complementary_strand += 'x'
    return complementary_strand

if __name__ == "__main__":
    user_input = input("Enter strand: ")
    print()
    strand = check_input(user_input)
    complementary_strand = complementary(strand)
    print("Complementary strand is {}".format(complementary_strand))

Typically, a character-for-character replacement is best done using str.translate():

strand.translate(str.maketrans('AaTtGgCc', 'TtAaCcGg'))

However, your situation is trickier, because you also want to map all invalid characters to 'x'.  To achieve that, you would need to read the str.maketrans() documentation carefully, and figure out that you can wrap the mapping in a collections.defaultdict.

from collections import defaultdict

DNA_COMPLEMENTS = defaultdict(lambda: 'x', str.maketrans('AaTtGgCc', 'TtAaCcGg'))

def complementary(strand):
    return strand.translate(DNA_COMPLEMENTS)

Note that lambda: 'x' is a way to make a function that, when called with no parameters, returns the letter 'x'.


Eight queens with Java The following code works great but take too much time. placeQueens requires much time too.  The program takes 5-10 seconds.

public class EightQueen {

    public static void startSimulation(){

        long startTime = System.currentTimeMillis();
        char[] board; // Create an array

        // Repeat while queens are attacking
        do {
            // Generate a board
            board = getNewBoard();

            // Place eight queens
            placeQueens(board);

        } while (isAttacking(board));

        // Display solution
        print(board);
        long endTime = System.currentTimeMillis();
        System.out.println(endTime - startTime);
    }

    /** placeQueens randomly places eight queens on the board*/
    public static void placeQueens(char[] board) {
        int location;
        for (int i = 0; i < 8; i++) {
            do {
                location = placeQueens();
            } while (isOccupied(board[location]));
            board[location] = 'Q';
        }
    }

    /** placeQueens randomly places one queen on the board */
    public static int placeQueens() {
        return (int)(Math.random() * 64);
    }

    /** isAttacking returns true if two queens are attacking each other */
    public static boolean isAttacking(char[] board) {
        return isSameRow(board) || isSameColumn(board) ||  isSameDiagonal(board);
    }

    /** isSameRow returns true if two queens are in the same row */
    public static boolean isSameRow(char[] board) {
        int[] rows = new int[8];
        for (int i = 0; i < board.length; i++) {
            if (isOccupied(board[i])) {
                rows[getRow(i)]++;
            }
            if (rows[getRow(i)] > 1)
                return true;
        }
        return false;
    }

    /** isSameColumn returns true if two queens are in the same column */
    public static boolean isSameColumn(char[] board) {
        int[] columns = new int[8];
        for (int i = 0; i < board.length; i++) {
            if (isOccupied(board[i])) {
                columns[getColumn(i)]++;
            }
            if (columns[getColumn(i)] > 1)
                return true;
        }
        return false;
    }

    /** isSameDiagonal returns true if two queens are on the same diagonal */
    public static boolean isSameDiagonal(char[] board) {
        for (int i = 0; i < board.length; i++) {
            if (isOccupied(board[i])) {
                for (int j = 0; j < board.length; j++) {
                    if (isOccupied(board[j]) && Math.abs(getColumn(j) - getColumn(i)) ==
                            Math.abs(getRow(j) - getRow(i)) && j != i) {
                        return true;
                    }
                }
            }
        }
        return false;
    }

    /** isOccupied returns true if the element in x is the char Q */
    public static boolean isOccupied(char x) {
        return x == 'Q';
    }

    /** getNewBoard returns a char array filled with blank space */
    public static char[] getNewBoard() {
        char[] board = new char[64];
        for (int i = 0; i < board.length; i++)
            board[i] = ' ';
        return board;
    }

    /** print displays the board */
    public static void print(char[] board) {
        for (int i = 0; i < board.length; i++) {
            System.out.print(
                    "|" + ((getRow(i + 1) == 0) ? board[i] + "|\n" : board[i]));
        }
    }

    /** getRow returns the row number that corresponds to the given index */
    public static int getRow(int index) {
        return index % 8;
    }

    /** getColumn returns the column number that corresponds to the given index */
    public static int getColumn(int index) {
        return index / 8;
    }
 }

Just like bogosort will never be a fast sorting algorithm. Your "throw away the board and randomly place N new queens" solution will never really be faster than those 5 to 10 seconds.

Nevertheless it made me happy to see that it actually does find a solution somewhat consistently. And the question itself is composed fine as well, so I do think it deserves an answer.

Like CiaPan already suggested in a comment a far better way to solve the n-queens problem is with backtracking. My quick test program with this approach solves the 8-queens in 1 millisecond (or less). (And a the 20-queens in 50 ms).

However, the "reset and randomly place n new queens" approach is interesting to see, so let's add one major improvement to speed up finding a solution.

/** placeQueens randomly places eight queens on the board*/
public static void placeQueens(char[] board) {
    int location;
    for (int i = 0; i < 8; i++) {
        do {
            location = placeQueens(i);
        } while (isOccupied(board[location]));
        board[location] = 'Q';
    }
}

/** placeQueens randomly places one queen on the board */
public static int placeQueens(int row) {
    return row * 8 + (int)(Math.random() * 8);
}

This little change here got the time till solution down to under 100 ms consistently.
Why? Because this reduces the search space from O(n³) to O(n²).
The reason this works, is that in all solutions there is exactly 1 queen on each row. So I generate one randomly for each row, instead of on the entire board.


Project Euler #19: Counting Sundays in the 20th century using Pandas Project Euler #19 asks:

  How many Sundays fell on the first of the month during the twentieth century (1 Jan 1901 to 31 Dec 2000)?

I'm hoping I wasn't too off course from the spirit of the exercise by making use of my favorite libraries: pandas.

import pandas as pd

rng = pd.date_range(start='1/1/1901', end='12/31/2000', freq='D')
count = 0
for date in rng:
    if date.day == 1 and date.weekday() == 6:
        count += 1
count

It's a rather simple problem and a rather simple solution. I just made use of the date_range function that pandas has builtin that works well with datetime objects in python.

While I don't think my usual questions apply. Some specific questions:

Is it pythonic to run the for loop as such, or would a list comprehension surrounded by a len be more pythonic, e.g. len([x for x in rng if date.day == 1 and date.weekday() == 6])? Or is something entirely else even more pythonic?
Is there a way to avoid iterating over an object as large as rng is with it's 30,000+ items to avoid memory usage? If so what would be a preferred method (just pseudo-code or however you prefer to explain.)
As my attention has been brought to how powerful itertools is when improving performance and substituting lists for generators, I'm wondering how I would improve upon the below code with itertools if there is any such a way.

It is good to use a library rather than re-inventing everything yourself. Just be sure to avoid explicit looping in Python:

sum(date.day == 1 and date.weekday() == 6 for date in rng)

The above sums the number of times that date.day == 1 and date.weekday() == 6 automatically, with no loops of counters, It should also be more efficient (sum is implemented in C)


Update URL with new parameter value I needed a way to update a parameter in the URL, or add it if it doesn't exist, but keep any other variables the same value.  I built this function to do the task, and while it works, I feel like it's taking longer than it should.  Does anyone have any suggestion on what I could change or a faster method?

function changeURLParameter(sVariable, sNewValue)
{
    var aURLParams = [];
    var aParts;
    var aParams = (window.location.search).substring(1, (window.location.search).length).split('&');

    for (var i = 0; i < aParams.length; i++)
    {
        aParts = aParams[i].split('=');
        aURLParams[aParts[0]] = aParts[1];
    }

    if (aURLParams[sVariable] != sNewValue)
    {
        if (sNewValue.toUpperCase() == "ALL")
            aURLParams[sVariable] = null;
        else
            aURLParams[sVariable] = sNewValue;

        var sNewURL = window.location.origin + window.location.pathname;
        var bFirst = true;

        for (var sKey in aURLParams)
        {
            if (aURLParams[sKey])
            {
                if (bFirst)
                {
                    sNewURL += "?" + sKey + "=" + aURLParams[sKey];
                    bFirst = false;
                }
                else
                    sNewURL += "&" + sKey + "=" + aURLParams[sKey];
            }
        }

        return sNewURL;
    }
}

I'm not sure about ways to improve performance, but I did notice a couple things.

Guaranteed Returns

If the specified key is already set to the specified value, your function does not return anything. Maybe this is the expected behavior, but I could see it being a problem if you used the function to directly update the URL.

window.location.href = changeURLParameter("thriggle","boring");
// If thriggle already == boring, this redirects you to /undefined

One way to fix that would be to simply remove the If statement if (aURLParams[sVariable] != sNewValue){...}.

Without that check, regardless of whether the parameter is already set to the specified value, it'll be overwritten with the desired value and the URL string will be returned.

Original Sin

window.location.origin does not exist in older versions of Internet Explorer (9 and down). If your user base doesn't overlap with those poor souls still using IE9, feel free to ignore this one, but otherwise you could do something like this:

var sOrigin = window.location.origin ? window.location.origin : window.location.protocol + "//" + window.location.host;
sNewUrl = sOrigin + window.location.pathname;

Edit: even better, as you noted in your followup comment, just update window.location.search directly and you won't have to deal with window.location.origin!


Scraping HTML using Beautiful Soup I have written a script using Beautiful Soup to scrape some HTML and do some stuff and produce HTML back. However, I am not convinced with my code and I am looking for some improvements.

Structure of my source HTML file:

<!DOCTYPE html>

...

...

                    Some Heading 1

                    This section can have p, img, or even div tags

...
...

                    Some Heading

                    This section can have p, img, or even div tags

...

My code:

import re
from bs4 import BeautifulSoup
soup = BeautifulSoup(myhtml)
all_sections = soup.find_all('section',id=re.compile("article-section-[0-9]"))
for section in all_sections:
    heading = str(section.find_all('div',class_="heading")[0].text).strip()
    contents_list = section.find_all('div',class_="content")[0].contents
    content = ''
    for i in contents_list:
        if i != '\n':
            content = content+str(i)
    print '
'+heading+'

'+content+''

My code works perfectly without any issues so far, however, I don't find it pythonic. I believe that it could be done in a much better/simpler way.

Content_list is a list which has items like '\n'. With a loop running over this list, I am removing it.  Is there any better way?
I am not interested in article icon, so I am ignoring it in my script.
I am using strip method to remove extra white spaces in the heading.  Is there any better way?
Other than new lines, the div element within content can have anything, even nested divs. So far, I have run my script over a few pages I have and it seems to work. Anything here I need to take care of?
Lastly, is there any better way to generate HTML files? Once I scraped data, I will work on generating HTML files. These files will have same structure (CSS, JavaScript, etc) and I have to do is put scraped data into it. Can the above method I used (build a string and put content and headings) be improved in any way?

I am not looking for full code in answers; just give me subtle hints or point me in some direction.

regarding 1 you can:

new_content = [c for c in old_content if c != '\n']

or simply

new_content = old_content.replace('\n', '')

Regarding 5, if your are generating anything that is nontrivial, then it will pay off to learn some template engines like Jinja2. If that is too much, then you can make a simple template in text file and use regex or even replace() to substitute generic parts:

# template

%FOO%

%BAR%

and on the python side:

values = {"%FOO%": "the foos", "%BAR%": "the bars"}
template = open('template').read()
for k, v in values.iteritems():
    template = template.replace(k, v)
print template


Processing a list to build an ip in a string format The point of this code is to build an ip from a list of type ushort.

var ip = new StringBuilder();
List<ushort> ipList = new List<ushort>(4) {192, 168, 1, 1};

ipList.ToList().ForEach(x => ip.Append(x + "."));

return ip.Remove(ip.Length - 1, 1).ToString();

The code works and outputs an ip as expected, but the formatting I give it leaves to be desired, having to delete the last element of the string does not look like a reliable solution or at least, I don't feel like it is.

The code above would output, before returning, the following string:

"192.168.1.1."

And after removing the last character it will look like this:

"192.168.1.1"

There is a string.Join() method which would exactly do what you want like so

string ip = string.Join(".", ipList);

btw,  you don't need to call ToList() on a List<T>.


Code to test four conditions if C# First I have two conditions on the number of list elements,then in one of these conditions I have two others conditons to treat it :

My code:

List<StringBuilder> result = ReadResult_WS_Import(NameFile);
 if ((result != null) && (result.Any()))
                {
                    var is_No_Errors_Found_Exist = result[0].ToString().Contains(Properties.Resources.ARGS_RESULT_Is_No_Errors_Found_Exist);
                    var is_Import_Failed_Exist = result[0].ToString().Contains(Properties.Resources.ARGS_RESULT_Is_Import_Failed_Exist);
                    var is_The_Import_Has_Failed_Exist = result[0].ToString().Contains(Properties.Resources.ARGS_RESULT_Is_The_Import_has_Failed_Exist);

                    string[] result_LOG = result[0].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
                    if ((result.Count == 2) && (!is_The_Import_Has_Failed_Exist))
                    {
                        result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
                        StatusResult = Properties.Resources.ARGS_RESULT_OK_REJECT;
                    }
                    else if (is_No_Errors_Found_Exist)
                    {
                        StatusResult = Properties.Resources.ARGS_RESULT_OK;
                    }
                    else if (is_Import_Failed_Exist)
                    {
                        StatusResult = Properties.Resources.ARGS_RESULT_KO;
                    }

                    if ((is_The_Import_Has_Failed_Exist) && (result.Count == 2))
                    {
                        result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
                        StatusResult = Properties.Resources.ARGS_RESULT_KO_REJECT;
                    }

                    returnObject = new ReturnObject
                    {
                        CountLines = CountLines,
                        Status = StatusResult,
                        LOG = result_LOG,
                        REJECT = result_REJECT
                    };
                }

It works well, I don't have any problems,
but I want to know if there is other code organization ? or any feedback ?

Naming and underscores

Underscores help to separate long sentences. But variable names should not be long sentences. You should drop the underscores, stick with camelCasing (as per convention), and shorten the names.

bool errorsFound;
bool importFailed;

The underscores in your constants are not as much of an issue. I have generally moved away from the SCREAMCASING for constants and instead just PascalCase them (without underscores); but that's not a globally agreed upon standard.

As an aside, unit and integration test methods often do have long sentences for names, and underscores are actually useful there. So you're definitely allowed to use underscores where appropriate. Local variables with long names are hardly ever appropriate.

Simplifying the logic

if ((result.Count == 2) && (!is_The_Import_Has_Failed_Exist))
{
    result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
    StatusResult = Properties.Resources.ARGS_RESULT_OK_REJECT;
}

if ((is_The_Import_Has_Failed_Exist) && (result.Count == 2))
{
    result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
    StatusResult = Properties.Resources.ARGS_RESULT_KO_REJECT;
}

These two evaluations share some commonalities, and their bodies are identical (assuming OK/KO is a typo). This can be merged into a single evaluation:

if(result.Count() == 2 && (!is_The_Import_Has_Failed_Exist || is_The_Import_Has_Failed_Exist) )
{
    result_REJECT = result[1].ToString().Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
    StatusResult = Properties.Resources.ARGS_RESULT_OK_REJECT;
}

Clear naming

var is_Import_Failed_Exist = ...;
var is_The_Import_Has_Failed_Exist = ...;

For the life of me, I cannot figure out what the difference between them is. The naming of these variables (and the constants after which they are named) is not good, since a reader cant figure out what they represent.

returnObject = new ReturnObject { ... }

ReturnObject is such a vague name that it fails to express anything meaningful. I'll happily admit that I've struggled coming up with names for return DTOs too, but it needs to have some sort of descriptive name, e.g. ImportStatusDto or ImportStatusResult.

Properties.Resources.ARGS_RESULT_OK_REJECT

"OK" and "reject" seemingly contradict one another. Is this a good outcome, or a bad one? It's unclear and needs better naming to make this clear.

else if (is_No_Errors_Found_Exist)
{
    StatusResult = Properties.Resources.ARGS_RESULT_OK;
}
else if (is_Import_Failed_Exist)
{
    StatusResult = Properties.Resources.ARGS_RESULT_OK; // I assume "KO" was a typo
}

As a developer reading your code, it's very confusing why the failure of an import is considered a positive ("OK") outcome. This seemingly contradicts the evaluation above where the existence of errors is clearly not OK.

This may be a bug in your code. In that case, you probably would've spotted the bug easier if your variable names had been easier to read (see the above tips).

If this isn't a bug, then I'm still inclined to conclude that your naming introduces confusion where none should have existed.

CountLines

This sounds like a command or instruction ("Hey, you! Count those lines!"), which is how methods should be named.

Variable names (except for booleans) should be nouns or noun phrases. LineCount is the better name here.

For completeness' sake, boolean names should generally adhere to yes/no questions: isAlive, hasFood, ... or in other cases phrasings that strongly convey a binary result: userWasDeleted, importFailed, ...
In all of these cases, if I were to put a question mark behind the name, it would be bad English but you would understand the question and would easily identify what the meaning of "yes" and "no" is.

Avoid negative variable names

is_No_Errors_Found_Exist

While you can read it now, this can quickly get out of hand. How would you check that errors were found?

if(!is_No_Errors_Found_Exist)

That's a double negative, and really hard to parse. It's much better to stick with "positive" naming and then negate the boolean value

bool errorsFound = ...;

if(!errorsFound) { ... }

if(errorsFound) { ... }

In this example, there's no need to use a double negative, which improves readability.

Your internal voice should read this as "not errors found", which it intuitive and clearly expresses the intention of the evaluation.

Note that when I say "positive", I mean "not negated" instead of "good". A boolean called isFailed is a "positive" naming. Not because failure is good (it clearly isn't), but because it's not negated.
importDidNotFail would be the "negative" naming (because of the "not"), which you should try to avoid to enhance overall readability.

In conclusion

This is more of a review on your question than the code, but there is a common thread here.

  First I have two conditions on the number of list elements,then in one of these conditions I have two others conditons to treat it

Try to read this from the perspective of a StackExchange user who knows nothing about you or your code. Does this explain what your code is trying to achieve?

I think you're explaining things the way they already make sense to you, while not considering how others will (or won't) understand what you write.

I mention this because code readability should be written from the perspective of those who don't know the code. Whenever you name something (a variable, a method, ...) you need to ask yourself a simple question:

  Would a developer who is new to the project understand what this method does/what this variable expresses?

This is by far the biggest issue in your code. Your naming has made it very hard for anyone else to read your code and understand its purpose.

And it's not just about other people. If you start working on another project, and in a few months' time you have to return to this project, you're going to be scratching your head too, because you won't remember all the things you currently know and will have to build up that working knowledge again.

The other minor issues/bugs I've found are actually fairly common in codebases where readability is hampered, because low readability leads to a high likelyhood of making unintentional mistakes.

I suspect that if you work on improving your code readability, you'll automatically start seeing how to improve parts of the code more and more.


Day finder program, inputs date, outputs day of the week I have made this simple program that computes the day the week for the years 1700 - 2399. I am looking for suggestions/ improvement advice. I am not well versed with OOP and would like suggestions on making this program more compliant with its principles.
(I do know that this can be done after importing packages but wanted to do it manually for practice).

based on: http://blog.artofmemory.com/how-to-calculate-the-day-of-the-week-4203.html

import java.util.Scanner;

public class DayFinderLaunch {

    public static void main(String[] args) {

        DayFinderLaunch ob = new DayFinderLaunch();
        ob.accepter();

    }

    public void accepter() {

        Scanner sc = new Scanner(System.in);
        System.out.println("Enter the day, the month and year of concern in that order, all in numbers. \nNote: Year must be within 18th and 24th Century.\n");
        int d = sc.nextInt();
        int m = sc.nextInt() - 1;
        int y = sc.nextInt();

        DayFinder ob1 = new DayFinder();
        ob1.validCheck(d, m, y);
        sc.close();
    }
}

public class DayFinder {

    DayFinderLaunch ob2 = new DayFinderLaunch();

    public boolean isLeap(int y) {

        boolean leap = false;

        if (y % 4 == 0 && y % 100 != 0) {
            leap = true;
        }

        else if (y % 100 == 0 && y % 400 == 0) {
            leap = true;
        }

        return leap;
    }

    public void compute(int d, int m, int y) {

        int yc = ((y % 100) / 4 + (y % 100)) % 7;

        int[] mArr = { 0, 3, 3, 6, 1, 4, 6, 2, 5, 0, 3, 5 };

        String[] dArr = { "Sunday", "Monday", "Tuesday", "Wednesday",
            "Thursday", "Friday", "Saturday" };

        int mc = mArr[m];

        int cc;

        int[] cArr = { 4, 2, 0, 6, 4, 2, 0 };
        cc = cArr[y / 100 - 17];

        int sum = yc + mc + cc + d;

        if (isLeap(y) && m < 2) {

            sum = sum - 1;
        }

        int fcode = sum % 7;

        System.out.printf("\nThe day of the week on %d/%d/%d : %s", d, m + 1,
                y, dArr[fcode]);
    }

    public void validCheck(int d, int m, int y) {

        int[] mDays = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };

        if (isLeap(y)) {
            mDays[1] = 29;
        }

        if (m > 11 || m < 0 || d > mDays[m] || d < 1 || y < 1700 || y > 2399) {

            System.out.println("Invalid date, please try again.\n");
            ob2.accepter();
        }

        else {
            compute(d, m, y);
        }
    }
}

Declaring a new DayFinderLaunch inside DayFinder is bad (and wrong): DayFinder must find the day and nothing more (another error here: DayFinderLaunch calls DayFinder that calls DayFinderLaunch that calls DayFinder (and so on...)

compute is the main method of DayFinder, I'd rename it to findDay, getDayOfWeek or similar. You're calling it from a validation method, wrong!

Moreover, this method MUST NOT print to the console the result. A better approch is return the name of the day so the developer can choose how to manage it (Console log? File log? Message Box?)

The date validation should be done inside getDayOfWeek(). If validation fails, an exception should be throw: DayFinder MUST NOT ask for user input again, it's not its job!

...
final String dayOfWeek = getDayOfWeek(2017, 5, 30);

System.out.println("Day: " + dayOfWeek);
...
fileWriter.writeLine("Day: " + dayOfWeek);
...

public String getDayOfWeek(final int year, final int month, final int day) {
    if (!isDateValid(year, month, day)) {
        throw new InvalidDateException(String.format("Date %d/%d/%d is not valid", year, month, day));
    }

    String dayOfWeek;

    ...

    return dayOfWeek;
}

I don't understand what mArr, cArr, yc, mc etc. are, you should use significative names.
mArr, cArr and mDays could be extracted from methods and declared as constants in DayFinder
dArr could become a Day enum.

Example:

public enum Day {
    SUNDAY("Sunday")
    MONDAY("Monday"),
    ...
    ...
    SATURDAY("Saturday");

    private final String name;

    private Day(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }
}

so you can retrieve the day name using the following:

String dayOfWeek = Day.values()[index].getName(); // index must be valid of course!

isLeap should be moved in a DateUtils class:

public final class DateUtils {
    public static boolean isLeap(int year) {
        return ((y % 4 == 0) && (y % 100 != 0))
            || ((y % 100 == 0) && (y % 400 == 0));
    }
}

I didn't check if your algorithm is correct.


Compute binary XOR of all integers in a range, mod 2 here I am taking 3 inputs. 1st input will take test cases and the other two input will take starting and ending range. the program is running fine as expected but the limit of this question for compilation is 1 sec, and my code is taking 5.01 sec.

How can I make it more efficient so that I can submit the code?

  The challenge was to take number of test cases.

  Each test cases show take 2 input (starting and ending range) Eg: 1  4
  (i.e 1,2,3,4)

  Do the bitwise XOR operation for all of them (i.e  1^2^3^4)

  Which when
  you perform will be equal to 4

  Now just check if it is even or odd and print the same.

Here's my code:

from sys import stdin, stdout
t = stdin.readline()
for i in range(int(t)):
    p = 0
    a, b = map(int,stdin.readline().split())
    for x in range(a,b+1):
        p ^= int(x)
    if p % 2 == 0:
        stdout.write(str("Even\n"))
    else:
        stdout.write(str("Odd\n"))

compiling in python 3.6

INPUT:
    4
    1 4
    2 6
    3 3
    2 3

OUTPUT:
    Even
    Even
    Odd
    Odd

Working perfectly with no issue in code.

Welcome to CR, nice challenge

A few comments about the code

Many unnecessary conversion

Python is a duck typed language, if it talks like a duck, walks like a duck... it must be a duck!

This means that

p ^= int(x)

Here x is already an int, same goes for the str conversion later
Use _ variable names for variable you don't use

  for i in range(int(t)):

Replace the i with _
You could return directly

if p % 2 == 0:
    return "Even"
else:
    return "Odd"

Instead, you could do which uses a ternary operator

return "Even" if p % 2 == 0 else "Odd"

As for the speedup

I've used this SO link to inspire me, which does a way better job of explaining this then I could ever do

In short there is a trick to get the XOR'd product of a certain range

Using the method from the link, I get a massive speedup,

For these timings: range(1, 1000)

Bitmagic:  0.023904799999999997
OP: 2.2717274

Code

# https://stackoverflow.com/questions/10670379/find-xor-of-all-numbers-in-a-given-range
def bit_magic(bound):
    magic = [bound, 1, bound + 1, 0]
    return magic[bound % 4]

def bitwise_check(lower_bound, upper_bound):
    p = bit_magic(upper_bound) ^ bit_magic(lower_bound - 1)
    return "Odd" if p & 1 else "Even"

def main():
    n = int(input("Number of testcases: "))
    for _ in range(n):
        lower_bound, upper_bound = map(int, input().split())
        print(bitwise_check(lower_bound, upper_bound))

if __name__ == '__main__':
    main()


Using Pythagoras' Theorem to Check Point Distance I want to procedurally generate several points on a graph that are suitably distant from each-other. These points are meant to represent castles that are part of towns in the world-map of my game. However, I still find these points very close or touching. How can I create a set of points that are all at least a certain distance from each-other?

Here is a picture of one of the maps I've generated. The castles are black/dark grey.

Here is the function I'm using to generate the castles. I return a set which I iterate through in another function. This adds elements of appropriate int values to a list matrix which is iterated through with Pygame to display the map.

import random as r
import math as m

def createTiles(num, xBounds, yBounds, dist, tileSet):

    """
    Creates num amount of tuple tile locations and returns them in a set.

    :param num: int
        The amount of tile locations to be returned.

    :param xBounds: tuple of 2 ints
        The first element is the minimum x-values should be.
        The second element is the maximum x-values should be.

    :param yBounds: tuple of 2 ints
        The first element is the minimum x-values should be.
        The second element is the maximum x-values should be.

    :param dist: string or tuple of 2 strings.
        Each string contains a greater or less than sign, and a number.
        This indicates whether to cap the distance between points,
        or to keep points at least a certain distance away from each-other.
        For example, "<30" would keep points within 30 tiles of each-other.
        If a tuple is used,
        it must both cap the distance and maintain a distance.

    :param tileSet: set of tuples
        Contains already existing tile locations to be checked for distance.

    :return: set of tuples
        Returns tileSet with new tile locations.
    """
    tempSet = set()
    # handle different object types of dist
    if type(dist) == tuple or type(dist) == list:
        for d in dist:
            if d[0] == "<":
                cap = int(d[1:])
            else:
                keep = int(d[1:])
    elif type(dist) == str:
        if dist[0] == "<":
            cap = int(dist[1:])
        else:
            keep = int(dist[1:])
    else:
        keep = 0
        cap = 255

    # create tile locations and check distance against the tiles in tileSet.
    for n in range(num):
        x = r.randint(xBounds[0], xBounds[1])
        y = r.randint(yBounds[0], yBounds[1])
        for t in tileSet:
            distance = m.sqrt((x - t[0]) ** 2 + (y - t[1]) ** 2)
            while distance < keep or distance > cap or (x, y) in tileSet:
                x = r.randint(xBounds[0], xBounds[1])
                y = r.randint(yBounds[0], yBounds[1])
                distance = m.sqrt((x - t[0]) ** 2 + (y - t[1]) ** 2)

        tempSet.add((x, y))

    # Combine tileSet and tempSet.
    tileSet = tileSet.union(tempSet)
    # Check distance for all tiles in tileSet:
    for t1 in tileSet:
        for t2 in tileSet:
            distance = m.sqrt((t1[0] - t2[0]) ** 2 + (t1[1] - t2[1]) ** 2)
            while distance < keep or distance > cap:
                t1 = (r.randint(xBounds[0], xBounds[1]),
                      r.randint(yBounds[0], yBounds[1]))
                distance = m.sqrt((t1[0] - t2[0]) ** 2 + (t1[1] - t2[1]) ** 2)

    return tileSet

Thanks for looking through this. If you want any more information or anything just comment.

There are several reasons why the code does not work.

Here's the body of the first loop for generating points, in pseudo-code:

Pick a random point (x, y)
for t in tileSet:
    Compute distance between (x, y) and t
    while distance is wrong:
        Pick another random point (x, y)
        Recompute distance between (x, y) and t

The problem here is that when you pick another random point (x, y), you only test it to see if it has the wrong distance from the particular point t. But if (x, y) has the wrong distance from one of the previous points in tileSet, you'll never find out because that will never be tested.
The second loop has the same problem. Again, in pseudo-code:

for t1 in tileSet:
    for t2 in tileSet:
        Compute distance between t1 and t2
        while distance is wrong:
            Pick another random point t1
            Recompute distance between t1 and t2

The new random point t1 might be the wrong distance from one of the previous points in tileSet, but that will never be tested.
Additionally, after picking the new random point t1, the code doesn't update tileSet to replace the old point with the new one.


Inventory of objects with item types and quantities I am thinking about how an inventory could be designed. The requirements for this inventory will be the following:

There will be predefined item types in the inventory.
The user can define new item types and add them to the inventory.
In the interaction with the GUI the user will be shown the item types contained in the inventory and he will click a "+" or a "-" to increase/decrease the quantity of that item.

The initial design I have come up with is the following:

Item.java

public class Item {

    private String name;
    private String itemId;
    private int quantity;

    public Item(String name, String itemId) {
        this.name=name;
        this.itemId=itemId;
        this.quantity=0;
    }

    public void add() {
        this.quantity++;
    }

    public void remove() {
        this.quantity--;
    }

    public String getName() {
        return this.name;
    }

    public String getItemId() {
        return this.itemId;
    }

    public int getQuantity() {
        return this.quantity;
    }

    public String toString() {
        String s="";
        s+=" name = "+this.name;
        s+=" , id = "+this.itemId;
        s+=" , quantity = "+this.quantity;
        return s;
    }
}

and Inventory.java :

public class Inventory {
    private ArrayList<Item> items = new ArrayList<Item>();

    public Inventory() {
        this.items.add(new Item("Book", "4877326352"));
        this.items.add(new Item("Pen", "21366363636"));
    }

    public void increaseQuantity(String itemName) {
        for (Item item : this.items) {
            if (itemName.equals(item.getName())) {
                item.add();
                return;
            }
        }
    }

    public void decreaseQuantity(String itemName) {
        for (Item item : this.items) {
            if (itemName.equals(item.getName())) {
                if (item.getQuantity()>=0) {
                    item.remove();
                }
                return;
            }
        }
    }

    public void createItem(String name, String itemId) {
        this.items.add(new Item(name,itemId));
    }

    public String toString() {
        String s="";
        s+="Inventory contains:\n";
        for (Item item:this.items) {
            s+='\t'+item.toString();
        }
        return s;
    }

    public void printInventory() {
        System.out.println(this.toString());
    }

    public static void main(String[] args) {
        Inventory inventory = new Inventory();
        inventory.printInventory();
        inventory.increaseQuantity("Book");
        inventory.increaseQuantity("Pen");
        inventory.printInventory();
        inventory.decreaseQuantity("Book");
        inventory.printInventory();
        inventory.createItem("Pencil", "92827234534");
        inventory.printInventory();
        inventory.increaseQuantity("Pencil");
        inventory.printInventory();
    }

}

Now of course there are some issues with this implementation. (e.g there are no checks for creating duplicate item types etc...), yet these issues can be solved easily.

But the thing that worries me the most is the fact that Item has quantity as a field. Is that considered bad design?

If quantity was not a field in item then I guess Inventory would have an ArrayList of items, and increaseQuantity() would invoke items.add(new Item()). But then, isn't it pointless to have duplicate items in an ArrayList when all you need to know is what kind of items you have and in what quantity?

Yes, having a quantity field in Item is bad design.
The reason is that it is the wrong abstraction. If you pick up and hold ten pebbles in your hand, the number ten is a property of your hand (because it is how many things it is holding), not the pebble which is just a pebble.
Consider if you drop all the pebbles, now the quantity of pebbles in your hand changes to 0. But if the quantity was a property of the pebble, and you dropped all the pebbles how would you model that would each of the pebbles have a quantity of 0 now?
What if you hand the pebbles over to some one else? Then you would change the quantity in respective hand, but if the quantity was part of the pebble you couldn't model this transaction at all.
Similarly the quantity should be a property of the inventory (how many of X does it contain), not of the Item which is just an item.
Somehow this makes me think of Database Normal Forms.
Simply have a Map with Item as key and an Integer quantity as value. This solves your duplicate items problem as well.
Map<Item, Integer> inv = new HasMap<>();

// add x of item ( works for new items too )
inv.put(item, inv.getOrDefault(item, 0) + x);

Note that you will need to implement equals and hashCode for Item.


Change one column data from List I have this code:

for (int i = 0; i < lst.Count(); i++)
{
    lst[i].ColumnOrder = (short)i;
}

But I was trying to find a way to do it via LINQ or something else with better performance. I'm pretty sure that there are better ways to change the sequence of a column.  I use this code in a drag & drop context.

You can use LINQ to do it, but that will end up as pretty ugly code. LINQ is used to process data and return a result, and that is not what you are doing here, so using LINQ for this would only mean that your code would produce a dummy result and have a side effect doing so.

Well, for completeness, here is some ugly code that misuses LINQ extension methods to loop through the items:

lst.Select((item, i) => item.ColumnOrder = (short)i).Last();

(Note the Last call that is used to pull the result out of the expression, so that it actually loops through the items. Even if you don't use the result, you still have to loop through the entire result to make it complete the loop.)

Your original code is just the simplest way to do it (except for the use of Count() instead of Count as svick mentioned), and is likely to give the best possible performance. There are other alternatives of course that gives almost the same performance. You can use an enumerator to loop through the items, but then you still need a counter for the column order:

int i = 0;
foreach (var item in lst) {
  item.ColumnOrder = (short)(i++);
}


Implement The Singleton Pattern for Persistency managers Swift I am creating an iOS app and I want to implement Singleton Pattern.
I have created "Singleton" LibraryAPI to act as an entry point UserManager object to get data from web API. after that I used a facade Pattern "method" to call UserManager implementation.

final class LibraryAPI {

    static let shared  =  LibraryAPI()
    private let userManager  =  UsersManager()
    private let isOnline  =  false

    private init(){

    }

    func getUsers() -> [User] {
        return userManager.getUsers()
    }
}

my questions:

if I have another Manager Class like "album" class should I use the same LibraryAPI and it will become a monolithic class and how to avoid that?
should I create a "Singleton" LibraryAPI class for each manager object like UserLibraryAPI and albumLibraryAPI?

Note: any references or articles are welcome :)

This seems to be an extremely common way of handling networking in iOS and Swift probably because it is very simple to reason about a single global singleton object for services.

I would recommend thinking carefully about using a singleton.

Why not just have a NetworkManager object that you initialize?

Even better why not separate out the services into a few logical classes? One service per class may get a bit arduous, but if you think critically you will likely be able to group services together appropriately. If you then find yourself with many Service objects, you can combine these objects with a facade pattern, that groups functionality when appropriate for a given View/ViewModel.

You will want to use classes, or structs with non static functions because when something isn't a singleton you can use dependency injection. Instead of accessing the static methods of your singleton you can inject a NetworkProvider object into the class in the constructor that uses it. This will make your code far more robust and testable. The injected object, if you created an appropriate Protocol, can be mocked.

Personally, I prefer something like Moya https://github.com/Moya/Moya/blob/master/docs/CommunityProjects.md. Its very swifty, logical, and well formatted. It also makes the relationships between services very clear. As your project gets larger, you will want to create multiple provider objects, but it provides a very concise way of writing your service layer.

Summary - Use dependency injection and normal non static structs or classes. If you have multiple services, separate them out into a separate struct/class based on similarity or simply separate out one service into a single struct or class. A library like Moya can help to structure your code to make it more readable.


Proper use of mysqli->close(); database.config.php

<?php
class Database
{

    private $host = "localhost";
    private $db_name = "trial";
    private $username = "root";
    private $password = "";
    public $conn;

    public function dbConnection()
    {

        $this->conn = null;
        try
        {
            $this->conn = new PDO("mysql:host=" . $this->host . ";dbname=" . $this->db_name, $this->username, $this->password);
            $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }
        catch(PDOException $exception)
        {
            echo "Connection error: " . $exception->getMessage();
        }

        return $this->conn;
    }
}
?>

class.php

<?php
include_once 'database.config.php';
class USER
{
    private $conn;

    public function __construct()
    {
        $database = new Database();
        $db = $database->dbConnection();
        $this->conn = $db;
    }

    public function runQuery($sql)
    {
        $stmt = $this->conn->prepare($sql);
        return $stmt;
    }
}
?>

index.php

<?php
require_once 'class.php';
$user = new USER();

$teacher_fetch = $user->runQuery("SELECT * FROM teacher WHERE Id=:_id");
$teacher_fetch->execute(array(":_id"=>$_SESSION['teacher_id']));
$fetch_teacher = $teacher_fetch->fetch(PDO::FETCH_ASSOC);

echo $fetch_teacher['Name'];
?>

As it's a best practice to use close() in last where should I use it in the index.php file?

Option 1

$teacher_fetch->execute(array(":_id"=>$_SESSION['teacher_id']));

//close connection here

$fetch_teacher = $teacher_fetch->fetch(PDO::FETCH_ASSOC);
echo $fetch_teacher['Name'];

Option 2

$teacher_fetch->execute(array(":_id"=>$_SESSION['teacher_id']));
$fetch_teacher = $teacher_fetch->fetch(PDO::FETCH_ASSOC);

//close connection here

echo $fetch_teacher['Name'];

Option 3

$teacher_fetch->execute(array(":_id"=>$_SESSION['teacher_id']));
$fetch_teacher = $teacher_fetch->fetch(PDO::FETCH_ASSOC);
echo $fetch_teacher['Name'];

//close connection here

Or Somewhere else?

Also, how should I close it?

$user->close(); OR $teacher_fetch->close(); OR $fetch_teacher->close(); OR Something else?

The fact that mysqli has nothing to do with PDO aside, there are several things about your code at whole:

this "class Database"  from some article is a dummy. It is absolutely useless and there is not a single reason to prefer it over original PDO. Let alone some flaws in the code inside.
under no circumstances you should create a new database connection in the constructors of your classes. A single existing instance should be passed as a constructor parameter instead.
runQuery() function is misplaced, misnamed and useless. It just performs PDO::prepare() which you can always call directly. It makes whole User class a dummy as well, just a useless chunk of code.
some User-related method should be written in this class instead

Given all the above let's rewrite your code

database.config.php will be a code from my article How to connect to MySQL using PDO:

<?php
$host = '127.0.0.1';
$db   = 'test';
$user = 'root';
$pass = '';
$charset = 'utf8mb4';

$options = [
    \PDO::ATTR_ERRMODE            => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES   => false,
];
$dsn = "mysql:host=$host;dbname=$db;charset=$charset";
try {
     $pdo = new PDO($dsn, $user, $pass, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

here it will give you an instance of PDO class.

user.class.php

<?php
class User
{
    private $conn;

    public function __construct(PDO $pdo)
    {
        $this->conn = $pdo;
    }
    public function findTeacher($id)
    {
        $stmt = $this->conn->prepare("SELECT * FROM teacher WHERE id=?");
        $stmt->execute([$id]);
        return $stmt->fetch();
    }
}

here you have a findTeacher() method logically placed in the user class.

index.php

<?php
require_once 'database.config.php';
require_once 'user.class.php';

$user = new User($pdo);
$teacher = $user->findTeacher($_SESSION['teacher_id']);
echo $teacher['Name'];

As you can see, this code is much more concise and logical, actually utilizing some basic OOP, as opposed to your current code which could be written (again more concisely) without any classes.

As for your question, in the average PHP script you don't have to close neither the statement nor the connection. PHP will close it for you automatically.