Comparing strings with different newlines Recently, I was burdened with the task of finding a bug. It turns out the problem was strings from different systems containing different newlines. Comparing two strings with different newlines (but same "text") still are not equal. E.g. "new\nline" (Unix flavor) and "new\r\nline" (Windows flavor) are not equal.

Since the code will be dealing with both types of newlines, I wrote a method to test for equality independent from newline type (the code treats "\n", "\r", "\r\n" and "\n\r" the same (even though "\n\r" isn't really used as newline)).

After I got the code done I would like your opinion on it. What do you think of variable names or method names (I know I could have chosen better names)? Is there a way to optimize the code or make it more readable?

public class StringUtils {
    public static final char LF = '\n';
    public static final char CR = '\r';

    public static boolean equalsIgnoreNewlineTwirks(String str, String other){
        if (str == null || other == null){
            return false;
        }
        if (str == other){
            return true;
        }

        char[] s1 = str.toCharArray();
        char[] s2 = other.toCharArray();
        int index1 = 0, index2 = 0;
        while (true){
            boolean oob1 = index1 >= s1.length, oob2 = index2 >= s2.length;
            if (oob1 | oob2){
                return oob1 & oob2;
            }

            char ch1 = s1[index1], ch2 = s2[index2];
            if (ch1 != ch2){
                if (ch1 != LF && ch1 != CR) return false;
                if (ch2 != LF && ch2 != CR) return false;

                if (index1 + 1 < s1.length && isCRAndLF(s1[index1], s1[index1 + 1])){
                    index1++;
                }
                if (index2 + 1 < s2.length && isCRAndLF(s2[index2], s2[index2 + 1])){
                    index2++;
                }
            }

            index1++; index2++;
        }
    }

    private static boolean isCRAndLF(char ch1, char ch2){
        return (ch1 == CR && ch2 == LF) || (ch1 == LF && ch2 == CR);
    }
}

The code looks quite complicated at first glance, but when reading it, it is straightforward. If you want to make the code shorter, you can just do this:

public static boolean equalsIgnoreNewlineStyle(String s1, String s2) {
    return s1 != null && s2 != null && normalizeLineEnds(s1).equals(normalizeLineEnds(s2));
}

private static String normalizeLineEnds(String s) {
    return s.replace("\r\n", "\n").replace('\r', '\n');
}

Concerning running time and GC stress, your code is probably better. Use a benchmark to see how much better it is.

The word twirks sounded negative to me, therefore I replaced it with style.

Since the two strings have equal rights and are treated the same, none of them should be called the "other one".

You should not make the constants public, since there is no need to do that.


Avoiding shell injection when calling Git in Python I'm developing a command line application in Python which processes information that users provide. I've been reading the Python documentation on this topic and found that the built-in module subprocess provides the call method, which executes a string as if it was run on a command line. The disadvantage of this approach is that it can lead to shell injection.

The solution I came up with uses a set of illegal substrings, such as rm or -rf, and then uses regex to check whether the provided string contains a substring from the set.

import re
from string import ascii_letters, punctuation

# Generate a set of characters used for string sanitization.
punctuation_minus = set(punctuation) - set(('.', '_', '-'))
substrings = set(("rm", "&&", "-f", "null", "/dev/null", "2>&1", "-rf",
              "rf"))
illegal = [re.escape(item) for item in (punctuation_minus | substrings)]
illegal_as_str = "|".join(illegal)

def sanitize_string(s):
    """Cleans a string from illegal characters."""
    # Get all legal strings.
    operations = re.split(illegal_as_str, s)
    # Remove trailing spaces
    no_trailings = [x.strip() for x in operations]
    # Remove empty strings.
    list_of_operations = filter(None, no_trailings)
    # Generate a safe string to return.
    operations_str = " ".join(list_of_operations)
    try:
        operations_str.decode("utf-8")
        return operations_str
    except UnicodeDecodeError as detail:
        print("UnicodeDecodeError: " + str(detail))
        return False

The inherent issue with this approach is that it heavily depends on the set of illegal characters I'm using. Is there any way I can sanitize a string taking into account the common attacks? And if not, how can I improve my code?

The user is expected to provide git commands, such as git add or git push. Each one of these git commands needs certain parameters that the user needs to provide. The next example shows how all the illegal substrings are removed from a user input when adding files to the staging area:

>>> git_add_str = "git add {}"
>>> files_to_add = input("Files to add as string:")
Files to add as string: "file1.py file2.py; rm -rf /"
# Clean files_to_add before modifying git_add_str.
>>> clean_str = sanitize_string(files_to_add)
>>> clean_str
'file1.py file2.py'
>>> git_add_str = git_add_str.format(clean_str)
>>> git_add_str
'git add file1.py file2.py'

This output is considered legal as it does not contain any of the illegal substrings. Now it is safe to run subprocess.call with this string and shell=True.

Your defense strategy is known as "enumerating badness".  As a rule, any strategy based on enumerating badness is doomed to fail.  For example, you prohibit rm -rf /, but have you considered find / -delete, which basically does the same thing?  How about some use of the dd if=/dev/zero?  What if a user legitimately wants to perform an operation on a file named normal.py or even rm.c?  Why are you silently altering the command to do something else?

If you ever need to execute an external program with user-supplied parameters, sensible practices would be to use subprocess.run() or subprocess.Popen()

with the command line as a list already split into words,
with the shell=False parameter (the best way to avoid shell injection is to avoid using the shell at all!),
with the command given using an absolute path (which is essential with shell=False),
with -- preceding the user-supplied filenames, so that filenames that start with - are treated literally.

Do not attempt to prohibit any "bad" words; you want your code to work for any legitimate filenames.  Do check that the filenames are legitimate, though — for example, not containing too many ../../../...

Better yet, use a Python module that takes care of interacting with Git so that you don't have to deal with the problem yourself.


Creating a pythonic snippet to read and clean .csv files I am trying to import a .csv file, clean the contents, and make sure all the errors are eliminated.

IO error
file format error
data error

I am using the snippet below.  Could someone help me clean it and make it more pythonic?

try:
    fl = request.FILES['csv']
except:
    return HttpResponse("Some IO Error")
try:
    reader = csv.reader(fl)
except:
    return HttpResponse("File Not Supported")
reader.next() # this makes sure the heading is
cleaned_csv_content = list()
for row in reader:
    err = 0
    fname, lname, e_mail  = row
    try:
        validate_email(email)
    except:
        pass
        err = 1
    if not err:
        cleaned_csv_content.append(fname, lname, e_mail)

Firstly, you should raise not return your errors.

Secondly, try: with a bare except: is bad practice (e.g. http://blog.codekills.net/2011/09/29/the-evils-of--except--/); what do you expect could go wrong with each step? Test for specific errors and deal with them appropriately. It is this which has hidden the NameError I mention at the end.

Thirdly, your use of the flag err seems a bit odd; much more Pythonic to use an actual boolean err = False or, better, else::

try:
    validate_email(e_mail) # see comment below
except: # but only the errors you expect - see above
    pass
else:
    cleaned_csv_content.append((fname, lname, e_mail)) # note additional parentheses

or, even better, refactor validate_email to return True for a valid email and False otherwise, rather than raising errors:

if validate_email(e_mail):
    cleaned_csv_content.append((fname, lname, e_mail))

Finally, note that "email" != "e_mail".


Best way to implement a web MVC application (language agnostic) Well, the questions is a little bit broader, so I'll assume that the basics is already defined:

Controller

FrontController
Controllers (plugin hooks, request/response object, view handler)

View

View (template, scripts, partials, helpers)

Model

Domain model objects
Domain model repository
Database abstraction layer

Let me know if I missed something.

Assuming that the above is correct. Now the big question:

Should controllers implement business logic?

// (an example in php)
class UsersController {
   public function create() {
      $user = new User();
      $user->setName($this->getRequest()->getPost('name'));
      // ...
   }
}

The point is, the create action is deciding which information a user need to be created. Therefore, I'm assuming that it's business logic.

// (an example in php just delegating the job to the repository)
class UsersController {
   public function create() {
      $this->userRepository->create($this->getRequest->getPost());
   }
}

If we delegate the job to the repository, then I'm assuming that now the controller is just doing its job, no business logic in there.

Now the second question. Should repositories deal with create, edit and delete operations?

Finally, it will be common for repositories to populate associations and aggregations. So the user repository can also populate the user object with its article associations, for instance. In this case, should the repository call the article repository to do so?

Now about the aspect oriented programming:

We need to deal with ACLs. Whether a domain object or collection is accessible by the current user is determined by the ACLs.

Isn't it part of the model?

Should controller implement ACL's checks as a plugin hooked on the pre-dispatch or, since it's business logic, should our repositories know if its methods are accessible by the current user?

Now what I achieved so far

Well, I ended up with a service layer with protected methods like:

class UsersService extends AbstractService {
   protected function _create($data) {
       $this->validate($data)
       $this->userRepository->create($data);
   }
}

and with a overloading (again in php, but in java for instance, we'd assume a proxy)

class AbstractService {
    public function __call($method, $args) {
       // ACLs checks
       // then call the protected method
    }
}

My service layer is responsible to hold validations and ACLs checks.

So in the end:

class UsersController {
   public function create() {
      $this->userService->create($this->getRequest->getPost());
   }
}

Should controllers implement business logic?

That depends on your design and style of doing things "your" MVC-way. As you're designing a framework you should probably allow both to keep the usage of the framework flexible.
Taking a closer look on your code examples I'd say you're only shifting things around anyway. Why do you have a controller when you use it to access the request? Shouldn't the request lead into the controller call and the controller should not care at all about the request any longer? Just a counter question, if you want to make your framework strict for some reason, think about why.

Now the second question. Should repositories deal with create, edit and delete operations?

Normally that's within the domain of a repository, but whatever suits your design needs, a repository could be just a delegate to different create, edit and delete components as well. Application-wise it shouldn't make a difference whether or not, so probably do a risk analysis and then decide which part of the question you actually need to answer now or if you can solve this by just deciding now, and if the future shows that your design needs change, change your design.

We need to deal with ACLs. Whether a domain object or collection is accessible by the current user is determined by the ACLs.
Isn't it part of the model?

Sure, it's part of the model, the ACL Domain Model (and explicitly not other models).


Adding minutes to a time value in Ruby I recently submitted a code-challenge for an interview according to the following guidelines:

About

Write a function or method that accepts two mandatory arguments and returns a result.

Requirements:

Use Ruby or another language of your choosing, and write production quality code.
Your function or method should not use any date/time library.
The first argument is a time value in the form of a string with the following format: [H]H:MM {AM|PM}
The second argument is the number of minutes, as an integer value
The return value of the function should be a string of the same format as the first
argument, which is a result of adding the minutes to the time.

For example, add_minutes('9:13 AM', 10) would return 9:23 AM

Additional notes:

We just want to see how you code, so this exercise is not meant to be too hard or take too long.
If you spend an hour on this, stop coding and finalize by adding some notes about what you would do if you had more time.

Although my Ruby proficiency has somewhat atrophied due to primarily working with php for the last 8 months, I was able to produce the following well-within one-hour of beginning the challenge:

# FIRST ATTEMPT

# require 'time'

# def flux_capacitor(time,mins)
#   the_time = Time.parse(time.to_s)
#   mins = the_time + 10*60
# end

# FINAL REFACTOR

def flux_capacitor(time,mins)
  the_time = time.scan(/\d/).join('').to_i
  meridian = time.scan(/(A|M|P)/).join('')
  new_time = the_time + mins
  back_in_time = new_time.to_s.insert(-3,':') + " #{meridian}"
end

puts flux_capacitor("9:13 AM",10)
puts flux_capacitor("9:13 PM",10)
p flux_capacitor("10:13 PM",10).class

# DOX

# http://ruby-doc.org/core-2.2.3/Time.html
# https://www.ruby-forum.com/topic/125709
# https://www.ruby-forum.com/topic/104359
# http://apidock.com/ruby/String/insert
# http://rubular.com/

Simply-stated, is there a better approach?  I've asked others and they think my solution is fairly clean.  Obviously, I know my solution could benefit with a conditional statement for edge cases (e.g. flux_capacitor("9:50 AM", 15)).

More testing is required:
flux_capacitor("11:59 AM", 62)
 => "12:21 AM"

Definitely test some corner cases.

Updated Thoughts
I think you should completely rethink the way you are handling time.
Use a regex to validate that time is a valid string, then simply use string.split to get each portion of the time: hours, minutes, and AM/PM.
Next you should convert those into a quantity of minutes:
hours += 12 if meridian == 'PM'
total_minutes = hours*60+minutes+mins

Next I would recalculate the time as a 24-hour time (which goes up to 23 hours, 59 minutes):
# this is integer math so the decimal portion will be discarded
# adjusted_minutes may be greater than 23, but adjusted_minutes should always be less than 60
adjusted_hours = (total_minutes / 60)
adjusted_minutes = total_minutes - (60*adjusted_hours)

# modulus 24 converts adjusted_hours into 24-hour time
adjusted_hours = adjusted_hours % 24

Now convert to 12 hour time if necessary and determine if this is AM or PM
# the added mins may have been several days worth, so adjusted_hours might be very large.

if adjusted_hours > 12
    meridian = "PM"
    adjusted_hours -= 12
else
    meridian = "AM"
end

Finally return a string of the new time:
# zero pad the minutes, as this is expected for a time
return "%d:%02d %s" % [adjusted_hours, adjusted_minutes, meridian]

Final Solution
Since a solution that reliably passes all test cases has yet to be put forth...
def flux_capacitor(time, mins)
    #check if time in valid format
    time_match = time.strip.match /^(12|11|10|0?\d):([012345]\d)\s+(AM|PM)/

    #throw error on invalid time
    raise(ArgumentError, "Invalid time: #{time.strip}") if not time_match

    #calculate new time
    strhours, strminutes, meridian = time_match.captures
    hours = (meridian == "AM" ? strhours.to_i : strhours.to_i + 12)
    total_minutes = hours * 60 + strminutes.to_i + mins
    total_minutes = total_minutes % (24*60) # we only want the minutes that fit within a day
    adjusted_hours, adjusted_minutes = total_minutes.divmod(60)
    if adjusted_hours > 12
        adjusted_hours -= 12
        meridian = "PM"
    else
        meridian = "AM"
    end

    "%d:%02d %s" % [adjusted_hours, adjusted_minutes, meridian]
end

Testing:
[ "11:13 PM",
  "13:09 PM", #invalid!
  "1:59 AM",
  "04:49 PM",
  "4:79 PM" #invalid!
].each do |time|
    begin
      puts flux_capacitor(time, 0)
    rescue
      puts $!
    end
end
puts "----------"
puts "#{flux_capacitor("11:13 PM", 10)}, expected: 11:23 PM"
puts "#{flux_capacitor("11:13 PM", 12*60)}, expected 11:13 AM"
puts "#{flux_capacitor("11:13 PM", 24*60)}, expected 11:13 PM"
puts "#{flux_capacitor("11:13 PM", 24*60 + 1)}, expected 11:14 PM"
puts "#{flux_capacitor("11:59 AM", 62)}, expected 1:01 PM"
puts "#{flux_capacitor("11:59 PM", 62)}, expected 1:01 AM"


Need fast csv parser for Python to parse 80GB csv file I am looking for the fastest way to parse an 80GB csv file with 300 columns in Python.

The csv file does not have backquote commas, e.g  a,"blah,blah,blah",c

I have tried Python's built-in csv module which gives me ~50MB/s speed.

with open(file_name) as csvfile:
    reader = csv.reader(csvfile,delimiter=',')
    for row in reader:
        pass

With plain line.split() I get ~55MB/s:

with open(file_name) as csvfile:
    for line in csvfile:
        row = line.split(',')

Without parsing, I get ~400MB/s:

with open(file_name) as csvfile:
    for line in csvfile:
        pass

Using cperf I see that most of time time is used for split() or csv.reader() or re.split(), which I have also used.

Can I make this faster? If a plain read from disk is 400MB/s, I would like to get ~200MB/s while parsing the csv file if possible.

I would suggest you to create a thread pool and do line.split() in separate thread. Playing with thread pool size can give you balance between processing speed and consumed resources.


Dismissing a modal dialog I am implementing a modal dialog in my Android application, allowing the user to either pick a player from a list, or create one with predefined traits.

PlayerPickerListener

public interface PlayerPickerListener {
    Activity getActivity();
    void playerCreated();
}

PlayerPicker (extends Dialog)

public PlayerPicker(Context context, String title, final PlayerPickerListener listener) {
    super(context);

    setTitle(title);
    setContentView(R.layout.player_dialog);

    View.OnClickListener changeHandler = new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            // PlayersActivity is basically a ListView selecting the player
            Intent intent = new Intent(listener.getActivity(), PlayersActivity.class);
            intent.putExtra(REQ_CODE, SELECT_ACTION);
            listener.getActivity().startActivityForResult(intent, SELECT_ACTION);
        }
    };

    Button b = (Button) findViewById(R.id.button_change);
    b.setOnClickListener(changeHandler);

    View.OnClickListener newHandler = new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            EntityPlayer player = PlayerManager.createNewPlayer();
            listener.playerCreated();
        }
    };
    b = (Button) findViewById(R.id.button_new);
    b.setOnClickListener(newHandler);

}

MainActivity (implements PlayerPickerListener)

public Activity getActivity() { return this; }

public void displayPlayer(View view) {
    myPlayerPicker = new PlayerPicker(this, "Pick a player", this);
    myPlayerPicker.show();
}

public void playerCreated() {
    myPlayerPicker.dismiss();
    // some extra stuff
}

@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
    if (requestCode == PlayersActivity.SELECT_ACTION && resultCode == RESULT_OK && data != null) {
        myPlayerPicker.dismiss();
        // some extra stuff
    }
    super.onActivityResult(requestCode, resultCode, data);
}

I have the following issues with my approach:

I am led to add the getActivity() method in my listener, because I have no other way of starting an intent from the Dialog. I need an intent to go to my ListView and return with a result.
Because of the above behavior, I am calling dismiss() on my Dialog from inside my Activity. For the new player case, I could easily add the dismissal of the Dialog inside the OnClickListener, but I am leaving it on the Activity just to have a type of uniform behavior.

My main question is whether it is common practice to dismiss a Dialog in its own code, or externally.

I'd argue that it would not seem natural for a dialog to be shut down from another source. A Dialog should disappear when its objective is complete. Is this bad design?

The code in general looks good, but here are a few observations:

Activity

You can access the activity from the context as well. In your case, the context is actually the activity context. Therefore, you can cast to activity so that you can start an intent.
Dialog

You can disable the dialog from wherever you want, depending on the needs. Disabling from inside is ok if, for example, you want to do something simple and return a callback to the caller and disable it immediately. Something simple and that's it, no complex operations involved, like rotation for example.

If you want to handle rotations and other complicated operations as coming from memory, etc, then it might make sense to do it externally from the parent application, or from the application that started the dialog.


PowerShell - Execute Windows Task Scheduler function ExecuteWindowsTaskScheduler {
param(
    [Parameter(Mandatory = $true)]
    [String]$TaskName, #e.g. "Open Notepad task"

    [Parameter(Mandatory = $true)]
    [String]$TimeToExecute, #e.g. "3:45pm/am"

    [Parameter(Mandatory = $true)]
    [ValidateSet('Once','Daily','Weekly, Monthly')]
    [string]$FrequencyToExecute,

    [Parameter(Mandatory = $true)]
    [String]$DomainAndUser, #e.g. "yourdomain\yourusername"

    [Parameter(Mandatory = $true)]
    [String]$ProgramWithPath, #e.g. "C:\PowerShell\yourFile.ps1"

    [Parameter(Mandatory = $false)]
    [bool]${DebugMode}
)

$TriggerParams = @{
    At = $TimeToExecute
}

# Add the appropriate frequency value to the splatting table
if ($FrequencyToExecute -eq 'Monthly') {
    $TriggerParams.Add('Weekly',$true)
}
else {
    $TriggerParams.Add($FrequencyToExecute,$true)
}

# Specify the trigger settings
if ($FrequencyToExecute -eq "Weekly") {
    $Trigger = New-ScheduledTaskTrigger @TriggerParams -WeeksInterval 1 -DaysOfWeek Monday
}
elseIf  ($FrequencyToExecute -eq "Monthly") {
    $Trigger = New-ScheduledTaskTrigger @TriggerParams -WeeksInterval 4 -DaysOfWeek Monday
}
else {
    $Trigger = New-ScheduledTaskTrigger @TriggerParams
}

# Specify what script to run and with its parameters
$Action = New-ScheduledTaskAction -Execute "PowerShell.exe" -Argument $ProgramWithPath
$TaskExists = Get-ScheduledTask | Where-Object {$_.TaskName -like $TaskName }

if($TaskExists) {
    Unregister-ScheduledTask -TaskName $TaskName -Confirm:$false
}

I'm definitely not happy with this part of the code:

# Add the appropriate frequency value to the splatting table
    if ($FrequencyToExecute -eq 'Monthly') {
        $TriggerParams.Add('Weekly',$true)
    }
    else {
        $TriggerParams.Add($FrequencyToExecute,$true)
    }

    # Specify the trigger settings
    if ($FrequencyToExecute -eq "Weekly") {
        $Trigger = New-ScheduledTaskTrigger @TriggerParams -WeeksInterval 1 -DaysOfWeek Monday
    }
    elseIf  ($FrequencyToExecute -eq "Monthly") {
        $Trigger = New-ScheduledTaskTrigger @TriggerParams -WeeksInterval 4 -DaysOfWeek Monday
    }
    else {
        $Trigger = New-ScheduledTaskTrigger @TriggerParams
    }

Is there a way to improve this part? I mean, I had to create a small hack because there is no Monthly parameter for New-ScheduledTaskTrigger.

Your instincts are good to dislike that bit of code.

Whenever you can, you should put code in data structures rather than in control statements. It makes the code clearer and easier to maintain.

So for instance you could put your settings in a hash table like this:

$FrequencySettings = @{
    Once     = @{Once   = $true}
    Daily    = @{Daily  = $true}
    Weekly   = @{Weekly = $true; WeeksInterval = 1; DaysOfWeek = 'Monday'}
    Monthly  = @{Weekly = $true; WeeksInterval = 4; DaysOfWeek = 'Monday'}
    }

# Let's test it:
$FrequencyParams = $FrequencySettings['Monthly']
New-ScheduledTaskTrigger @FrequencyParams -At "0:00"

I haven't checked that the particular settings I've put in above are all correct. I just want to show you the basic idea.


Improved indices trick We all know about the indices trick, right? Here's (hopefully) an improved version...

template <::std::size_t...> struct indices { };

namespace detail
{

template<class A, class B> struct catenate_indices;

template <::std::size_t ...Is, ::std::size_t ...Js>
struct catenate_indices<indices<Is...>, indices<Js...> >
{
  using indices_type = indices<Is..., Js...>;
};

template <::std::size_t, ::std::size_t, typename = void> struct expand_indices;

template <::std::size_t A, ::std::size_t B>
struct expand_indices<A, B, typename ::std::enable_if<A == B>::type>
{
  using indices_type = indices<A>;
};

template <::std::size_t A, ::std::size_t B>
struct expand_indices<A, B, typename ::std::enable_if<A != B>::type>
{
  static_assert(A < B, "A > B");
  using indices_type = typename catenate_indices<
    typename expand_indices<A, (A + B) / 2>::indices_type,
    typename expand_indices<(A + B) / 2 + 1, B>::indices_type
  >::indices_type;
};

}

template <::std::size_t A>
struct make_indices : detail::expand_indices<0, A>::indices_type
{
};

template <::std::size_t A, ::std::size_t B>
struct make_indices_range : detail::expand_indices<A, B>::indices_type
{
};

Usage:

#include <iostream>

#include "utility.hpp"

template <::std::size_t ...Is>
void show_indices(indices<Is...>)
{
  [](...){}((((::std::cout << Is << ::std::endl), 0))...);
}

int main()
{
  show_indices(make_indices_range<3, 9>());

  return 0;
}

Any suggestions?

You can get rid of the <type_traits> header by replacing your std::enable_if by a bool template specialization:

template <::std::size_t, ::std::size_t, bool>
struct expand_indices_impl;

template <::std::size_t A, ::std::size_t B>
struct expand_indices:
    expand_indices_impl<A, B, A==B>
{};

template <::std::size_t A, ::std::size_t B>
struct expand_indices_impl<A, B, true>
{
    using indices_type = indices<A>;
};

template <::std::size_t A, ::std::size_t B>
struct expand_indices_impl<A, B, false>
{
    static_assert(A < B, "A > B");
    using indices_type = typename catenate_indices<
        typename expand_indices<A, (A + B) / 2>::indices_type,
        typename expand_indices<(A + B) / 2 + 1, B>::indices_type
    >::indices_type;
};

Moreover, in C++ (and many other programming languages, see Python range for example), ranges tend to have the end-exclusive form [begin, end); therefore, make_indices_range<3, 9> should contain the indices from 3 to 8, and not from 3 to 9. It is easy to alter the behaviour:

template <::std::size_t A>
struct make_indices:
    detail::expand_indices<0, A-1>::indices_type
{};

template <::std::size_t A, ::std::size_t B>
struct make_indices_range:
    detail::expand_indices<A, B-1>::indices_type
{};

As proposed in the comments, make_indices_range also needs to provide an empty range when A == B:

template <::std::size_t A>
struct make_indices_range<A, A>:
    indices<>
{};


Determining what functions should be implemented in this user-authentication class Long story short, I would like to use OOP for my new PHP project. It has a "login" requirement (i.e. user will first need to enter the username and password (login.php), if it's correct, it will be redirected to index.php and then fetch some products information). Also there will be a settings page that allows user to change the password, and email if necessary.

In summary:

When user attempts to login, the program will check if the username, password entered is correct or not. If it's correct, username (or maybe the whole User object) will be saved in the $_SESSION.
When the user settings page is loaded, it will display all user related fields on screen (i.e. by using $_SESSION).
The user can make changes in the user settings pages and data should be saved to the database.

I have this User class:

class User {
     private $username;
     private $email;
     private $hashed_password;
     private $dob;
...

     public __construct($user) {
          $this->username = $user;
     }

     public function isPasswordCorrect($userenteredpassword) {
           $this->loadFromDatabase();
           //hashed userenteredpassword, compare it against $this->hashed_password
           //if it matches, return true, otherwise return false.
     }

     public function saveToDatabase() {
          //save info to database
     }

     public function loadFromDatabase() {
         $row = select fields from user table where username = $this->username limit 1
         if ($row is found) {
              $this->hashed_password = $row['hashed_password'];
              $this->dob = $row['dob'];
              ...
         }
     }
}//class

However, I was told that the function isPasswordCorrect shouldn't be implemented inside class User, as it is not its job to determine whether the login is valid or not.  Instead, I should create an authService helper and use it to determine whether the login is good or not.

Now, this makes me wonder if the rest of my User class design is okay or not. If I shouldn't put the password checking function in the User class, then is it okay to implement the database related functions in User?  How do I determine which function should be implemented in the class?  Should I follow Single Responsibility principle? Wouldn't it be too extreme?

The Single Responsibility Principle is a good thing to aim for, for many classes. It doesn't always mean that each class only does one thing (though it can), but that it is responsible for a small amount of tightly-related functionality.

That said, the job of some classes is to collect together a number of responsibilities. User may well be one of these, but the individual responsibilities can still be delegated to other classes - e.g. you could always give User an instance of a Credentials class that deals with the password checking.

You almost certainly don't want your database-related functions in User as if you write any tests for User, many of them are likely to have a need to hit the database when they run.


Java Left Rotate Array upto n times Java problem to left rotate an array upto n times. It would be nice if anyone can review this code.

Task:

A left rotation operation on an array of size  shifts each of the array's elements  unit to the left. For example, if left rotations are performed on array , then the array would become .

Given an array of  integers and a number, , perform  left rotations on the array. Then print the updated array as a single line of space-separated integers.

solution

public class LeftRotateArray {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        Scanner in = new Scanner(System.in);
        int Array_Size = in.nextInt();
        int Rotation_Number = in.nextInt();
        int arr[] = new int[Array_Size];
        for (int i = 0; i < Array_Size; i++) {
            arr[i] = in.nextInt();
        }
        for (int i = 0; i <= Array_Size - 1; i++) {
            if (i < Array_Size - Rotation_Number)
                System.out.print(arr[i + Rotation_Number] + "\t");

            else
                System.out.print(arr[i - Array_Size + Rotation_Number] + "\t");

        }
    }

}

As people have already pointed out you have to actually rotate the array.To do that  a really cool way would be using the mod(%) operator so even if the Rotation_Number variable that you are using is greater than the array length your code will run fine.

Declare a new array

   int arrRotated[] = new int[Array_Size];

and to store and print the array

    for (int i = 0; i <= Array_Size - 1; i++) {
        arrRotated[i] = arr[(i+Rotation_Number)%arr.length];
        System.out.print(arrRotated[i]);
    }


Synchronized block over concurrent collections Here, this vector was used at many APIs in the code with sync block on it. And all the places they want to search another collection based on each itr.next().

synchronised(vector)
{
    Iterator itr = vector.iterator();
    while (itr.hasNext)
    {
       //Perform some search on another collection using the value from itr.next() :: THIS IS COSTLY JOB WITH SYNCH BLOCK
       //If that collection doesn't have any entry for itr.next() then perform itr.remove();
     }
}

This is like a vector of conference IDs.  Using each conference ID, it gets a conference from list of conferences, then searches again for a user within that conference having user ID from list of users available in that conference.

As stated above: now the sync block is doing a time-consuming search job on another objects.  And other threads keep waiting for this vector lock.  And the total number of threads in the blocked state increases much while load testing.

I changed the code from Vector to Collections.newSetFromMap(new ConcurrentHashMap<psConference,Boolean>()). I assume that, since this is concurrent collection, no ConcurrentModificationException while performing iteration. Add/remove operations while iteration is allowed here, so I have removed all sync block in my code.

But I come to know from ConcurrentHashMap Java doc:

  iterators are designed to be used by only one thread at a time.

From the above, I think I have moved the sync block problem from my code to the Java API. There is no advantage over this change.

I finally found the best return copy of original vector collection each time any APIs requires that vector of conference IDs. Let the iteration happen on this new vector with that costly searching and other jobs and then finally perform the remove separately.

Vector newVectorObject = mgr.getCopyOfVector();
List toBedeleted = new ArryList();

Iterator itr = newVectorObject .iterator();
    while (itr.hasNext)
    {
       //Perform some search on another collection using itr.next()
       //If that collection doesn't have any entry for itr.next() then perform toBedeleted.add(itr.next())
     }
mgr.removeVector(toBedeleted);

public Vector getCopyOfVector(){
      return new Vector(vector);
}

public Vector removeVector(List toBedeleted){
      return vector.removeAll(toBedeleted);
}

I think in this case, no lock is required on the vector.  No threads will be blocked.

It looks like you are trying to re-invent the CopyOnWriteArrayList.

By the way, Vector is very old fashioned, but I guess you are working with legacy code.  The code also does not use generics.  CopyOnWriteArrayList started with Java 5, so might not be able to use it if you are stuck with an older Java version.


How can I make a shorter binary rule in haskell? Here is a code I experimented with recently:
let br = [0]:[n:(concat $ take n br) | n <- [1..]] in concat br

This code produces the binary rule 0 1 0 2 0 1 0 3 ...
Here is the idea of how it works:

create a list starting with [0]

each step, concatenate all the current elements in the list, put the next number at the begining and add at the end.

[[0] ...]
[[0], [1, 0] ...]
[[0], [1, 0], [2, 0, 1, 0]...]
[[0], [1, 0], [2, 0, 1, 0], [3, 0, 1, 0, 2, 0, 1, 0] ...]
...

My question now is: is there a shorter/smarter one-liner to get this infinite lazy binary-rule ?
Edit:
I found another way to do the same thing:
let f n = if n==0 then [0] else n:([0..n-1] >>= f) in [0..]>>= f

The extra question would be: wich one of the 2 version is more readable for an haskell programmer, and why ?

I'm a little out of practice with haskell, but I'm quite fond of it. I want to address your "extra" question first.

wich one of the 2 version is more readable for an haskell programmer, and why ? [sic]

Haskell programmers aren't a special magic kind of people. They tend to take the benefits of brevity more seriously than folks in love with python, but I think they'll still agree that golfed code is not readable/smarter/better. The basic rule of good (code) writing applies: Write what you mean.

create a list starting with [0]
each step, concatenate all the current elements in the list, put the next number at the beginning and add at the end.

Neither of your proposed solutions is obviously doing that, although it only takes a minute to see that your first solution is doing that.
Noughtmare's suggestion is based on a different construction of the OEIS sequence, although the OEIS presents this as an invariant, not a construction, so the reader still needs to think the matter through (a little).
Give-or-take efficiency, write code that obviously does what you're saying it does. Unfortunately, the OEIS entry doesn't describe the sequence in terms that translate directly to a lazy infinite list declaration, so you need to provide some documentation of your own.
-- https://oeis.org/A007814
rulerSequence :: [Integer]
rulerSequence = concat halfs
  where seed = [0, 1]
        -- The sequence is built by concatenating with itself
        -- and incrementing the last index. `nextHalf` builds
        -- the right-hand-side of each such concatenation.
        nextHalf priorHalfs = let priors = concat priorHalfs
                              in (init priors) ++ [(last priors) + 1]
        halfs = seed:[nextHalf (take iteration halfs) | iteration <- [1..]]


Simple javascript toggle div function I have written a code that triggers my div with another menu. I do not know which one is better and should be used. Can I get some feedback?
First code in one function:
function sidebar() {
var menu = document.querySelector('#sidebar-container');
      var menuSmall = document.querySelector('#sidebar-container2');
      var icon = document.getElementById('right');
      menuSmall.style.display = "block";
      menu.style.display = "none";
      if(menu.style.display == "none") {
        icon.onclick = function () {
          menu.style.display = "block";
          menuSmall.style.display = "none";
}
}

Second code with two functions but does the same work:
function sidebar() {
      var menu = document.querySelector('#sidebar-container');
      var menuSmall = document.querySelector('#sidebar-container2');
      menuSmall.style.display = "block";
      menu.style.display = "none";

    },
    function openbar() {
      var menu = document.querySelector('#sidebar-container');
      var menuSmall = document.querySelector('#sidebar-container2');
      menuSmall.style.display = "none";
      menu.style.display = "block";
    }

I think that is easier to write and understand the state change if you don't need to think about the edge cases.
In the first case, you already start in the state that you want and only need to invert the both states. In the second, you just need to "turn off" every states and "turn on" the one that you want to.
I think that is simplier if you think this way.
Two sidebars
I think that you can use DOMTokenList.toggle():

const sidebars = document.querySelectorAll('.sidebar');
const icon = document.getElementById('icon');

const toggle = el => el.classList.toggle('hidden');

icon.addEventListener('click',() => sidebars.forEach(toggle));
/* the display value of a div is block by default */
.hidden {
  display: none;
}

first

second

icon

Docs:

DOMTokenList
Element.classList
DOMTokenList.forEach()

More than two sidebars
Maybe toggle can't be useful in this case. However, I think that datasets might help

const sidebars = document.querySelectorAll('.sidebar');
const btns = document.querySelectorAll('.btn');

const getSidebar = btn => btn.dataset.sidebar;

const hide = el => el.classList.add('hidden');

btns.forEach(btn =>
  btn.addEventListener('click', () => {
    const target = document.getElementById(getSidebar(btn));
    sidebars.forEach(hide);
    target.classList.remove('hidden');
  })
);
.hidden {
  display: none;
}

sidebar 1

sidebar 2

sidebar 3

<button class="btn" data-sidebar="first">1</button>
<button class="btn" data-sidebar="second">2</button>
<button class="btn" data-sidebar="third">3</button>


Multiplying 4*4 matrices I'm multiplying 4x4 matrices for learning purposes. How can I optimize this for performance?

HTML

-1	0	-1	0

8	-9	8	-9

5	6	2	-4

-2	7	-8	1

4	3	4	3

-1	7	-1	7

5	2	9	-3

6	2	4	1

jQuery

var first, fifth,
    second, sixth,
    third, seventh,
    forth, eighth;

    for (var i = 0; i < 4; i++) {
        for (var j = 0; j < 4; j++) {
            first = parseInt($('.matrex-1 td.0-' + i).text());
            second = parseInt($('.matrex-2 td.' + j + '-0').text());
            third = parseInt($('.matrex-1 td.1-' + i).text());
            forth = parseInt($('.matrex-2 td.' + j + '-1').text());
            fifth = parseInt($('.matrex-1 td.2-' + i).text());
            sixth = parseInt($('.matrex-2 td.' + j + '-2').text());
            seventh = parseInt($('.matrex-1 td.3-' + i).text());
            eighth = parseInt($('.matrex-2 td.' + j + '-3').text());

            $('.matrex-3 td.' + j + '-' + i).text(first * second + third * forth + fifth * sixth + seventh * eighth);
        }
    }

For small matrix multiplications, I think it is typically fastest (surprisingly) to implement the basic matrix multiplication you learn in elementary/middle school, which gives us an asymptotic runtime complexity of n^3, where n is the length and width of the arrays (assuming both arrays have the same dimensions and aspect ratios).

In Pseudo code, this algorithm goes as follows:

for (i = 0; i < n; i++) {
        for (j = 0; j < n; j++) {
            resultingArray[i][j] = 0;
            for (k = 0; k < n; k++) {
                resultingArray[i][j] += firstArray[i][k] * secondArray[k][j];
            }
        }
    }
    return resultArray;

For larger matrices, typically 30 and up (subjective), there is an algorithm called Strassen's Algorithm that multiplies matrices with an approximate asymptotic runtime complexity of 2^n, again, where n is the length and width of the matrices. This problem is a recursive algorithm that breaks the problem into a lot of sub-problems by breaking down and working on web-matrices. This algorithm typically has a numeric "cross-over" point variable that is defined by the developer and represents the size bound of the matrices where it would be more efficient to simply multiply using the standard matrix multiplication algorithm. The algorithm would roughly go as follows:

if (n <= crossOverPoint) {
        resultArray = basicMatrixMult (n, firstArray, secondArray, resultArray);
    } else {
        double[][] firstArraySubArray1 = partitionSubArray (firstArray, 0, (n / 2), 0, (n / 2));
        double[][] firstArraySubArray2 = partitionSubArray (firstArray, (n / 2), n, (n / 2), n);

        double[][] secondArraySubArray1 = partitionSubArray (secondArray, 0, (n / 2), 0, (n / 2));
        double[][] secondArraySubArray2 = partitionSubArray (secondArray, 0, (n / 2), (n / 2), n);

        resultArray = strassenMatrixMult ((n / 2), addMatrices (firstArraySubArray1, firstArraySubArray2),
                                                 addMatrices (firstArraySubArray1, firstArraySubArray1),
                                                 resultArray);
    }
    return resultArray;

Here's some more info in case you wanted to read up on Strassen's Algorithm:
https://en.wikipedia.org/wiki/Strassen_algorithm


Breadth First Search implementation in Python 3 to find path between two given nodes This is my Breadth First Search implementation in Python 3 that assumes cycles and finds and prints path from start to goal.

Some background - Recently I've been preparing for interviews and am really focussing on writing clear and efficient code, rather than just hacking something up like I used to do.

This is another step in that direction when I'm revisiting some basic algorithms and trying to write best implementations of them as I can from my knowledge and obviously some help and explanations from internet. Any criticism would be helpful. I've tried to make the code as readable and efficient as I could.

from collections import deque

def bfs(graph, start, goal):
    if start == goal:
        print([start])
        return

    queue = deque([start])

    # dict which holds parents, later helpful to retreive path.
    # Also useful to keep track of visited node
    parent = {}
    parent[start] = start

    while queue:
        currNode = queue.popleft()
        for neighbor in graph[currNode]:
            # goal found
            if neighbor == goal:
                parent[neighbor] = currNode
                print_path(parent, neighbor, start)
                return
            # check if neighbor already seen
            if neighbor not in parent:
                parent[neighbor] = currNode
                queue.append(neighbor)
    print("No path found.")

def print_path(parent, goal, start):
    path = [goal]
    # trace the path back till we reach start
    while goal != start:
        goal = parent[goal]
        path.insert(0, goal)
    print(path)

if __name__ == '__main__':
    graph = {'A': set(['B', 'C']),
             'B': set(['A', 'D', 'E']),
             'C': set(['A', 'F']),
             'D': set(['B']),
             'E': set(['B', 'F']),
             'F': set(['C', 'E'])}

    bfs(graph, 'D', 'F')

Comments

Provide a docstring, detailing what the code does and returns

Pep-8

When writing Python, it is advised to follow the styleguide. This means snake_case instead of hybridCamelCase for variable.

Another formatting I use is

graph = {
    'A': set(['B', 'C']),
    'B': set(['A', 'D', 'E']),
....
    'F': set(['C', 'E']),
}

Instead of your representation. If ever you change something to this, the diff of your code versioning system will be a lot clearer and simpler. (note the trailing comma after the last entry)

Separate presentation with the calculation.

Use a function traversing the graph, and returning the path, and another one to present the result (if needed). This way, you can also write unittests that don't need to mock print to test the algorithm

Use an exception to signal something exceptional

Instead of just printing that there is no path, you can signal this by returning None or raising an exception

class NoPathException(Exception): pass

Data structure

instead of keeping a separate dict with the path, it is easiest if you stack the queue with the node and the path used to reach it so far. That make your effort a lot easier. There is no need to keep the dict, but an ordinary set with the nodes visited should suffice.

Minimize the indents

To minimize the number of indents and keep your line length under control, instead of

for item in loop:
    if condition:
        do_something()

you can do:

for item in loop:
    if not condition:
        continue
    do_something()

Especially with a lot of nested conditions, this is a lot more compact.

Complete algorithm

def bfs2(graph, start, goal):
    """
    finds a shortest path in undirected `graph` between `start` and `goal`.
    If no path is found, returns `None`
    """
    if start == goal:
        return [start]
    visited = {start}
    queue = deque([(start, [])])

    while queue:
        current, path = queue.popleft()
        visited.add(current)
        for neighbor in graph[current]:
            if neighbor == goal:
                return path + [current, neighbor]
            if neighbor in visited:
                continue
            queue.append((neighbor, path + [current]))
            visited.add(neighbor)
    return None  # no path found. not strictly needed

if __name__ == '__main__':
    graph = {
        'A': set(['B', 'C']),
        'B': set(['A', 'D', 'E']),
        'C': set(['A', 'F']),
        'D': set(['B']),
        'E': set(['B', 'F']),
        'F': set(['C', 'E']),
    }
    path = bfs2(graph, 'D', 'F')
    if path:
        print(path)
    else:
        print('no path found')


Member list reveals member information on click (#2) Not a follow-up, but related to Member list reveals member information on click (#1). I now want my CSS reviewed.

What I'm doing?

The content in a list of members is revealed by jQuery (see review #1). Instead of having the content pop out, I'm using a transition to make this smooth. Because one can't transition to height: auto;, it's triggered by using max-height: 2000px;.

I have feeling this can be improved.

HTML:

            Member Name 1

Content area. No layout things inside. Paragraphs, lists, images.

            Member Name 2

Content area. No layout things inside. Paragraphs, lists, images.

            Member Name 3

Content area. No layout things inside. Paragraphs, lists, images.

CSS:

.block-list__link {
    text-decoration: none;

    .block-list__title:after {
        /* Adding a space followed by `&raquo;` after titles inside links */
        content: "\0020" "\00BB";
    }
}

.block-list__thumbnail {
    width: 100px;
    height: 100px;
    background-color: #f60;
    float: left;
}

.block-list__title {
    line-height: 100px;
    margin-bottom: 0;
    margin-left: 30px;
}

.block-list__content {
    overflow: hidden;

    /* Omitting vendor-prefixes for demonstration */
    transition: max-height 0.8s linear;

    height: 0;
    max-height: 0;

    &.visible {
        margin-top: 30px;
        height: 100%;
        max-height: 2000px;
    }
}

this popped out at me, I haven't looked at the entire code completely yet, so take this with a grain of salt please

not sure why you do this

   Member Name 2

I would probably write it like this instead

    Member Name 2

I am pretty sure that you don't need that extra div
I think the image tag is going to act similar to the div tag, and I assume that you want to style the image and not the div anyway,  it looks more like a container for the image which I think is redundant. ( I have been wrong before and am not afraid to admit it when someone gives me some solid evidence )

that is the only thing that I can see right now, but I haven't spent much time looking at it either.


Aggregate prescriptions per drug in a CSV file I have a question that is similar in spirit to this previously asked question. Nonetheless, I can't seem to figure out a suitable solution.

Input: I have CSV data that looks like  (FICTIONAL PATIENT DATA)

id,prescriber_last_name,prescriber_first_name,drug_name,drug_cost
1000000001,Smith,James,AMBIEN,100
1000000002,Garcia,Maria,AMBIEN,200
1000000003,Johnson,James,CHLORPROMAZINE,1000
1000000004,Rodriguez,Maria,CHLORPROMAZINE,2000
1000000005,Smith,David,BENZTROPINE MESYLATE,1500

Output: from this I simply need to output each drug, the total cost which is summed over all prescriptions and I need to get a count of the unique number of prescribers.

drug_name,num_prescriber,total_cost
AMBIEN,2,300.0
CHLORPROMAZINE,2,3000.0
BENZTROPINE MESYLATE,1,1500.0

I was able to accomplish this pretty easily with Python. However, when I try to run my code with a much larger (1gb) input, my code does not terminate in a reasonable amount of time.

import sys, csv

def duplicate_id(id, id_list):
    if id in id_list:
        return True
    else:
        return False

def write_file(d, output):
    path = output
    # path = './output/top_cost_drug.txt'
    with open(path, 'w', newline='') as csvfile:
        fieldnames = ['drug_name', 'num_prescriber', 'total_cost']
        writer = csv.DictWriter(csvfile, fieldnames=fieldnames)
        writer.writeheader()
        for key, value in d.items():
            print(key, value)
            writer.writerow({'drug_name': key, 'num_prescriber': len(value[0]), 'total_cost': sum(value[1])})

def read_file(data):
    # TODO: https://codereview.stackexchange.com/questions/88885/efficiently-filter-a-large-100gb-csv-file-v3
    drug_info = {}
    with open(data) as csvfile:
        readCSV = csv.reader(csvfile, delimiter=',')
        next(readCSV)
        for row in readCSV:
            prescriber_id = row[0]
            prescribed_drug = row[3]
            prescribed_drug_cost = float(row[4])

            if prescribed_drug not in drug_info:
                drug_info[prescribed_drug] = ([prescriber_id], [prescribed_drug_cost])
            else:
                if not duplicate_id(prescriber_id, drug_info[prescribed_drug][0]):
                    drug_info[prescribed_drug][0].append(prescriber_id)
                    drug_info[prescribed_drug][1].append(prescribed_drug_cost)
                else:
                    drug_info[prescribed_drug][1].append(prescribed_drug_cost)
    return(drug_info)

def main():
    data = sys.argv[1]
    output = sys.argv[2]
    drug_info = read_file(data)
    write_file(drug_info, output)

if __name__ == "__main__":
    main()

I am having trouble figuring out how to refactor this to handle the larger input and was hoping someone on CodeReview could take a look and provide me some suggestions for how to solve this problem. Additionally, if you happen to see any other issues, I'd love the feedback.

Performance

Your performance problem is primarily due to the duplicate_id() function, namely the if id in id_list test.  Testing whether an n-element list contains a particular item is O(n).  If you call that for n rows, then it's O(n2).  That would not be acceptable at all for large files.

Sorting the input would help bring it down to O(n log n).

But really, what you want is a solution with O(n) runtime, based on the hashing principle.  Use dict, set, or similar data structures so that each lookup takes constant time.

Style

Use csv.DictReader to avoid the ugliness of discarding the first row using next(readCSV), as well as the magic column numbers in row[0], row[3], and row[4].

Instead of writing two cases depending on whether an entry already exists in a dictionary, use collections.defaultdict, collections.Counter, dict.get(key, default), or dict.setdefault(key, default).

Suggested solution

from collections import Counter, defaultdict
import csv
import sys

def write_file(output_filename, drugs):
    with open(output_filename, 'w') as f:
        w = csv.DictWriter(f, ('drug_name', 'num_prescriber', 'total_cost'))
        w.writeheader()
        w.writerows({
                'drug_name': drug_name,
                'num_prescriber': len(prescriber_totals),
                'total_cost': sum(prescriber_totals.values()),
            }
            for drug_name, prescriber_totals in drugs.items()
        )

def read_file(input_filename):
    drugs = defaultdict(Counter)
    with open(input_filename) as f:
        for row in csv.DictReader(f):
            drugs[row['drug_name']][row['id']] += float(row['drug_cost'])
    return drugs

def main(input_filename, output_filename):
    write_file(output_filename, read_file(input_filename))

if __name__ == '__main__':
    main(sys.argv[1], sys.argv[2])


Print the frequency of characters as a horizontal histogram Well, I've used some of the feedback from my last posting to improve how I wrote this program. Mainly using the void parameter in main, initializing my int array to zero without using a for-loop, and better naming of variables. If there are some issues that are going on with my code, or there's some things I can do better, please let me know.

Any feedback is welcome!

#include <stdio.h>

/* print histogram of the frequencies of different characters in its input */
int main(void)
{
  int histogram[95] = {0}; /* initialize 95 spaces for ASCII characters 32 - 127 */
  int c, i, j;

  while ((c = getchar()) != EOF) {
    ++histogram[c-' '];
  }

  for (i = 32; i < 127; ++i) {
    printf("%c ", i);
    ++histogram[c-' '];
    for (j = 0; j < histogram[i-' ']; ++j) {
      putchar('x');
    }
    putchar('\n');
  }
}

When dealing with offset arrrays (using position 0 to indicate character 32), it is important for readability to use symmetrical access patterns for the process. In your case, you load the values as c-' ', but what you use to read them, is accessed by i = 32; i < 127 .... and this asymmetrical access is inconvenient.

You also have no handling for out-of-bound characters.... can you trust your input sources to not have a carriage return, or a tab? Even esoteric null characters, or other binary values. I would have a catch-all 'bucket' for invalid values. That will prevent segfaults or other undefined actions.

You have a small bug in your code. In the print-loop, you increment the last character received.... what is this line for in the 'print' loops:

++histogram[c-' '];

Finally, for code reuse, it is often important to extract your functionality to a single reentrant method, other than the main method. Consider a signature like:

void histogram(const int offset, const int range)

where your histogram can be configured to take an offset position, and range, like:

histogram('a', 'z' - 'a' + 1);

which will count just the lower-case letters...

Something like:

#include <stdio.h>
#include <memory.h>

void histogram(const int offset, const int range) {
  int histogram[range];
  memset(histogram, 0, sizeof(histogram));

  int special = 0;

  int c;
  while ((c = getchar()) != EOF) {
    if (c < offset || c >= (offset + range)) {
        special++;
    } else {
        ++histogram[c - offset];
    }
  }

  for (int i = 0; i < range; ++i) {
    c = i + offset;
    printf("%c ", c);
    for (int j = 0; j < histogram[i]; ++j) {
      putchar('x');
    }
    putchar('\n');
  }

  printf("- ");
  for (int j = 0; j < special; j++) {
    putchar('x');
  }
  putchar('\n');
}

int main(void) {
    histogram(' ', 95);
}


RegEx version control this is my first time on this stack exchange :)

I created a little script to check for the latest version of specified API version available, but I am wondering if I am using the best way possible for checking regex and includes?

The var arr will be filled dynamically with version folders using fs.readdir, but for simplicity I wrote them in the array manually and the var pattern will come from req.params.v using nodejs express router middleware.

Just looking for an approval or if there is a better way, thank you!

var arr = ['v1.0.0', 'v5.2.4', 'v5.2.9', 'v5.20.4', 'v6.4.0'];
var pattern = 'v5.2';

const regex = /^v[0-9](\.[0-9]{1,2})?(\.[0-9])?$/;

if (regex.test(pattern) && (arr.filter(s => s.includes(pattern)) != 0)) {

    if (pattern.match(/\./g) == null) {
        console.log('major version');
        console.log(pattern);
    } else if (pattern.match(/\./g).length == 1) {
        console.log('minor version');
        console.log(pattern);
        pattern = pattern + '.';
    } else {
        console.log('patch version');
        console.log(pattern);
    }
    const matches = arr.filter(s => s.includes(pattern));
    console.log(matches[matches.length - 1]);

} else {
    console.log('Specify correct API version!');
}

Thank you in advance!

Likely logic flaw

In the first conditional:

if (regex.test(pattern) && (arr.filter(s => s.includes(pattern)) != 0)) {

Array.filter() "creates a new array"1 so the second conditional expression compares an array with zero. Those two should never be loosely equal so that conditional always evaluates to true. Perhaps you intended to check the length property of the array.

Improving efficiency

The code has repeated operations - e.g. arr.filter(s => s.includes(pattern)) appears twice, and then there are multiple calls to pattern.match(). The call to arr.filter() could be stored in a variable and then used whenever needed instead of re-filtering.

Also, the regular expression stored in regex could be used with String.match() to get an array of matches. Because the regular expression has capturing groups, those capturing groups can be used instead of calling pattern.match(/\./g) multiple times to check for the number of dots. Instead, check if the array returned by String.match() isn't null and then if the 2nd and 3rd elements are undefined.

See a comparison in this jsPerf.

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter


Looping through rows of data sets in files I have files containing data sets which contain 11,000 rows. I have to run a loop through each row, which is taking me 25 minutes for each file.

     z <- read.zoo("Title.txt", tz = "")
  for(i in seq_along(z[,1])) {
    if(is.na(coredata(z[i,1]))) next
   ctr <- i
    while(ctr < length(z[,1])) {
     if(       (   abs  ( coredata(z[i,1]) -coredata(z[ctr+1,1])  )      )  > std_d) {
       z[ctr+1,1] <- NA
       ctr <- ctr + 1

      } else {
  break
    }
 }
}

Where Title.txt is a file containing 11,000 rows. It looks like this (first five rows):

"timestamp" "mesured_distance" "IFC_Code" "from_sensor_to_river_bottom"
 "1" "2012-06-03 12:30:07-05" 3188 1005 3500
 "2" "2012-06-03 12:15:16-05" 3189 1005 3500
 "3" "2012-06-03 12:00:08-05" 3185 1005 3500
 "4" "2012-06-03 11:45:11-05" 3191 1005 3500
 "5" "2012-06-03 11:30:15-05" 3188 1005 3500

How can I increase the speed of this code?

In your code, std_d is not defined.  Presumably it is some threshold value, since it is used in a comparison of absolute differences.

First, reformat your code so that I can better determine what it is supposed to do:

for(i in seq_along(z[,1])) {
    if(is.na(coredata(z[i,1]))) next
    ctr <- i
    while(ctr < length(z[,1])) {
        if((abs(coredata(z[i,1])-coredata(z[ctr+1,1]))) > std_d) {
            z[ctr+1,1] <- NA
            ctr <- ctr + 1
        } else {
            break
        }
    }
}

Then determine what the algorithm is that you are trying to do:

Looping over each element of the first column of z, skipping any missing values, compare that value to every other value to the end of column. If the current value and some subsequent value differ by more than some threshold (in absolute difference), set that subsequent value to NA.

R is a vectorized language, so explicit for loops are often not the most efficient way to perform an operation.

Let's vectorize that inner while loop.

for(i in seq_along(z[,1])) {
    if(is.na(coredata(z[i,1]))) next
    ctr <- (i+1):length(z[,1])
    z[i+which(abs(coredata(z[i,1]) - coredata(z[ctr,1])) > std_d),1] <- NA
}

I make ctr a vector of all the indexes higher than i, and use that most functions are vectorized.  For example, with i equal to 1, (and picking std_d equal to 2) working the statements from inside out:

> ctr <- (i+1):length(z[,1])
> ctr
[1] 6 5
> i
[1] 5
> i <- 1
> ctr <- (i+1):length(z[,1])
> ctr
[1] 2 3 4 5
> coredata(z[i,1]) - coredata(z[ctr,1])
 4  3  2  1
-3  3 -1  0
> abs(coredata(z[i,1]) - coredata(z[ctr,1]))
4 3 2 1
3 3 1 0
> abs(coredata(z[i,1]) - coredata(z[ctr,1])) > std_d
    4     3     2     1
 TRUE  TRUE FALSE FALSE
> which(abs(coredata(z[i,1]) - coredata(z[ctr,1])) > std_d)
4 3
1 2
> i+which(abs(coredata(z[i,1]) - coredata(z[ctr,1])) > std_d)
4 3
2 3
> z[i+which(abs(coredata(z[i,1]) - coredata(z[ctr,1])) > std_d),1]
2012-06-03 11:45:11 2012-06-03 12:00:08
               3191                3185
> z[i+which(abs(coredata(z[i,1]) - coredata(z[ctr,1])) > std_d),1] <- NA
> z
                    mesured_distance IFC_Code from_sensor_to_river_bottom
2012-06-03 11:30:15             3188     1005                        3500
2012-06-03 11:45:11               NA     1005                        3500
2012-06-03 12:00:08               NA     1005                        3500
2012-06-03 12:15:16             3189     1005                        3500
2012-06-03 12:30:07             3188     1005                        3500

This change alone would probably speed up your code a lot. You could aslo pull the computation of length(z[,1]) outside the loop, but that will be a much smaller improvement.

The sample of data you gave is too small to do any reasonable benchmarking on.


Binary Search Tree insert while keeping track of parent for node to be added This question has a follow up question:
  Binary Search Tree insert while keeping track of parent for node to be added - iteration 2

I am implementing a red black tree for fun and am wondering how I should modify my basic BST insert. Note: this happens before the red black tree rules are applied, it just finds the correct place within the tree to add the node, places it, sets references, value and defaults the color to RED. I am mainly struggling to see if there may be a better way to tack on the parent reference for the newly added node. The implementation I have here looks ahead one step with a NULL check where a BST insert that does not need to track the parent would not need.

struct node * bstInsert(struct node *n, int x) {

  if (n != NULL) {

    int cmp = (n->val < x) ? -1 : (n->val > x);

    if (cmp == -1) {

      if (n->left == NULL) {
        n->left = createAChild(n, n->left, x);
      } else {
        bstInsert(n->left, x);
      }

    } else if (cmp > 0) {

      if (n->right == NULL) {
        n->right = createAChild(n, n->right, x);
      } else {
        bstInsert(n->right, x);
      }

    }

  }

  return n;
}

struct node * createAChild(struct node *par, struct node *n, int x) {

  n = malloc(
    sizeof(struct node)
  );
  n->parent = par;
  n->left = n->right = NULL;
  n->val = x;
  n->color = RED;

  return n;

}

Is there a cleaner solution to setting the parent reference for the node to be added?

Instead of if (n != NULL) and then indenting the whole code of function, it is better to write:
if (n == NULL) { return NULL; }
The line int cmp = (n->val < x) ? -1 : (n->val > x); is very hard to read.
I suggest finding a better name to cmp - for example isGreater and changing its type to boolean.


Refactor similar queries but reverse side of inequality operator How can I refactor this?

public static IQueryable<T_COMPANY> WhereTotalTransactionOrShowCaseAtLeast(this IQueryable<T_COMPANY> query, int minimum, DateTime? date = null)
{
    if (date == null)
    {
        query = query.Where(company => (company.Products.Count + company.References.Count) >= minimum ||
                                     ( (company.T_RFQs.Count + company.T_POs.Count + company.T_INVOICEs.Count) >= minimum) );
        return query;
    }
    else
    {
        var notNullDate = date.Value;

        var result = (from company in query
                      let rfqCount = company.T_RFQs.Count(item => item.create_date <= notNullDate)
                      let poCount = company.T_POs.Count(item => item.create_date <= notNullDate)
                      let invoiceCount = company.T_INVOICEs.Count(item => item.create_date <= notNullDate)
                      let productCount = company.Products.Count(item => item.create_date <= notNullDate)
                      let referenceCount = company.References.Count(item => item.create_date <= notNullDate)
                      where (rfqCount + poCount + invoiceCount) >= minimum || (productCount + referenceCount) >= minimum
                      select company);

        return result;
    }

}

public static IQueryable<T_COMPANY> WhereTotalTransactionOrShowCaseLessThan(this IQueryable<T_COMPANY> query, int minimum, DateTime? date = null)
{
    if (date == null)
    {
        query = query.Where(company => (company.Products.Count + company.References.Count) >= minimum ||
                                     ((company.T_RFQs.Count + company.T_POs.Count + company.T_INVOICEs.Count) < minimum));
        return query;
    }
    else
    {
        var notNullDate = date.Value;

        var result = (from company in query
                      let rfqCount = company.T_RFQs.Count(item => item.create_date <= notNullDate)
                      let poCount = company.T_POs.Count(item => item.create_date <= notNullDate)
                      let invoiceCount = company.T_INVOICEs.Count(item => item.create_date <= notNullDate)
                      let productCount = company.Products.Count(item => item.create_date <= notNullDate)
                      let referenceCount = company.References.Count(item => item.create_date <= notNullDate)
                      where (rfqCount + poCount + invoiceCount) >= minimum || (productCount + referenceCount) < minimum
                      select company);

        return result;
    }

}

From what I can understand you are looking for a way to consolidate this code into one function and specify that the comparrison with minimum can either be less than or greater than or equal to. You could try something like this

public static IQueryable<T_COMPANY> WhereTotalTransactionOrShowCaseAtLeast(this IQueryable<T_COMPANY> query, int minimum, DateTime? date = null)
    {
        WhereTotalTransactionWithCompare(query, minimum, (x, min) => x >= min, date);
    }

    public static IQueryable<T_COMPANY> WhereTotalTransactionOrShowCaseLessThan(this IQueryable<T_COMPANY> query,
                                                                                int minimum,
                                                                                DateTime? date = null)
    {
        WhereTotalTransactionWithCompare(query, minimum, (x, min) => x < min, date);

    }

    public static IQueryable<T_COMPANY> WhereTotalTransactionWithCompare(this IQueryable<T_COMPANY> query,
                                                                                int minimum,
                                                                                Func<int, int bool> compareToMinimum,
                                                                                DateTime? date = null)
    {
        if (date == null)
        {
            query = query.Where(company => (company.Products.Count + company.References.Count) >= minimum ||
                                         (comparer(company.T_RFQs.Count + company.T_POs.Count + company.T_INVOICEs.Count, minimum)));
            return query;
        }
        else
        {
            var notNullDate = date.Value;

            var result = (from company in query
                          let rfqCount = company.T_RFQs.Count(item => item.create_date <= notNullDate)
                          let poCount = company.T_POs.Count(item => item.create_date <= notNullDate)
                          let invoiceCount = company.T_INVOICEs.Count(item => item.create_date <= notNullDate)
                          let productCount = company.Products.Count(item => item.create_date <= notNullDate)
                          let referenceCount = company.References.Count(item => item.create_date <= notNullDate)
                          where (rfqCount + poCount + invoiceCount) >= minimum || comparer(productCount + referenceCount, minimum)
                          select company);

            return result;
        }

    }


Address book app to learn bash I'm fairly proficient in C and Python, but want to learn some skills for administrating my new Linux machine. I wrote this simple address book app to teach myself shell scripting. This is the first script I've written longer than a couple of lines.

The address book is a plain text file stored in $HOME/.sab-db. Entries are separated by newlines and the three fields (first name, surname, email) are separated by colons, like the fields in /etc/passwd.

The application is divided into four scripts that have a single task each. This is inspired by how Git is organized. Is this a good way to isolate independent code in bash?

How should I handle shared code between scripts? I've use two different approaches below: I created a standalone script that the others call for determining the database name, and I copy-pasted the die function. I've heard of script sourcing, but I've never seen that used for creating bash "libraries".

sab-find-db

#!/bin/bash

DB_NAME=".sab-db"

if [ -n "$SAB_DB_DIR" ]; then
    echo "$SAB_DB_DIR/$DB_NAME"
else
    echo "$HOME/$DB_NAME"
fi

sab-search

#!/bin/bash

function die {
    echo "$1" >&2
    exit 1
}

if [ $# -eq 1 ]; then
    # Fields can't include : since it's the delimiter
    if [ $(echo "$1" | grep ":" | wc -l) -gt 0 ]; then
        exit 0
    else
        file=$(mktemp)
        grep -i "$1" $(./sab-find-db) >"$file" 2>/dev/null
    fi
elif [ $# -eq 0 ]; then
    file=$(./sab-find-db)
else
    die "usage: $0 [<first name> | <surname> | <email>]"
fi

cat "$file"

while read line; do
    echo "First name: $(echo "$line" | cut -d: -f1 -)"
    echo "Surname:    $(echo "$line" | cut -d: -f2 -)"
    echo "Email:      $(echo "$line" | cut -d: -f3 -)"
    echo
done <"$file"

if [ $# -eq 1 ]; then
    rm "$file"
fi

sab-add

#!/bin/bash

function die {
    echo "$1" >&2
    exit 1
}

if [ $# -eq 3 ]; then
    entry="$1:$2:$3"
elif [ $# -eq 0 ]; then
    echo -n "First name: "
    read first_name
    echo -n "Surname: "
    read surname
    echo -n "Email: "
    read email
    entry="$first_name:$surname:$email"
else
    die "usage: $0\n [<first name> <surname> <email>]"
fi

if [ $(echo "$entry" | grep -o ":" | wc -l) -gt 2 ]; then
    die "Fields can't include colon"
# Check if any : is next to another : or start or end of line
elif [ $(echo "$entry" | grep "\(^:\)\|\(::\)\|\(:$\)" | wc -l) -gt 0 ]; then
    die "No fields can be left empty"
elif [ $(grep -i "^$entry\$" $(./sab-find-db) 2>/dev/null | wc -l) -gt 0 ]; then
    die "This entry already exists. It was not added again."
else
    echo "$entry" >>$(./sab-find-db)
fi

sab-remove

#!/bin/bash

function die {
    echo "$1" >&2
    exit 1
}

if [ $(grep -i "\<$1\>" $(./sab-find-db) 2>/dev/null | wc -l) -eq 0 ]; then
    die "No match found"
fi

file=$(mktemp)

if [ ! $(grep -vi "\<$1\>" $(./sab-find-db) >"$file" 2>/dev/null) ]; then
    mv "$file" $(./sab-find-db)
else
    rm "$file"
fi

Your main questions

  The application is divided into four scripts that have a single task each. This is inspired by how Git is organized. Is this a good way to isolate independent code in bash?

If the scripts are short, it's simpler to have all the code in one script, with each main functionality in its own function.

If the scripts are longer, and you suspect that they will get longer and longer, then it makes sense to keep them as separate scripts in a dedicated directory. However, in this case, if the scripts need to call each other, then you need a way so that they can find each other. A simple way is to add the directory of the scripts to PATH. Another simple way is to require that users must cd into the directory of the scripts of they want to use them.

Since you launch another script using the ./ prefix, that will only work if the other script is in the current directory.

  How should I handle shared code between scripts? I've use two different approaches below: I created a standalone script that the others call for determining the database name, and I copy-pasted the die function. I've heard of script sourcing, but I've never seen that used for creating bash "libraries".

Yes, script sourcing is more common. Another benefit of that will be that you can define your common functions in one place, no need to copy-paste die into every script that needs it.

Something I often do is cd to the directory of the script, and then source the common configuration and function definitions, like this:

cd $(dirname "$0")
. ./config.sh
. ./functions.sh

Function definitions

The convention is to put parentheses in function definitions, and to omit the " function " keyword, like this:

die() {
    echo "$1" >&2
    exit 1
}

Benefit from program exit codes

You can benefit from the exit codes of programs more aggressively. For example instead of this:

if [ $(echo "$1" | grep ":" | wc -l) -gt 0 ]; then

It's better to write this way:

if echo $1 | grep -q :; then

There are several things going on here:

I omitted the quoting of $1, because for the purpose here, it doesn't matter
grep exits with success if a pattern is found
The -q flag makes grep output nothing, but that of course doesn't change the fact of success or not, it's just too avoid garbage output
The statement has been radically changed. Instead of comparing the output of the wc command, we are operating based on exit codes. There is no more [ ... ]

We can do even better. Instead of echo, we can use "here strings" to cut out one more process:

if grep -q : <<< "$1"; then

But the best is to use pattern matching with [[:

if [[ $1 = *:* ]]; then

Use the principle about exit codes to rewrite the rest of your code and the other scripts.

Exit with non zero on error

In many places you exit with code 1 to signal an error but not here:

if [ $(echo "$1" | grep ":" | wc -l) -gt 0 ]; then
   exit 0

It would be better to echo a friendly message, and then exit with 1.

Safety

This part is a bit scary:

if [ $# -eq 1 ]; then
    rm "$file"
fi

Within the same script, file is sometimes a temporary file, sometimes it's the actual database. This can be confusing. It's easy to make mistakes in shell scripting, and with one simple mistake, your database might go up in smoke. I suggest to reorganize your code: move this dangerous operation closer to the code that creates the temp file. In addition, refer to the temp file by a different name, and use that name in the rm command. In other words, make it really hard to mistakenly delete the wrong file.


Deleting duplicates from a string I'm pretty new, but I'm always looking for different/better ways to solve these problems as it helps me understand it better and hopefully will make me better programmer. Feel free to point out any mistakes or industry "dont's" as well.

#include <cstdio>
using namespace std;

//Function Prototypes
void deleteDupes(char message[], int msg_length);
int length(char message[]);

int main()
{
    //Declaring vars
    int msg_length = 0;
    char message[500] = "The dog jumped up and down and ate chocolate, then he bit the cat";

    //Display the original message and it's length
    msg_length = length(message);
    puts(message);
    printf("The message length before deleting the dupes is: %d\n\n", msg_length);

    //Deletes the duplicates and displays the new message and length
    deleteDupes(message, msg_length);
    msg_length = length(message);
    puts(message);
    printf("The message length after deleting the dupes is: %d\n", msg_length);

    return 0;
}//end main

//Function to determine the message length
int length(char array[])
{
    int i = 0;
    for(i; '\0' != array[i]; i++){}
    return i;
}

//Function to delete the duplicate char(s) from the array message
void deleteDupes(char message[], int msg_length)
{
    for(int i = 0; i < msg_length; ++i)
    {
       for(int j = i + 1; j < msg_length; ++j)
       {
           if(message[j] == message[i])
           {
               for(int k = j; k < msg_length; ++k)
               {
                   message[k] = message[k + 1];
               }
            msg_length -= 1;
            --i;
           }
       }
    }
}

C++ is C++

A lot of this code is basically C. So let's C++ify it. <cstdio> is the C-style IO library. In C++, we have <iostream>, with cout. The biggie here is:

char message[500] = "The dog jumped up and down and ate chocolate, then he bit the cat";

C++ has a library for dealing with strings. It's called <string>, and is very useful:

std::string message = "The dog ...";

std::string manages its own memory and comes with a whole bunch of member functions. One of those is size():

size_t msg_length = message.size();
std::cout << message << '\n';
std::cout << "The message length before deleting the dupes is: "
          << msg_length << "\n\n";

Although if you want to stick with pointers, you still wouldn't need to write your own length function as there is strlen. But if you really insist on writing your own loop, please put the thing you're comparing on the left-hand side:

for (; array[i] != 0; ++i) ;

Or, since we're comparing against 0:

for (; array[i]; ++i) ;

Putting constants on the left-hand side is, frankly, terrible.

deleteDupes

The C++ standard library comes with a whole bunch of algorithms. The algorithms tend to be very useful, both in terms of code readability, performance, and correctness. For one thing, your loop has a bug (you have --i which presumably should be --j).

I would use the Erase-Remove idiom. That is:

void deleteDupes(std::string& message) {
    message.erase(
        std::remove_if(message.begin(), message.end(), ???),
        message.end()
        );
}

What do we put in the ???s? We need a predicate that will return true if this is a character we've seen before. Easy way of keeping track of that is an std::unordered_set and thanks to C++11, we can stick the predicate in-line:

void deleteDupes(std::string& message) {
    std::unordered_set<char> seen;
    message.erase(
        std::remove_if(message.begin(), message.end(), [&](char c){
            return !seen.insert(c).second;
        })
        message.end()
        );
}

Basically, insert will tell you if it succeeded or not (i.e. this is the first time you've seen c or not). If it's the first time, we return false (and insert into our set). Otherwise, we return true and the remove_if algorithm marks this character for deletion. We then erase all the dupes in one go.

Simple. Correct. Readable.


Unit of Work and Repository with Entity Framework 6 Based on the reply of this question I have created the following code. I need to check whether it's good or not.

Here is my entity class:

public class Employee
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Designation { get; set; }
}

This is my db context implementation:

 public class MyDataContext<T> : DbContext where T:class
{
    private IDbSet<T> _dbSet;

    public MyDataContext() : base("name=DefaultConnectionString")
    {
        _dbSet = this.Set<T>();
    }

    public MyDataContext(IDbSet<T> dbSet )
        : base("name=DefaultConnectionString")
    {
        this._dbSet = dbSet;
    }

    public IDbSet<T> DbSetOjbect
    {
        get { return _dbSet; }
    }
}

Now I have implemented the EmployeeService business logic class and the IEmployee service class:

 public interface IEmployeeService
{
    List<Employee> GetEmployees();
}

Here is the implementation:

public class EmployeeService : IEmployeeService
{
    private IDbSet<Employee> employee;

    public EmployeeService()
    {
        var employeeContext = new MyDataContext<Employee>();
        employee = employeeContext.DbSetOjbect;
    }

    public EmployeeService(IDbSet<Employee> employee)
    {
        this.employee = employee;
    }

    public List<Employee> GetEmployees()
    {
        return employee.ToList();
    }
}

The following is my controller code in ASP.NET MVC controller.

 public class EmployeeController : Controller
{
    private readonly IEmployeeService _employeeService;

    public EmployeeController()
    {
        _employeeService = new EmployeeService();
    }

    public EmployeeController(IEmployeeService employeeService)
    {
        _employeeService = employeeService;
    }

    public ActionResult Index()
    {
        return View(_employeeService.GetEmployees());
    }
}

I want to check whether it is a good approach for TDD Test Driven Development or not.

I've never TDD'd, but don't do that:

public class MyDataContext<T> : DbContext where T : class

This gives you a context-per-entity, which might work for ultra-simplistic CRUD scenarios, but doesn't scale very well and will quickly give you headaches as soon as you need to deal with more than a single entity type in a single transaction - because that's what a unit-of-work encapsulates: a transaction.

DbContext is a unit-of-work, and IDbSet<T> is a repository; they are an abstraction; by wrapping it with your own, you're making an abstraction over an abstraction, and you gain nothing but complexity.

This blog entry sums it up pretty well. In a nutshell: embrace DbContext, don't fight it.

If you really want/need an abstraction, make your DbContext class implement some  IUnitOfWork interface; expose a Commit or SaveChanges method and a way to get the entities:

public interface IUnitOfWork
{
    void Commit();
    IDbSet<T> Set<T>() where T : class;
}

Then you can easily implement it:

public class MyDataContext : DbContext, IUnitOfWork
{
    public void Commit()
    {
        SaveChanges();
    }
}

I don't like IEmployeeService either. This looks like an interface that can grow hair and tentacles and become quite a monster (GetByName, FindByEmailAddress, etc.) - and the last thing you want is an interface that needs to change all the time.

I'd do it something like this, but I'm reluctant to use the entity types directly in the views, I'd probably have the service expose EmployeeModel or IEmployee instead (see this question for more details - it's WPF, but I think lots of it applies to ASP.NET/MVC), so as to only have the service class aware of the Employee class, leaving the controller and the view working off some IEmployee implementation, probably some EmployeeModel class, idea being to separate the data model from the domain model.

public class EmployeeService
{
    private readonly IUnitOfWork _unitOfWork;

    public EmployeeService(IUnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    IEnumerable<Employee> GetEmployees()
    {
        return _unitOfWork.Set<Employee>().ToList();
    }
}


Implementing hash table in Python using Tuples I am trying to implement a basic hash table using a list of tuples. The code produce the desired output. Please help me improve my code as I feel there are a lot of while loops.
#global variables to keep track of the hash table
count = 0
size = 1000

def hash_table(size):
    return [None] * size

def hash_item(key, value):
    return (key,value)

def hash_func(key):         #simple hash function
    index = hash(key) % 1000
    return index

def insert_hash(table,key,value):
    index = hash_func(key)
    item = hash_item(key,value)
    global count
    global size
    if count == size:
        raise Exception("Hash table limit reached")
    if table[index] == None:
        table[index] = item
    else:
        collision_handler(table, index, item)
    count = count + 1

def print_hash(table,key):
    index = hash_func(key)
    while table[index][0] != key:      #in case there is a collision:
        index = index + 1
    print(table[index])

def delete_hash(table,key):
    index = hash_func(key)
    while table[index][0] != key:
        index = index + 1
    table.pop(index)

def collision_handler(table,index,item):         #handling collisions using open addressing
        while table[index] != None:
            index = index + 1
        table[index] = item

a = hash_table(size)

b = hash_func('1')

insert_hash(a, '1','john')

print_hash(a,'1')

delete_hash(a,'1')

insert_hash(a,'1','edward')

print_hash(a,'1')

Despite the code produce the desired output in your simple test case, your implementation is quite flawed.
a = hash_table(size)

insert_hash(a, 1, 'john')
insert_hash(a, 2, 'edward')
delete_hash(a, 1)
print_hash(a, 2)

Output:
Traceback (most recent call last):
  File "C:/Code Review/hash_table.py", line 53, in <module>
    print_hash(a, 2)
  File "C:/Code Review/hash_table.py", line 30, in print_hash
    while table[index][0] != key:      #in case there is a collision:
TypeError: 'NoneType' object is not subscriptable
>>>

The issue: table.pop(index) will move items after the one being delete forward by one position in the table.  Given that they are indexed by the hash values, this would mean that they can't be found anymore.
a = hash_table(size)

for _ in range(2000):
    insert_hash(a, 1, 'john')
    delete_hash(a, 1)

Output:
Traceback (most recent call last):
  File "C:/Code Review/hash_table.py", line 51, in <module>
    insert_hash(a, 1, 'john')
  File "C:/Code Review/hash_table.py", line 22, in insert_hash
    if table[index] == None:
IndexError: list index out of range
>>>

After each insert/delete, the table size has shrunk by one item.  After a thousand insert/deletes, the table can't hold any values!
a = hash_table(size)

insert_hash(a, 999, 'john')
insert_hash(a, 999, 'edward')

Output:
Traceback (most recent call last):
  File "C:/Code Review/hash_table.py", line 51, in <module>
    insert_hash(a, 999, 'edward')
  File "C:/Code Review/hash_table.py", line 25, in insert_hash
    collision_handler(table, index, item)
  File "C:/Code Review/hash_table.py", line 42, in collision_handler
    while table[index] != None:
IndexError: list index out of range
>>>

A collision at the last slot in the hash table doesn't have room to handle the collision, despite the table only having a 999 empty slots.  You need to use circular addressing.
You need to add more test cases, and rework the code to handle these issues.  I generated these issues quickly by exploiting the fact that hash(int) == int for sufficiently small integers, but they would arise using strings for keys as well.  It is just be harder to generate cases which deterministicly fail using string keys.


Best data Structure for storing a phone keypad format I was trying to code the phone keypad characters with its numbers. The first idea which came to my mind is using a HashMap. After trying various options I came up with the below Code. I want to know if the data structure and methodology which I implemented is good or does it need some more improvements.
Things which I know and don't want to implement:

I do not want to hardcode the keypad. I know that there are various ways of implementing a hardcoded keypad. I visited this and this link to learn about other ways.
I would really be thankful if the solution is given in java 7 syntax. I don't have much idea about java 8 syntax, but it'll be helpful to know the solution in that too, if possible.

Keypad feature according to my problem statement:

0 and 1 buttons are not functional.
7 and 9 buttons contain 4 letters each and others contain 3 letters each.

public static void keypadDemo(){
        int ax = 97; // I need to store lower-case letters so took this
        Map<Integer, ArrayList<Character>> hm = new HashMap<>(); // This is my data-structure
        for(int i = 0; i < 10; i++){
            if(i == 0 || i == 1){
                hm.put(i, null); // 0 and 1 keys are not functional as per my problem statement.
            }else if(i == 7 || i == 9){
                hm.put(i, new ArrayList<>(Arrays.asList((char) ax, (char) ++ax, (char) ++ax, (char) ++ax)));
                ax++;
            }else{
                hm.put(i, new ArrayList<>(Arrays.asList((char) ax, (char) ++ax, (char) ++ax)));
                ax++;
            }
        }
       System.out.println(hm);
    }

Any help will be welcomed. Thanks
EDIT:
Keypad format:

The problem statement:

We are given two inputs:

A phone number in string format.
A list of strings.

The output is the list containing only those strings which correspond to any substring of the given phone number when converted to the corresponding letters on the phone keypad.

Example test case:

Input:
3662277
words = ["foo", "bar", "baz", "emo", "foobar", "cap", "car", "cat"]

Output:
outputList = ["foo", "bar", "foobar", "emo", "cap", "car"]

I coded the solution and it got accepted too by online judge. But since, I'm learning about Collections in java so wanted to know whether my choice of the data structure is good or not.

It's not possible to say if this method is a good or useful abstraction without context which you have not provided. Accordingly, this review is of the method by itself.
My primary concern with this method is that it is hard to read and understand. Complexity leads to bugs, and should be avoided where possible. This method is doing a simple thing in a complex way. Readers have to worry about a loop and three branches instead of a simple, static mapping.
keypadDemo() should return the Map, not print it. Let the client decide what to do with it.
ax and hm are very poor variable names. Variable names should clearly indicate the value they hold.
Using an integer to magically act as characters makes the method harder to read.
Use the most appropriate abstraction. Map<Integer, List<Character>> would be preferable. The type of list is an implementation detail.
A loop is the wrong structure to build this map. The code is hard to read, which means it's easy to introduce bugs and hard to notice them.
Short-circuiting the loop might make it easier to read.
Empty lists would be better than using null. It's easier to write code that correctly handles an empty list than to special-case null checks.
In idiomatic Java, there is whitespace before a { and after a }. There is whitespace after a for, if, etc.
If I were to need this method in my own code, it would look like: (untested)
/**
 * Generates a mapping from digits on a telephone keypad to the letters for
 * digit. The letters are in alphabetical order. Digits with no letters
 * map to an empty list.
 */
public static Map<Integer, List<Character>> digitsToCharacters() {
    Map<Integer, List<Character>> digitsToCharacters = new HashMap<>();
    digitsToCharacters.put(0, Arrays.asList());
    digitsToCharacters.put(1, Arrays.asList());
    digitsToCharacters.put(2, Arrays.asList('a', 'b', 'c'));
    digitsToCharacters.put(3, Arrays.asList('d', 'e', 'f'));
    digitsToCharacters.put(4, Arrays.asList('g', 'h', 'i'));
    digitsToCharacters.put(5, Arrays.asList('j', 'k', 'l'));
    digitsToCharacters.put(6, Arrays.asList('m', 'n', 'o'));
    digitsToCharacters.put(7, Arrays.asList('p', 'q', 'r', 's'));
    digitsToCharacters.put(8, Arrays.asList('t', 'u', 'v'));
    digitsToCharacters.put(9, Arrays.asList('w', 'x', 'y', 'z'));
    return Collections.unmodifiableMap(digitsToCharacters);
}


Improve performance by excluding multiple conditions and List This method works correctly and verifies whether a user has permissions to view or edit the first parameter (value). I think it is not the best implementation, because there are too many conditions and the List object isn't necessary. How to improve it?

protected override Exception GetValidationException( object value, string name )
{
  List<Entity> entities = value is int ? new List<Entity>( 1 ) { EntityCache.Entity.GetEntity( (int) value ) } :
                          value is Guid ? new List<Entity>( 1 ) { EntityCache.Entity.GetEntityByGuid( (Guid) value ) } :
                          value is int[] ? (value as int[]).Select( i => EntityCache.Entity.GetEntity( i ) ).ToList() :
                          value is Entity ? new List<Entity>( 1 ) { value as Entity } :
                          value is Entity[] ? (value as Entity[]).ToList() :
                          null;

  if( entities == null )
    throw new ArgumentException( string.Format( RI.UnknownArgumentType, name, value, value.GetType().Name ) );

  bool hasPermission = (from one in entities
                        select EntityPermissionCache.PermissionChecker( one.Id, one.EntityOrganizationId ) &&
                               _checkEditInsteadOfView ? EntityTypePermissionCache.PermissionChecker( true, one.TypeId ) : true)
                       .All( a => a );

  return hasPermission ? null : new AccessViolationException( RI.AccessDenied );
}

I think the assignment of your entities (bad name) is uber-abusing ternary expressions.

That said, my first thought was why on Earth would you want to return an Exception? So I googled up a bit and found this:

  Is it valid for a function to return exception objects?

  Absolutely. Here is a short list of examples where this may be appropriate:

  An exception factory.
  A context object that reports if there was a previous error as a ready to use exception.
  A function that keeps a previously caught exception.
  A third-party API that creates an exception of an inner type.

Fine then. But there's a flaw in your API - you're taking in an Object for what seems to be an Entity.

If I understand what you're trying to do here, I believe you'd be better off with an overloaded method:

protected override Exception GetValidationException(int value, string name)
protected override Exception GetValidationException(Guid value, string name)
protected override Exception GetValidationException(int[] value, string name)
protected override Exception GetValidationException(Entity value, string name)
protected override Exception GetValidationException(Entity[] value, string name)

...Or I'd find a way to make something like this work:

protected override Exception GetValidationException<TValue>(TValue value, string name)


sorting a map by converting it to vector I have a map that I want to print out sorted by value, I convert it to vector and sort the vector. Is this code correct?

#include <map>
#include <iostream>
#include <vector>
#include <algorithm>
#include <utility>
int main()
{
    std::map<char, int> freq;
    std::string text;
    std::getline(std::cin, text);
    std::vector<std::pair<char, int>> items;
    for(auto & ch: text)
            freq[ch]++;

    for(auto [key, value]: freq)
        items.push_back(std::make_pair(key, value));

    std::sort(items.begin(), items.end(),

              [](auto a, auto b)
                { return a.second > b.second;});

    for(auto [key, value]: items)
       std::cout << key << " " << value << std::endl;
    return 0;

}

The code is correct. However, I still have some recommendations:

Sort the includes, so you can easily spot recurring/missed ones

#include <algorithm>
#include <iostream>
#include <map>
#include <utility>
#include <vector>

You do not need to run the loop, you can simply pass the map to the std::vector constructor. As you use structured bindings and therewith at least C++17, I suggest Template Argument Deduction to omit the type of the vector

std::vector items(freq.begin(), freq.end());

The comparison function takes the arguments by copy, which is not optimal. Rather use const auto&:

[] (const auto& a, const auto& b) { return a.second > b.second;})

Note that std::sort may change the ordering of elements with equal value. If you want those elements with equal frequency to appear in the same order than in the map you would need std::stable_sort. However, keep in mind that this requires additional resources in memory and compute time.
You are using a std::map, which is an ordered container. If you are only interested in the frequencies then a std::unordered_map will generally offer better performance. Although for a simple alphabet this will most likely be negligible.