Fetching records, optionally including deleted records I have an original method that almost does what I want (a method I did not create), but now I need to find records that were deleted so I added another method GetAllGuideRecords minus the delete condition in the where clause.

But now I kind have repeated code so I decided to pack everything in one method, with an optional parameter. I sometimes like using optional parameters because it gives me the flexibility to not break existing code.

Should I create overload method instead, stick with what I have, or is there a better way?

Original code:

 public ED010_GUIDE GetNonDeletedGuideRecords(int guideId, int versionId)
        {
            var rec = ErgDb.ED010_GUIDE.FirstOrDefault(a => a.ERG_EDITION_ID == versionId && a.GUIDE_ID == guideId && a.DATE_DELETED_DTE == null);
            return rec;
        }

public ED010_GUIDE GetAllGuideRecords(int guideId, int versionId)
    {
        var rec = ErgDb.ED010_GUIDE.FirstOrDefault(a => a.ERG_EDITION_ID == versionId && a.GUIDE_ID == guideId);
        return rec;
    }

New code:

  public ED010_GUIDE GetGuideRecord(int guideId, int versionId, bool getDeletedAndActiveRecords )
        {

            var rec = ErgDb.ED010_GUIDE.FirstOrDefault(a => a.ERG_EDITION_ID == versionId && a.GUIDE_ID == guideId);

            if (!getDeletedAndActiveRecords)
            {
                return  (rec != null && rec.DATE_DELETED_DTE == null)? rec:null;
            }
            else
            {
                return rec;
            }

        }

Lots of ways to approach this one.  Here is how I would approach this (and why).

When working through decisions like this, I tend to think about several concerns:

Maintainability
Readability
Testability
Scalability

Maintainability

A core principal in S.O.L.I.D. programming is the idea of the "O" or the Open closed principal which states:

  Code should be open for extension and closed for modification.

In other words, I shouldn't have to change existing code to accomodate new functionality in an ideal world.

There are several reasons for this:

In some schools of thought, EVERY change is a breaking change (change == bad).
When changing existing code, all dependent code needs to be tested for regressions / compatibility and any derived code must be checked for consistency.
Classes should only change as a result of fixing bugs.

Readability

This is where opinions tend to vary widely, but IMO overloads are much easier to read.

When you use an overload, it allows you to specify an additional set of documentation:

/// <summary>
/// ...description stuff... This method differs from overloads because of x.
/// </summary>

I find this DRAMATICALLY helps readability (and maintainability) for future devs.  It also speaks to the initial intent of your authorship so the functions function is explicit and unlikely to be misunderstood.

Your duplication is also unnecessary since in your case the calls could be chained together:

public IQueryable<ED010_GUIDE> GetGuideRecords(int guideId, int versionId, bool includeDeleted)
{
    var rec = this.GetGuideRecords(guideId, versionId);

    if (!includeDeleted) {
        rec = from x in rec
              where x.DATE_DELETED_DTE == null
              select x;
    }

    return rec;
}

public IQueryable<ED010_GUIDE> GetGuideRecords(int guideId, int versionId)
{
    return from x in ErgDb.ED010_GUIDE
           where x.ERG_EDITION_ID == versionId && x.GUIDE_ID == guideId
           select x;
}

Testability

This one can really go either way in your case.  I would opt for the separation into separate methods most likely because it means that the setup logic for any unit tests would be much simpler as I don't have to account for this or other future scenarios breaking my tests.

Which leads me to my last thought.

Scalability

Or perhaps Flexibility is a better term.

The optional parameters way gets REALLY messy if additional circumstances continue to evolve in the future as you are setting a precedent of non-explicit method signatures.  Future DEVs or indeed yourself are likely to continue the pattern of fragile conditionals, broken unit tests and vague signature usage that can lead to bugs.

Recommendation

Ultimately, I would probably do two things:

Don't do either
Use a "unit-of-work" as an argument

I would go for something like this:

public virtual IQueryable<ED010_GUIDE> GetGuideRecords(GuideQueryRequest request)
{
    var rec = from x in ErgDb.ED010_GUIDE
              where x.ERG_EDITION_ID == request.VersionId && x.GUIDE_ID == request.GuideId
              select x;

    if (!request.IncludeDeletedRecords)
    {
        rec = from x in rec
              where x.DATE_DELETED_DTE == null
              select x;
    }

    return rec;
}

public class GuideQueryRequest
{
    public GuideQueryRequest()
    {
    }

    public int VersionId { get; set; }
    public int GuideId { get; set; }
    public bool IncludeDeletedRecords { get; set; }
}

The inclusion of the GuideQueryRequest type means that you can add new properties to your signature without having to change your signature and break all of the code that depends on it.

This strategy leverages the best aspects of maintainability, flexibility, and readability because you don't need to create additional overloads to supply new arguments, and there are no optional arguments to specify and create ambiguity.

It also gives you a canvas to add robust documentation to the GuideQueryRequest type on how it should be used and what the properties are used for.

In theory, whichever type defines the GetGuideRecord would mark that method as VIRTUAL so that derived types can override the functionality (if new properties are added to the GuideQueryRequest type) and not break the existing functionality.

EDIT

Based on all of the great feedback from @Heslacher, I have updated the above code examples to be correct.

I have also changed the signature of the methods to return IQueryable instances.  It depends on the needs of your application, but I like to have the option of performing additional sorting operations or filtering on a query before the query has been executed.  It also makes more sense given that the method doesn't know what the criteria of the GuideQueryRequest properties will be, it is impossible for this function to know whether or not multiple records (or indeed any records) will be returned.  This delegates the responsibility of how to use the query and the query results to the calling code.

I switched to Linq-to-SQL syntax because I like that format when querying a database and expression syntax when working in memory with results.

For more information on S.O.L.I.D. and a definition of letters and theory, check out these links:

http://www.blackwasp.co.uk/SOLIDPrinciples.aspx
http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod


Pythonic way to convert XML attribute to float, or else get None My Python code is converting XML attributes in an ElementTree to appropriate values. How do I best convert a value in this dict to a float() if it exists, or else get None?

Best I have now is:

attr = etree_elem.attrib
self.radius = float(attr['radius']) if 'radius' in attr else None
...
if self.radius:
  # do stuff using radius

I dislike this because it's not DRY (repeats both 'radius' and attr). I had code like this:

self.radius = float(attr.get('radius', -1))
...
if self.radius >= 0:
  # do stuff using radius

But (a) that only works when I know that a particular attribute value is otherwise invalid, and (b) it's not as "clean" to me as using None to properly signify that no radius was passed.

It's a bit difficult to get None without writing an explicit conditional.  The next best option may be to go with float('nan') instead of -1 to make it harder to accidentally use it as a value.

self.radius = float(etree_elem.attrib.get('radius', 'nan'))

You can use math.isfinite() to test the existence of a value.  The self.radius >= 0 test would also continue to work.


How could this "complex method" for codeclimate be refactored? I'm trying to figure out how to refactor some code based on what codeclimate is telling me.  So far it seems that any method that is longer than 5 lines or has an if statement is "complex". In this instance, I'm trying to create an event for a calendar based off of a meeting record.

Here is the Event model:

class Event < ActiveRecord::Base
  validates :title, :eventcolor_id, presence: true
  belongs_to :meeting
  belongs_to :eventcolor

  def self.create_from_meeting(meeting, eventcolor)
    meeting.meeting_date_end ||= meeting.meeting_date_start
    Event.find_or_initialize_by(meeting_id: meeting.id) do |e|
      e.title = meeting.title
      e.datetime_start = Time.zone.local_to_utc(meeting.meeting_date_start)
      e.datetime_end = Time.zone.local_to_utc(meeting.meeting_date_end)
      e.location = meeting.location
      e.address1 = meeting.address1
      e.address2 = meeting.address2
      e.city = meeting.city
      e.state = meeting.state
      e.zip = meeting.zip
      e.description = (!meeting.description.blank?) ? meeting.description : ''
      e.eventcolor_id = eventcolor
      e.save
      e.committees << meeting.committee if !meeting.committee.nil?
    end
  end
end

which is called from my meetings_controller with this line:

Event.create_from_meeting(meeting, params[:eventcolors_select])

A few things caught my eye:

You don't check whether e.save actually worked. If it fails, you'll have problems down the line.
meeting.meeting_date_end ||= meeting.meeting_date_start
Don't set an attribute on a record you don't "own"! In this case, a meeting object is being passed to the method to be copied - not altered. Use a local variable instead. It could be a nasty surprise to other parts of the code, to find that the meeting's been altered. Treat the meeting as strictly read-only.
Why are some of your meeting's date/time-attributes prefixed with the class name? It's redundant; they're attributes on a Meeting instance, so there's no reason to name them meeting_*. You also prefix event.datetime_start with the type, which isn't Ruby-like or Rails-like either. In both cases, something like starts_at and ends_at would be more than enough. It'd follow the convention set by updated_at and created_at and be immediately recognizable.
Speaking of, why are your meeting's date/time attributes defined in the local timezone? I'd advice you to store everything as UTC, so you don't have to worry about conversion (as you do in your current code). Because timezone conversion always ends up being painful.
e.description = (!meeting.description.blank?) ? meeting.description : ''
The parentheses aren't necessary, but moreover there's the present? method, which does what you want here: The opposite of blank?.
However, you could also just do e.description = meeting.description || '' since blank? triggers on nils and empty strings, but the latter is the default anyway, so you really only want to default in case of nil.
Similarly, this if !meeting.committee.nil? can be written a few other ways:

unless meeting.commitee.nil?
if meeting.commitee.present?
if meeting.commitee

The last one is the simplest and clearest, but all of them are, to my eyes, better than saying "if the committee is not not there"
My biggest concern is that this is a lot of data duplication - seemingly for no reason. Since Event already belongs to a Meeting, why duplicate anything? Everything should be available since you have event.meeting. Duplicating everything doesn't really make sense as long as you have the Meeting record. All you gain is the task of keeping two separate sets of data in sync whenever one changes. To me, this approach doesn't really make sense.

So I'd stick with:

event = meeting.events.create(eventcolor_id: params[:eventcolors_select])

and maybe add some accessor methods (decorators) to Event to give you the correct datetime_start and datetime_end. No need for the method at all.

Semantically, I'd prefer to have it flipped the other way, though: Make a meeting belong_to an event. A calendar event is a pretty generic entity: It can be anything with a time and (optionally) a place. A meeting, meanwhile, is a more specific thing. So to my mind, it'd make more sense to have a meeting attached to a calendar event, rather than the other way around.

Or you can do Single Table Inheritance with Event as the parent class. Or have an even more generic model - or models - the wrap the time and/or place, and have both Event and Meeting refer to that.

Basically, there are many ways to handle this without duplicating data.

Meanwhile, just looking at the code as-is, you can get rid of some lines:

# Note: May raise ActiveRecord::RecordInvalid
def self.create_from_meeting(meeting, eventcolor)
  Event.find_or_initialize_by(meeting_id: meeting.id) do |e|
    # copy all the identically-named attributes
    %w(title location address1 address2 city state zip description).each do |attribute|
      e.send("#{attribute}=", meeting.send(attribute))
    end
    e.datetime_start = Time.zone.local_to_utc(meeting.meeting_date_start)
    e.datetime_end = Time.zone.local_to_utc(meeting.meeting_date_end || meeting.meeting_date_start)
    e.eventcolor_id = eventcolor
    e.save! # raises an exception if it fails
    e.committees << meeting.committee if meeting.committee
  end
end

But it's still not very pretty, just shorter. Again, the main issue is the duplication.


Converting a 12h clock to 24h clock I started picking up Rust on my free time, and I'm curious about what style errors have I made and where should I improve as a programmer.

The problem is from HackerRank and very simple: there is a fixed-format input of time in 12 hour format (07:05:45PM), and the objective is to convert it to 24 hour format (19:05:45).

My own first impressions:

The problem is very simple, but the code seems too lengthy?
I'm nesting match constructs, and it doesn't seem concise.

use std::io::{stdin, BufRead, Error, ErrorKind, Result};

struct Time {
    hour: u8,
    minute: u8,
    second: u8
}

impl Time {
    pub fn from_12h(time : String) -> Result<Time> {
        assert!(time.chars().count() == 10);
        let hour : String = time.chars().take(2).collect();
        let minute : String = time.chars().skip(3).take(2).collect();
        let second : String = time.chars().skip(6).take(2).collect();
        let period : String = time.chars().skip(8).take(2).collect();

        match Time::hour_from_12h_to_24h(hour.parse::<u8>().unwrap(), period) {
            Ok(hour_24h) =>
                Ok(Time {
                    hour: hour_24h,
                    minute: minute.parse::<u8>().unwrap(),
                    second: second.parse::<u8>().unwrap()
                }),
            Err(e) => Err(e),
        }
    }

    fn hour_from_12h_to_24h(hour : u8, period : String) -> Result<u8> {
        match period.as_ref() {
            "AM" => Ok(hour % 12),
            "PM" => Ok(hour % 12 + 12),
            _ => Err(Error::new(ErrorKind::InvalidData, format!("invalid period {}", period))),
        }
    }

    pub fn to_24h(&self) -> String {
        format!("{:02}:{:02}:{:02}", self.hour, self.minute, self.second)
    }
}

fn main() {
    match read_12h_time() {
        Ok(time_12h) =>
            match Time::from_12h(time_12h) {
                Ok(time) => println!("{}", time.to_24h()),
                Err(err) => println!("error: {}", err)
            },
        Err(err) => println!("input error: {}", err),
    }
}

fn read_12h_time() -> Result<String> {
    let mut buffer = String::new();
    match read_line(&mut buffer) {
        Ok(length) =>
            match length {
                10 => Ok(buffer),
                _ => Err(Error::new(ErrorKind::InvalidInput, format!("expected 10 characters, got {}", length)))
            },
        Err(e) => Err(e),
    }
}

fn read_line(buffer : &mut String) -> Result<usize> {
    let stdin = stdin();
    let mut handle = stdin.lock();
    handle.read_line(buffer)
}

Your code doesn't really work when used by humans. The first thing I did was run the code and type in a time, which failed. When a human enters data through the terminal, they will hit Enter, which is returned from read_line:

$ echo '07:05:45PM' | cargo run
input error: expected 10 characters, got 11
$ echo -n '07:05:45PM' | cargo run
19:05:45

Style issues - learn to love rustfmt:

There's no space before : when specifiying a type on a variable.
Use trailing commas.
Match arms longer than one line should use curly braces {}

Accept a &str instead of String; the function doesn't need to take ownership of the string.
There's no need for the turbofish (::<>) on the parse calls; type inference handles that just fine.
Become very familiar with all the methods on Option and Result and Iterator. These are the bread and butter of idiomatic, concise, and performant Rust code. Specifically, if you match on a Result and have a match arm for Ok and Err, it's likely you want to use Result::map.
Avoid iterating through the characters from the beginning multiple times, it is less efficient than needs to be. Instead, use the same iterator multiple times through Iterator::by_ref.
I'd use expect in your main method, instead of matching and printing. There is a difference between the two, expect will cause a panic on Err, but that seems reasonable in main.
I never use unwrap, instead preferring at least expect; this helps find the line when the code finally fails.
Library code should very rarely panic; they should report errors. Especially report errors when it's because of input supplied to the code, panic for internal logic errors.
The match in read_12h_time can be flattened; you can pattern match against Ok(10) directly.
stdin doesn't need to be explicitly locked; read_line will lock it.

use std::io::{stdin, Error, ErrorKind, Result};

struct Time {
    hour: u8,
    minute: u8,
    second: u8,
}

impl Time {
    pub fn from_12h(time: &str) -> Result<Time> {
        assert!(time.chars().count() == 10);
        let mut chars = time.chars();

        let hour: String = chars.by_ref().take(2).collect();
        let minute: String = chars.by_ref().skip(1).take(2).collect();
        let second: String = chars.by_ref().skip(1).take(2).collect();
        let period: String = chars.by_ref().take(2).collect();

        Time::hour_from_12h_to_24h(hour.parse().unwrap(), &period).map(|hour_24h| {
            Time {
                hour: hour_24h,
                minute: minute.parse().unwrap(),
                second: second.parse().unwrap(),
            }
        })
    }

    fn hour_from_12h_to_24h(hour: u8, period: &str) -> Result<u8> {
        match period {
            "AM" => Ok(hour % 12),
            "PM" => Ok(hour % 12 + 12),
            _ => Err(Error::new(ErrorKind::InvalidData, format!("invalid period {}", period))),
        }
    }

    pub fn to_24h(&self) -> String {
        format!("{:02}:{:02}:{:02}", self.hour, self.minute, self.second)
    }
}

fn main() {
    let time_12h = read_12h_time().expect("input error");
    let time = Time::from_12h(&time_12h).expect("error");

    println!("{}", time.to_24h());
}

fn read_12h_time() -> Result<String> {
    let mut buffer = String::new();
    match read_line(&mut buffer) {
        Ok(10) => Ok(buffer),
        Ok(length) => {
            Err(Error::new(ErrorKind::InvalidInput,
                           format!("expected 10 characters, got {}", length)))
        }
        Err(e) => Err(e),
    }
}

fn read_line(buffer: &mut String) -> Result<usize> {
    stdin().read_line(buffer)
}

On to bigger issues:

I don't like that the AM/PM matching and hour conversion are tangled together.

Instead, introduce an enumeration to handle the AM/PM.

It feels like 12 hour time is a different type from 24 time.

Rename existing type to something unique.
Store the new AM/PM enumeration.
Introduce new type and add conversion methods.

I don't like reusing io::Error for your own errors.

Introduce a custom error type. I like to use quick_error, but there are other solutions.

I don't like that IO is embedded inside of the logic of the library.

Separate IO and parsing logic and only combine them at a higher level - in this case main.
Push time-specific logic (e.g. 10 character checks) back into the library. Note that this removes the duplication of the logic in the assert and removes a panic inside the library.

The code doesn't fully participate in the Rust ecosystem.

Implement traits like FromStr and From and Display (perf!).
Derive standard traits to make it easier to use your types: #[derive(Debug, Copy, Clone, PartialEq)] is a good set.

There are too many panics inside the library portion.

Fully utilize Result and try! or the try operator ?.

There's too much unneeded allocation.

Instead of using characters and collecting them back to strings, operate on the incoming string and take slices instead.
Formatting can write directly into a formatter; there's no reason to allocate a string to just turn around and print it.

#[macro_use]
extern crate quick_error;

use std::str::FromStr;
use std::io::stdin;
use std::fmt;

quick_error! {
    #[derive(Debug, Clone, PartialEq)]
    enum Error {
        MissingMeridian {
            description("missing the meridian")
            display("Missing the meridian")
        }
        InvalidMeridian(what: String) {
            description("invalid meridian")
            display("Invalid meridian: {}", what)
        }
        MissingComponent(which: TimeComponent) {
            description("missing a time component")
            display("Missing {}", which)
        }
        InvalidComponent(which: TimeComponent, why: std::num::ParseIntError) {
            description("invalid time component")
            display("Invalid {}: {}", which, why)
            cause(why)
        }
    }
}

type Result<T> = std::result::Result<T, Error>;

#[derive(Debug, Copy, Clone, PartialEq)]
enum Meridian {
    AnteMeridian,
    PostMeridian,
}

#[derive(Debug, Copy, Clone, PartialEq)]
enum TimeComponent {
    Hour,
    Minutes,
    Seconds,
}

impl fmt::Display for TimeComponent {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use TimeComponent::*;

        let name = match *self {
            Hour => "hour",
            Minutes => "minutes",
            Seconds => "seconds",
        };
        write!(f, "{}", name)
    }
}

impl FromStr for Meridian {
    type Err = Error;

    fn from_str(s: &str) -> Result<Meridian> {
        match s {
            "AM" => Ok(Meridian::AnteMeridian),
            "PM" => Ok(Meridian::PostMeridian),
            _ => Err(Error::InvalidMeridian(s.to_string())),
        }
    }
}

#[derive(Debug, Copy, Clone, PartialEq)]
struct TwelveHourTime {
    hour: u8,
    minutes: u8,
    seconds: u8,
    meridian: Meridian,
}

impl FromStr for TwelveHourTime {
    type Err = Error;

    fn from_str(time: &str) -> Result<TwelveHourTime> {
        use Error::*;
        use TimeComponent::*;

        let mut parts = time.splitn(3, ":");

        let hour = parts.next().ok_or(MissingComponent(Hour))?;
        let minutes = parts.next().ok_or(MissingComponent(Minutes))?;
        let seconds_meridian = parts.next().ok_or(MissingComponent(Seconds))?;

        if seconds_meridian.len() < 4 {
            return Err(MissingMeridian);
        }

        let (seconds, meridian) = seconds_meridian.split_at(2);

        let hour = hour.parse().map_err(|e| InvalidComponent(Hour, e))?;
        let minutes = minutes.parse().map_err(|e| InvalidComponent(Minutes, e))?;
        let seconds = seconds.parse().map_err(|e| InvalidComponent(Seconds, e))?;
        let meridian = meridian.parse()?;

        Ok(TwelveHourTime {
            hour: hour,
            minutes: minutes,
            seconds: seconds,
            meridian: meridian,
        })
    }
}

#[derive(Debug, Copy, Clone, PartialEq)]
struct TwentyFourHourTime {
    hour: u8,
    minutes: u8,
    seconds: u8,
}

impl From<TwelveHourTime> for TwentyFourHourTime {
    fn from(other: TwelveHourTime) -> Self {
        let hour = match other.meridian {
            Meridian::AnteMeridian => other.hour,
            Meridian::PostMeridian => other.hour + 12,
        };

        TwentyFourHourTime {
            hour: hour,
            minutes: other.minutes,
            seconds: other.seconds,
        }
    }
}

impl fmt::Display for TwentyFourHourTime {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{:02}:{:02}:{:02}", self.hour, self.minutes, self.seconds)
    }
}

fn main() {
    let mut time = String::new();
    stdin().read_line(&mut time).expect("Unable to read input");

    let time: TwelveHourTime = time.trim().parse().expect("Unable to parse time");
    let time: TwentyFourHourTime = time.into();

    println!("{}", time);
}

  The problem is very simple, but the code seems too lengthy?

I don't suppose I made the length any better! However, I feel that the final code is pretty efficient, and when you start taking error cases into account, I would disagree that the problem is inherently "simple". Parsing text is complicated. You can use tools like regular expressions or parser combinators / generators, but those are just abstractions - the complexity is just hidden from view.

There's still logical aspects here that are strange. For example, the original code does hours % 12, which seems wrong, and my modified code does not, which also seems wrong! It's probably better to throw an error for invalid hourly values.


Resets Rows to default values if column A is missing a value I have this Subroutine that works, but I have recently started updating the sheet and adding new columns. I'm finding updating the current macro to be a bit of a chore.

Is there a way to use named ranges instead of declaring the cell ranges as in this code?

    Private Sub Reset_Rows_Click()
        Const USED_RANGE    As String = "A2:FI5002"
        Const BLANKS_COLS   As String = "A2:A5002,B2:B5002,F2:G5002,K2:K5002,M2:O5002,V2:Y5002,AA2:AB5002,AE2:AM5002,AU2:AW5002,AY2:AY5002,BA2:BD5002,BG2:BL5002,BN2:BU5002,CL2:CO5002,CX2:CX5002,DF2:FI5002"
        Const SELECT_COLS   As String = "H2:I5002,J2:J5002,P2:U5002,Z2:Z5002,AC2:AD5002,AN2:AT5002,AX2:AX5002,AZ2:AZ5002,BE2:BF5002"
        Const NA_COLS       As String = "E2:E5002"
        Const BLANKS_VAL    As String = vbNullString
        Const SELECTS_VAL   As String = " --Select--"
        Const NA_VAL        As String = "N/A"

        Dim ws As Worksheet, ur As Range

        Set ws = ActiveSheet
        Set ur = ws.Range(USED_RANGE)

        Application.ScreenUpdating = False
        With ur
            .AutoFilter Field:=1, Criteria1:="="
            If .SpecialCells(xlCellTypeVisible).Count > 1 Then
                .Range(BLANKS_COLS).Value2 = BLANKS_VAL
                .Range(SELECT_COLS).Value2 = SELECTS_VAL
                .Range(NA_COLS).Value2 = NA_VAL
           End If
                .AutoFilter
           End With
                'ws.Cells(5003, 1).EntireRow.Delete

'Sort
    Range("A3:FS5002").Select
ActiveWorkbook.Worksheets("Master").Sort.SortFields.Clear
ActiveWorkbook.Worksheets("Master").Sort.SortFields.Add Key:=Range( _
    "E3:E5002"), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
    xlSortNormal
ActiveWorkbook.Worksheets("Master").Sort.SortFields.Add Key:=Range( _
    "L3:L5002"), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
    xlSortNormal
ActiveWorkbook.Worksheets("Master").Sort.SortFields.Add Key:=Range( _
    "A3:A5002"), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
    xlSortNormal
With ActiveWorkbook.Worksheets("Master").Sort
    .SetRange Range("A3:FS5002")
    .Header = xlNo
    .MatchCase = False
    .Orientation = xlTopToBottom
    .SortMethod = xlPinYin
    .Apply
End With

Application.ScreenUpdating = True
End Sub

A couple of things -

What is Range("A3:FS5002").Select doing? It seem superfluous. Be sure to avoid things like .Select - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this - how to avoid using select in vba.

You use ActiveWorkbook.Worksheets("Master") five times. Why not make it a variable like

Dim masterSheet As Worksheet
Set masterSheet = ThisWorkbook.Sheets("Master")

Then replace all those instances with the variable?

Speaking of variables, ws and ur aren't very descriptive and it costs nothing to make them mean something like dataSheet and dataSheetUsedRange.

Yes, you can use named ranges from the worksheet as named ranges in VBA. That's what you want to do and it's definitely possible and cleaner. Honestly making the ranges be strings that have to be passed to a range method is, pretty confusing. If you need to get the last row each time instead of changing it, you can use -

    Dim lastRow As Long
    lastRow = mySheet.Cells(Rows.Count, A).End(xlUp).Row

Now you can just use the column letters to create your ranges like -

    Set myUsedRange = mySheet.Range("A2:A" & lastRow)

and so on.

You could also use a union like this -

    Dim blankRange As Range
    set blankrange = application.Union(Range("A2:A" & lastRow), Range("B2:B" & lastRow), etc)

Or use the Union with your named ranges

    Set blankRange = Application.Union(Range("BlankB"), Range("BlankFtoG"), Range("BlankK"), Range("BlankMtoO"))

Using the Union method is only one of the ways to do it, say if you had each part of the range as a different name or selection.

Let's talk about named ranges. When you name a range you first select the range in the worksheet, so say you've selected the cells in

H2:I5002,J2:J5002,P2:U5002,Z2:Z5002,AC2:AD5002,AN2:AT5002,AX2:AX5002,AZ2:AZ5002,BE2:BF5002

Now you go up to the spot next to the formula bar and type in a name SelectColumns. Now you can use a formula like =SUM(SelectColumns) on the worksheet.

In VBA you can use the range as well, like this -

Set mySelectRange = dataSheet.Range("SelectColumns")

If, say, you still wanted to use a string then you wouldn't need the quotation marks

Dim rangeToSelect As String
rangeToSelect = "SelectColumns"
Set myRange = dataSheet.Range(rangeToSelect)

I think for some reason you've been misguided that you need to use strings for named ranges. Strings can be used, but if you already have the ranges named, just use the built in name. You can cut out a lot of the constants you have.

If you want to define a name in VBA you can like this -

Sub NameIt()
    Dim myRange As Range
    Set myRange = Range("A1:A5")
    myRange.Name = "SelectColumns"
End Sub

But, as you can see, that's a lot more work by typing it rather than selecting them with the mouse. Something like the NameIt above would only be useful if you needed to define the ranges every time the sub ran, say by using the lastRow from above. You'd need to clear all the names, then assign them again with the relative lastRow.

Here's an example of a non-contiguous named range name in a worksheet -


Simple web alarm clock I decided to strengthen my "vanilla JavaScript" skills since I've lately been getting more into jQuery and just discovered the potential of node.js.

As a result, I decided to make a basic alarm clock in javascript and html. (Plus I needed an alarm clock).

Here's the Fiddle

// Load an alarm sound
    var sound = new Audio("http://www.freespecialeffects.co.uk/soundfx/sirens/alarm_01.wav");
    sound.loop = true;

// I just made this because It's easier to read.
getID = function(value){ return document.getElementById( value ); };

// List of all my objects im working with, and variables
var hour = getID("hour"),
    minute = getID("minute"),
    second = getID("second"),
    aHour = getID("aHour"),
    aMinute = getID("aMinute"),
    aSecond = getID("aSecond"),
    aSwitch = getID("aSwitch"),
    aOff = getID("turnOff"),
    refreshTime = 500,
    alarmTimer = null;

aSwitch.On = false;
aSwitch.value = "OFF";

// Turns the alarm off or on
function alarmSwitch(){
    switch( aSwitch.On )
    {
        case false:
            aSwitch.On = true;
            aSwitch.value = "ON";

            alarmSet();
        break;
        case true:
            aSwitch.On = false;
            aSwitch.value = "OFF";

            // CLEARS THE BEEPER
            clearTimeout( alarmTimer );
        break;
    }
}

// Stops the alarm and closes the "stop button"
function disableAlarm(){
    sound.pause();
    aOff.style.display = "none";
}

//Fires the BEEPER noise, -- this is called from the alarmTimer timeout
function alarmFire(){
    if( aSwitch.On )
    {
        aOff.style.display = "block";
        sound.play();
    }
    else
        alert("error..");
}

/*This is how the beeper goes off:
 * It first checks the set time so the alarm can go to the
 * next day without the user putting the date in.
 * The beeper goes off over the span of milliseconds difference.
 */
function alarmSet(){
    clearTimeout( alarmTimer );

    var tomo = false;// tomorrow.
    if( aHour.value < hour.value )
        {tomo = true;}
    else if( aHour.value == hour.value && aMinute.value < minute.value )
        {tomo = true;}
    else if( aHour.value == hour.value && aMinute.value == minute.value
            && aSecond.value < second.value )
        {tomo = true;}

    var date = new Date(), year = date.getFullYear(), month = date.getMonth(), day = parseInt( date.getDate() );

    if( tomo ){day += 1;}

    time = new Date( year, month, day, aHour.value, aMinute.value, aSecond.value, date.getMilliseconds() );
    time = ( time - (new Date()) );

    // This turns the alarm back on if it's off when this function is called.
    if( aSwitch.On == false)
        alarmSwitch();

    alarmTimer = setTimeout( function(){alarmFire();} ,parseInt(time) );
}

// This is how the clock works
timeRefresh = function(){
    date = new Date();
    hour.innerHTML = date.getHours();
    hour.value = hour.innerHTML;
    minute.innerHTML = date.getMinutes();
    minute.value = minute.innerHTML;
    second.innerHTML = date.getSeconds();
    second.value = second.innerHTML;

    setTimeout("timeRefresh()", refreshTime);
};

// This is called whenever the inputs are changed to stop invalid forms.
numCap = function( obj, min, max){
    obj.value = Math.max(obj.min, Math.min(obj.max, obj.value) );

    alarmSet();// Starts up the alarm automatically when a value is changed.
};

window.onload=function(){
    timeRefresh();

    var a = getID("aHour");
    a.value = hour.innerHTML;
    a = getID("aMinute");
    a.value = minute.innerHTML;
    a = getID("aSecond");
    a.value = second.innerHTML;
};

I would love for some feedback on my code! What could I do to improve/simplify the code?

Also, if you would like to see the HTML page along with it, just ask!

This is not a complete review, just a few things that jumped out to me:

getID()

Looks more like "easier to write" than "easier to read". From current function name "getID", most people reading the code will assume that the function will return an ID.

If you would like to use jQuery as you can simply write $("#hour") instead of getID("hour"). The core idea of jQuery is to let you select (or query) DOM elements.

alarmSwitch()

If you use simple if/else instead of the switch/case, the code will be much simpler.

if(aSwitch.On){
...
} else {
...
}

Variable Naming

Simply naming the variable tomorrow instead of tomo would have been better for read

var tomo = false;// tomorrow.

If/else blocks for single line

Consistently using if/else blocks, even for single line statements helps avoid hard to find bugs

if( aSwitch.On )
{
    aOff.style.display = "block";
    sound.play();
}
else
    alert("error..");


Music categorization database Recently I've tried to teach myself basic things about MySQL databases, however I received some help on Stack Overflow with creating a good database structure trying to avoid redundancy as much as possible.

I finally came up with the database structure below and would like to know if that's the way to go and if not, what you would improve/change to it.

CREATE TABLE artists (
    artist_id INT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    artist_name varchar(100) UNIQUE,
    artist_aka varchar(255) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE labels (
    label_id SMALLINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    label_name varchar(100) UNIQUE,
    label_aka varchar(255) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE producers (
    producer_id SMALLINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    producer_forename varchar(100),
    producer_nickname varchar(100) UNIQUE,
    producer_lastname varchar(100)
) ENGINE = 'InnoDB';

CREATE TABLE years (
    year_id TINYINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    year_value varchar(4) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE genres (
    genre_id TINYINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    genre_name varchar(10) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE flags (
    flag_id TINYINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    flag_name varchar(12) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE tags (
    tag_id SMALLINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    tag_name varchar(16) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE sources (
    source_id TINYINT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    source_name varchar(30) UNIQUE
) ENGINE = 'InnoDB';

CREATE TABLE riddims (
    riddim_id INT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    riddim_name varchar(40) UNIQUE,
    riddim_aka varchar(255) UNIQUE,
    genre_fk TINYINT,
    youtube varchar(11) UNIQUE,
    image varchar(11) UNIQUE,
    FOREIGN KEY (genre_fk) REFERENCES genres(genre_id) ON DELETE SET NULL ON UPDATE CASCADE
) ENGINE = 'InnoDB';

CREATE TABLE tunes (
    tune_id INT AUTO_INCREMENT NOT NULL PRIMARY KEY,
    riddim_fk INT DEFAULT NULL,
    artist_fk INT DEFAULT NULL,
    tune_name varchar(60) DEFAULT NULL,
    tune_aka varchar(255) DEFAULT NULL,
    label_fk SMALLINT DEFAULT NULL,
    producer_fk SMALLINT DEFAULT NULL,
    year_fk TINYINT DEFAULT NULL,
    lyrics TEXT DEFAULT NULL,
    flag_fk TINYINT,
    tag_fk SMALLINT,
    source_fk TINYINT,
    last_modified TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
    FOREIGN KEY (riddim_fk) REFERENCES riddims(riddim_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (artist_fk) REFERENCES artists(artist_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (producer_fk) REFERENCES producers(producer_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (year_fk) REFERENCES years(year_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (flag_fk) REFERENCES flags(flag_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (tag_fk) REFERENCES tags(tag_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (source_fk) REFERENCES sources(source_id) ON DELETE SET NULL ON UPDATE CASCADE
) ENGINE = 'InnoDB';

Later the website will contain information about "riddims" (rythms), artists that have a song on that riddim, songnames, release years etcetera.
I guess the best way to show what its supposed to do is showing you a first, unfinished version of the website.

Just a couple of suggestions.

First, artists and producers are both persons and a person can be both an artist and a producer (even on their own songs).

Second, more than one persons can work together on a song, but in lots of different roles (backup musician, backup singer, "featuring", "uncredited", or as a member of a groups). So performers and roles would probably look something like:

CREATE TABLE roles (
    role_id INT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    description varchar(100) not null
) ENGINE = 'INNODB';

CREATE TABLE performers (
    performer_id INT NOT NULL AUTO_INCREMENT PRIMARY KEY,
    tune_fk INT NOT NULL,
    person_fk INT NOT NULL,
    role_fk INT NOT NULL,
    FOREIGN KEY (tune_fk) REFERENCES tunes(tune_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (person_fk) REFERENCES persons(person_id) ON DELETE SET NULL ON UPDATE CASCADE,
    FOREIGN KEY (role_fk) REFERENCES roles(role_id) ON DELETE SET NULL ON UPDATE CASCADE,
) ENGINE = 'InnoDB';


Meta-programming template for collection statistics I find that I often want to calculate summary statistics from collections of data and decided to create a template to do that.  I'm interested in a general review, but I also have some specific questions.

Is the use of the lambda "too clever?"
Is std::accumulate OK?  Specifically, I'm thinking of the potential for overflow for large collections of small data types.
Is the template missing anything?
Should I be using something such as std::enable_if to assure that the Iter type is actually an iterator?

The code to be reviewed is in stats.h but stats.cpp shows how it can be used.

stats.h

#ifndef STATS_H
#define STATS_H
#include <cmath>
#include <numeric>
#include <iterator>

template <typename T, typename Iter>
struct Statistics {
    Statistics(Iter start, Iter end, bool sample=true) :
        n(std::distance(start, end)),
        sum(std::accumulate(start, end, 0)),
        mean(sum / n),
        variance([&start, &end, sample, this]()->T {
                T sumsq = 0;
                for (auto it=start; it != end; ++it) {
                    sumsq += (*it-mean)*(*it-mean);
                }
                return sumsq/(n- (sample ? 1 : 0));
            }()
        ),
        sdev(sqrt(variance))
    {
    }
    unsigned n;
    T sum;
    T mean;
    T variance;
    T sdev;
};
#endif //STATS_H

stats.cpp

#include <iostream>
#include <vector>
#include <list>
#include "stats.h"

template <typename T>
std::ostream &print(std::ostream &out, const T &stats) {
    out << "n = " << stats.n << '\n';
    out << "sum = " << stats.sum << '\n';
    out << "mean = " << stats.mean << '\n';
    out << "var = " << stats.variance << '\n';
    out << "sdev = " << stats.sdev << '\n';
    return out;
}

int main()
{
    std::vector<int> sample{9, 2, 5, 4, 12, 7};
    Statistics<float, decltype(sample.cbegin())> stats(sample.cbegin(), sample.cend());
    print(std::cout, stats);

    std::list<float> sample2{2.0, 4.0, 5.0, 7.0, 9.0, 12.0};
    Statistics<double, decltype(sample2.cbegin())> stats2(sample2.cbegin(), sample2.cend());
    print(std::cout, stats2);
}

Sample output

n = 6
sum = 39
mean = 6.5
var = 13.1
sdev = 3.61939
n = 6
sum = 39
mean = 6.5
var = 13.1
sdev = 3.61939

Use of T for accumulator and result

I question the decision to use a template parameter T as the result and accumulator type. Especially in the calculation of the variance, using T as accumulator and mean value will cause significant errors in the calculated standard deviation and variance. The mean value may be off by at most \$\lim_{x\rightarrow 1}x\$. I cannot think of any situation where if you are interested in statistics, you would want to use anything else than double.

Clever lambda

While the lambda trick is easy to understand, I question the necessity of it when the same code could have been implemented in the constructor body and with less typing. This would also allow to use double for the mean internally and only truncate to T after assigning, this would bound the error on the variance and standard deviation. Edit: at least if the result is within the domain of T.

Use of std::accumulate

For small types, say short the risk of overflow is large. For double you run the risk of truncation errors when there are many samples. For accuracy see my answer here: Function that normalizes a vector of double. The same applies here and depends on the level of accuracy that you require, seeing as it is statistics, I wouldn't be too worried.

What about min/max value?

One stat that I'm missing is the maximum and minimum value.

Performance

What if I don't need the standard deviation but only the mean? You might want to implement the sum, mean and stdev as separate classes so the user can choose if she only needs the mean for example.

Naming

I find the name Statistics a bit too generic. The name may suggest statistics about anything (like for example memory usage for the day) but here it means the sample count, sum, mean, variance and standard deviation.

Documentation

The variable sample wasn't immediately obvious that it was used to distinguish between sample and population statistics. A comment for the constructor explaining the arguments might be in order.

Checking for iterator type

The compiler will emit an error if the type used for iterator isn't suitable, so strictly speaking this isn't really necessary if it complicates the code. Also I'm not completely sure how you would test that a fulfills the iterator concept in a useful way. I mean you can test for the existence of std::iterator_trait<Iter>::iterator_category but that can have false negatives.

But I never turn down a nice error message telling me what I'm doing wrong instead of a horrendous template substitution failure wall of text that MSVC emits... :)


Find pairs in an integer array whose sum is equal to 10 I came up with 2 different solutions in Java.

First one is:

public static void main(String[] args) {

    int[] array = new int[]{10,3,4,6,1,9,10,0};
    int sum=0;

    for (int i=0;i<array.length;i++)
    {
      for (int j=i ; j<array.length;j++){

        sum = array[i] + array[j];
        if (sum==10){

          System.out.println(array[i] + " " + array[j]);

        }

      }

    }

I believe this one works in \$O(N^2)\$.

My second solution is:

  public static void main(String[] args) {
    int[] nums = new int[] {10, 3, 4, 6, 1, 9, 10, 0};
    Hashtable<Integer, Integer> reqNoList = new Hashtable<Integer, Integer>();
    int sum = 10;
    int i = 0, count = 0;

    while (i < nums.length && i < sum) {
      int key = sum - nums[i];
      if (reqNoList.containsKey(nums[i])) {
        Integer temp = reqNoList.get(nums[i]);
        System.out.println(key + " " + nums[i]);
        count++;
      } else {
        reqNoList.put(key, i);
      }
      i++;
    }
  }
}

I tried to do it in \$O(N)\$ but I have no idea.

The O(n²) approach

The first algorithm is indeed O(n²), because, for every element in the array, the array is traversed again to see whether an element after the current element is equal to 10 minus it. It is quite clear, and there is not much to change. Comments are:

There is a bug: you're checking the same element twice in a row, since j, in the inner loop, starts at i. Therefore, if you consider the array { 1, 5, 2 }, the pair (5,5) will be considered a solution, when 5 only appeared once in the input array. To fix this, j should start at i + 1.
Indent properly the code. As written, the curly braces are not aligned correctly, making it more difficult to read than it needs to be. A quick reformat by an IDE will produce

int[] array = new int[] { 10, 3, 4, 6, 1, 9, 10, 0 };
int sum = 0;

for (int i = 0; i < array.length; i++) {
    for (int j = i; j < array.length; j++) {
        sum = array[i] + array[j];
        if (sum == 10) {
            System.out.println(array[i] + " " + array[j]);
        }
    }
}

which is easier to go through.
There is no need to declare the sum variable outside the two for loops. This variable is only used inside to calculate the sum of two elements of the array. As a usual guideline, local variables should have the smallest possible scope, so in this case, you would prefer to have:

int sum = array[i] + array[j];

and remove the int sum = 0 before the loop.

The O(n) approach

The second algorithm is, as you say, O(n). The difference in approach with regard to the first, is that instead of traversing the array a second time for each element, we memorize along the way the difference between 10 and the current element. This means that we don't need to go through every element again, for each element, to check whether one satisfies; we can just continue and if we find one that we memorized, it means we just found a pair. A couple of comments again:

There is a bug in the boundary of the while loop: why mandate i < sum? Because of this, not every element in the array will be considered (only the first sum ones), and pairs will be missed. A simple example is the array { 0, 1 } with a sum to search of 1. Once fixed, you can even turn this into a classic for loop, with an index going from 0 to nums.length excluded.
Hashtable is considered an obsolete class. Not officially deprecated, but it has been superseded with faster solutions, like HashMap and ConcurrentHashMap. Since thread-safety is not a concern here, you can safely switch to HashMap.
Remove the unused code. The variable count is declared, and incremented, but never used. In the same way, the variable temp is declared and never used.
Notice how you're not using the value of the map. The code is doing reqNoList.put(key, i);, but then the stored i is never used. You could accomplish the same with a just HashSet instead of an HashMap.

They are not equivalent

But be careful, because there is a fundamental difference between the two algorithms, namely in how they handle duplicate pairs. Consider the array { 4, 4, 1, 0 }, and a sum to search of 5. The first algorithm will print two solutions, whereas the second one will just print one. So depending on what you want, there might be adjustements.

In you want to print them all using the second approach, you could do it by utilizing the value of the HashMap, and make it store the number of times a given element has been encountered.

As a final note, you should consider extracting each approach into their own method, like printPairsHavingSum(int[] array, int sum). It would clarify even more the code.


Cleaning up a class that contains the object it represents I've recently started writing a tic-tac-toe game (following the Odin project) and am trying to write a class to hold the board game.

What I'm not sure about, is that the board class holds all the data related to the board in a variable called @board. Now after thinking about it I decided that the Board class shouldn't know about the game logic, and just hold the board itself, but now I'm not sure if it's redundant or not.

Main problem is that right now, when I try to add logic to the Game class, I get a function that looks like this:

class Game
  .
  def is_over?
    board = @board.board
    .
    .
  end
end

Which just feels wrong...
my current implementation for it is like so:

module TicTacToe
  class Board
    attr_reader :board
    def initialize(size)
      @board = create_empty_board(size)
      @size = size
    end

    def print_board
      @size.times { print "-----" }
      puts
      @board.each do |row|
        row.each do |c|
          print "| #{c} |"
        end
      puts
      @size.times { print "-----" }
      puts
      end
    end

    def make_move(move, sym)
      if board[x][y] == ' '
        board[x][y] = sym
      else
        raise ArgumentError, "Invalid, cell not empty"
      end
    end

  private
    def create_empty_board(size)
      board = Array.new
      size.times do
        row = Array.new
        size.times do
          row << ' '
        end
        board << row
      end
      board
    end
  end
end

Yes, your board.board has a code smell, but it is hard to say what to do without knowing Game.

Actually Board#board is an array. I would recommend to add methods to Board that allow you to use the instance of Board instead of Board#board.

Example: To get a row of yor board you can use:

class Board
  ...
  def [](row) #Access to one row
    @board[row]
  end
  def each_row #loop on all rows...
    @board.each{|row| yield row }
  end

end

class Game
  ...
  def is_over?
    @board.each_row{|row|
     row.each{|field| } ##your check
    }
    ...
  end
end


Is this an optimal implementation for the knapsack algorithm? I have implemented a solution for the knapsack problem while breaking an item is allowed. I've used the dictionary data structure where I used weight as a key and value per weight as a value. After that, I sorted the dictionary by value and filled the knapsack until there's no space left.

Here is my python code -

def calculateKnapsack():
    userinputNW = list(map(int, input().split()))
    n = userinputNW[0]
    weight = userinputNW[1]
    weightvaluedict = {}
    revenue = 0
    for x in range(n):
        vw = list(map(int, input().split()))
        weightvaluedict[vw[1]] = vw[0]/vw[1]
    weightvaluedict = {k: v for k, v in sorted(
        weightvaluedict.items(), key=lambda item: item[1], reverse=True)}
    for k, v in weightvaluedict.items():
        if weight == 0:
            break
        if k < weight:
            revenue += k * v
            weight -= k
        else:
            revenue += weight * v
            weight = 0
    return revenue

I'm confused if it is an optimal solution or not. I searched online and most of the tutorial used 2D array. I wanted to ensure my code's optimality. Suggestions are welcome.

Thanks. And also this is my first question on the Code review platform so please forgive me if I did anything wrong.

I don't understand what you mean by "the knapsack problem white fraction is allowed," not even if I assume that the word "white" should have been "while" or "what" or "where". The whole thing that makes the knapsack problem NP-hard is that you're not allowed to break up a single item into "fractions." So to understand what you meant, I tried to look at the code.

The first thing I do to understand a piece of code is, I look at its function signature. What arguments does the function take? What verbs and prepositions are used in its name? Consider:

def countPrimesUpTo(n):
def listPrimesUpTo(n):
def printPrimesLessThan(n):

Here's your function signature.

def calculateKnapsack():

Well, that didn't give me any information at all...!

The second way I try to understand a piece of code is to look at what it does; that is, look at its unit-tests.

assert listPrimesUpTo(10) == [2,3,5,7]
assert listPrimesUpTo(5) == [5]
assert listPrimesUpTo(1) == []

Here's your unit tests.

pass

Okay, I guess I should just give up on understanding what the code is intended to do, and focus on the code itself.

    userinputNW = list(map(int, input().split()))
    n = userinputNW[0]
    weight = userinputNW[1]

Oh hey! Here's something that looks like function arguments. You could immediately refactor your function into

def computeKnapsack(n, weight):

But then I go on and I see there's more input happening later...

    weightvaluedict = {}
    for x in range(n):
        vw = list(map(int, input().split()))
        weightvaluedict[vw[1]] = vw[0]/vw[1]
    weightvaluedict = {k: v for k, v in sorted(
        weightvaluedict.items(), key=lambda item: item[1], reverse=True)}

So now our knapsack function looks like

def computeKnapsackRevenue(weights, capacity):
    revenue = 0
    for size, value in weights.items():
        if capacity == 0:
            break
        if size < capacity:
            revenue += size * value
            capacity -= size
        else:
            revenue += capacity * value
            capacity = 0
    return revenue

and we have some boilerplate user-input code that looks like

    num_items, capacity = map(int, input().split())
    weights = {}
    for x in range(num_items):
        v, k = map(int, input().split())
        weights[k] = v / k
    weights = {k: v for k, v in sorted(
        weights.items(), key=lambda kv: kv[1], reverse=True)}

That last line isn't doing anything at all. It doesn't matter how you permute the list of items, if all you're doing with the result is stuffing it back into an unsorted dict. So we can erase that line.

    num_items, capacity = map(int, input().split())
    weights = {}
    for x in range(num_items):
        v, k = map(int, input().split())
        weights[k] = v / k
    revenue = computeKnapsackRevenue(weights, capacity)
    print(revenue)

And there's our program! We might add some unit tests to prove that it does what we expect:

    assert computeKnapsackRevenue({10: 8, 10: 9, 10: 10}, 0) == 0
    assert computeKnapsackRevenue({10: 8, 10: 9, 10: 10}, 20) == 190
    assert computeKnapsackRevenue({10: 8, 10: 9, 10: 10}, 25) == 230
    assert computeKnapsackRevenue({10: 8, 10: 9, 10: 10}, 30) == 270
    assert computeKnapsackRevenue({10: 8, 10: 9, 10: 10}, 40) == 270
    assert computeKnapsackRevenue({}, 40) == 0

We might even find that it has a bug. :)


Functional tests for a web app repeating the same code My problem/concern is that I am trying to remove duplicate code from my functional tests without breaking the Page Object Pattern that I am following.

If you look at the code below, you will notice that 4 lines are constantly repeated in my tests, breaking the DRY principle - they are marked with a "todo" comment. But the problem is that the methods belong to 2 different Page Objects/classes. Therefore, I don't know how to condense this code because it doesn't belong in either page since it uses 2 different pages.

I'm following the Single Responsibility Principle that the Page Object class actually represents a single web page through my code, essentially doing 1 thing. I don't want to break this principle in order to condense my code. But I also don't want to maintain this duplicate code throughout a bunch of my tests.

The problem my code solves is that they are tests written against a GUI to exercise the functionality under test.

using Automation.Pages;
using NUnit.Framework;

namespace Automation.Tests
{
[TestFixture, Author("nikolay"), Category("LL"), Property("FunctionalityTested","ShortTextPopUp")]
class ShortTextPopupTests : BaseTest
{
    [Test]
    [Retry(2)]
    [Description("Validate that the pop up can open and close when it is clicked.")]
    public void GivenCorrectItemIsOpened_WhenTogglingPopup_ThenPopupOpensAndCloses()
    {
        //todo these 4 lines are repeated everywhere for the setup of this test
        var adminLoginPage = new ENAEPAdminLoginPage(Driver);
        adminLoginPage.GoToAndLoginWithOperationalBookletSupervisor();
        var adminResetPage = new AdminResetPage(Driver);
        var studentAssessmentPage = adminResetPage.GoToSpecificBookletLocation(BookletLocations.ShortTextPopup);

        studentAssessmentPage.ShortTextPopup.OpenPopUp();
        studentAssessmentPage.ShortTextPopup.ClosePopUp();
        Assert.IsFalse(studentAssessmentPage.ShortTextPopup.IsPopUpOpen(), "The pop up did not close after trying to close the pop up");
    }

    [Test]
    [Retry(2)]
    [Description("Validate that opening a pop up, clicking next, coming back to a page, closes that opened pop up")]
    public void NavigationToDifferentPageClosesPopup()
    {
        //todo these 4 lines are repeated everywhere for the setup of this test
        var adminLoginPage = new ENAEPAdminLoginPage(Driver);
        adminLoginPage.GoToAndLoginWithOperationalBookletSupervisor();
        var adminResetPage = new AdminResetPage(Driver);
        var studentAssessmentPage = adminResetPage.GoToSpecificBookletLocation(BookletLocations.ShortTextPopup);

        studentAssessmentPage.ShortTextPopup.OpenPopUp();

        studentAssessmentPage.ENAEPToolBar.ClickNextButton();
        studentAssessmentPage.ENAEPToolBar.ClickBackButton();
        Assert.IsFalse(studentAssessmentPage.ShortTextPopup.IsPopUpOpen(), "The pop up did not close after leaving and coming back to the page.");
    }

    [Test]
    [Retry(2)]
    [Description("Opening a short text pop up and then clicking on other things on the page should not close the pop up.")]
    [Property("TCID", "35458")]
    public void ClickingItemsOnScreenShouldNotClosePopUps()
    {
        //todo these 4 lines are repeated everywhere for the setup of this test
        var adminLoginPage = new ENAEPAdminLoginPage(Driver);
        adminLoginPage.GoToAndLoginWithOperationalBookletSupervisor();
        var adminResetPage = new AdminResetPage(Driver);
        var studentAssessmentPage = adminResetPage.GoToSpecificBookletLocation(BookletLocations.ShortTextPopup);

        studentAssessmentPage.ShortTextPopup.OpenPopUp();
        studentAssessmentPage.ClickRandomPlacesOnPage();
        Assert.IsTrue(studentAssessmentPage.ShortTextPopup.IsPopUpOpen(), "The popup did not stay open after clicking in random places in the assessment.");
    }
}

}

Move your 4 lines into a method with a [TestFixtureSetUp] Attribute. This will run everytime a test is run.

See here: http://www.nunit.org/index.php?p=fixtureSetup&r=2.2.10

For example:

class ShortTextPopupTests : BaseTest
{
    private ENAEPAdminLoginPage adminLoginPage;
    private AdminResetPage adminResetPage;
    private AppropriateType studentAssessmentPage;

    [TestFixtureSetUp]
    public void SetupFixture()
    {
        adminLoginPage = new ENAEPAdminLoginPage(Driver);
        adminLoginPage.GoToAndLoginWithOperationalBookletSupervisor();
        adminResetPage = new AdminResetPage(Driver);
        studentAssessmentPage = adminResetPage.GoToSpecificBookletLocation(BookletLocations.ShortTextPopup);
        studentAssessmentPage.ShortTextPopup.OpenPopUp();
    }

    [Test]
    [Retry(2)]
    [Description("Validate that the pop up can open and close when it is clicked.")]
    public void GivenCorrectItemIsOpened_WhenTogglingPopup_ThenPopupOpensAndCloses()
    {
        studentAssessmentPage.ShortTextPopup.ClosePopUp();
        Assert.IsFalse(studentAssessmentPage.ShortTextPopup.IsPopUpOpen(), "The pop up did not close after trying to close the pop up");
    }

    [Test]
    [Retry(2)]
    [Description("Validate that opening a pop up, clicking next, coming back to a page, closes that opened pop up")]
    public void NavigationToDifferentPageClosesPopup()
    {
        studentAssessmentPage.ENAEPToolBar.ClickNextButton();
        studentAssessmentPage.ENAEPToolBar.ClickBackButton();
        Assert.IsFalse(studentAssessmentPage.ShortTextPopup.IsPopUpOpen(), "The pop up did not close after leaving and coming back to the page.");
    }

    [Test]
    [Retry(2)]
    [Description("Opening a short text pop up and then clicking on other things on the page should not close the pop up.")]
    [Property("TCID", "35458")]
    public void ClickingItemsOnScreenShouldNotClosePopUps()
    {
        studentAssessmentPage.ClickRandomPlacesOnPage();
        Assert.IsTrue(studentAssessmentPage.ShortTextPopup.IsPopUpOpen(), "The popup did not stay open after clicking in random places in the assessment.");
    }
}


Can this PHP code to settle up payments be improved? I've made a function that takes an array of amounts owed by each user_id and works out the best way to square everyone up. The input array would look something like this, but may consist of 1 or more "users" who all owe (or are owed) money between each other:

//the owings should sum to 0.
$owings = array(
    1 => -1.72, //user 1 owes -1.72
    2 => -5.78,
    3 => 7.5,
    );

And here is my function:

/**
* Calculate the most efficient way to settle up a group of payments
*
* @param array $owings
*/
function settleUp($owings){
    $reimbursments = array();
    $payment = 0.11; $i = 1;
    while ( $payment > 0.1 || $payment < -0.1 ){
        $min = 0;
        $max = 0;
        foreach ($owings as $id => $owing){
            if ($owing < $min) {
                $min = $owing;
                $min_id = $id;
            }
            if ($owing > $max) {
                $max = $owing;
                $max_id = $id;
            }
        }
        $payment = -$min > $max ? -$max : $min;

        if ($payment !== 0) {
            $reimbursments[] = array(
                'from'      => $max_id,
                'to'        => $min_id,
                'payment'   => -$payment,
                );

            $owings[$max_id] += $payment;
            $owings[$min_id] -= $payment;
        }
        $i++;
    }
        return $reimbursments;
}

The result looks like this:

//print_r(settleUp($owings));
Array
(
    [0] => Array
        (
            [from] => 3
            [to] => 2
            [payment] => 5.78
        )
    [1] => Array
        (
            [from] => 3
            [to] => 1
            [payment] => 1.72
        )
)

Currently I use a while loop that iterates until the net balance owed is close to 0. Is there a better way of doing this? Perhaps using a recursive function?

Well if you want to split hairs, you could do a couple of minor changes (commented below)

Honestly, I can't see any advantage in making it recursive, it works fine as it is

function settleUp($owings){
    $reimbursments = array();

// $i is never used
 //   $payment = 0.11; $i = 1;

// by using a do/while loop you don't need to assign $payment to a random value to being the loop
    do {
        $min = 0;
        $max = 0;
        foreach ($owings as $id => $owing){
            if ($owing < $min) {
                $min = $owing;
                $min_id = $id;
            }
            if ($owing > $max) {
                $max = $owing;
                $max_id = $id;
            }
        }
        $payment = -$min > $max ? -$max : $min;

        if ($payment !== 0) {
            $reimbursments[] = array(
                'from'      => $max_id,
                'to'        => $min_id,
                'payment'   => -$payment,
            );

            $owings[$max_id] += $payment;
            $owings[$min_id] -= $payment;
        }
  //      $i++;
    } while ( $payment > 0.1 || $payment < -0.1 );

    return $reimbursments;
}


Downloading and saving news articles To be honest, I am pretty new to coding. I want to analyse the articles my local newspaper publishes on ther website. To do that, I have a list of URLs and from that, I want to download and save all the articles published. This is my code:

from bs4 import BeautifulSoup
import urllib.request
import pickle
import time

def savelist(allurls, dname):
    with open(dname, 'wb') as f:
        pickle.dump(allurls, f, pickle.HIGHEST_PROTOCOL)

def readlist(dname):
    with open(dname, 'rb') as f:
        return(pickle.load(f))

def turntosoup(liste):
    #turns a list of URLs to a list of soups
    soups = list()
    for url in liste:
        r = urllib.request.urlopen(url)
        soup = BeautifulSoup(r, 'html.parser')
        soups.append(soup)
    return(soups)

def einzelsuppe(url):
    r = urllib.request.urlopen(url)
    soup = BeautifulSoup(r, 'html.parser')
    return(soup)

def gettextdesk(soup):
    #extracts the Text from the Soup and returns a List
    article=(soup.find(class_='g_m_1of2'))
    try:
        hlwithhtml=article.find('h1')
        hl = hlwithhtml.string
    except:
        hl = ''

    try:
        pubdatewithhtml=article.find(class_='publication_date')
        pubdate = pubdatewithhtml.getText(' ', strip=True)
    except:
        pubdate = ''

    try:
        abstractwithhtml=article.find(class_='abstract ')
        abstract=abstractwithhtml.getText(' ', strip=True)
    except:
        abstract = ''

    try:
        contentwithhtml=article.find(class_='content')
        while contentwithhtml.script:
            contentwithhtml.script.decompose()
        content = contentwithhtml.getText(' ', strip=True)
    except:
        content = ''

    newspost = [str(hl), str(abstract), str(content), str(pubdate)]
    return(newspost)

def workonlist(urllist):
    souplist = turntosoup(urllist)
    soups = list()
    for soup in souplist:
        geparst = gettextdesk(soup)
        soups.append(geparst)
    return(soups)

start_time = time.time()

essenurls = readlist('Liste URLs Essen')

def chunker(seq, size):
    # found here: http://stackoverflow.com/questions/434287/what-is-the-most-pythonic-way-to-iterate-over-a-list-in-chunks
    return[seq[pos:pos + size] for pos in range(0, len(seq), size)]

def abarbeiten(essenurls):
    count = 50
    for teilurls in chunker(essenurls, 50):
        savelist(workonlist(teilurls), 'Essen %d' % count)
        count +=50

abarbeiten(essenurls)

print("--- %s seconds ---" % (time.time() - start_time))

Where should I change stuff to improve performance? It takes about 1.9 seconds per URL, and i wondered if I could make it faster.

This is the output of python3 -m cProfile -s cumtime Abrufen.py:

http://pastebin.com/2yLXGx3s (i couldn't post it here because of the character limit)

Here is a list of URLs as an example:

essenurls = ['http://www.derwesten.de/staedte/essen/schweinekopf-an-moschee-in-essen-altendorf-befestigt-id12200572.html','http://www.derwesten.de/staedte/essen/vor-drei-jahren-verschwand-pierre-pahlke-aus-essen-id12201302.html','http://www.derwesten.de/staedte/essen/essen-packt-an-geht-riesenbaerenklau-an-die-wurzel-id12201120.html']

Repetition and potential for bug hiding

try:
    hlwithhtml=article.find('h1')
    hl = hlwithhtml.string
except:
    hl = ''

try:
    pubdatewithhtml=article.find(class_='publication_date')
    pubdate = pubdatewithhtml.getText(' ', strip=True)
except:
    pubdate = ''

try:
    abstractwithhtml=article.find(class_='abstract ')
    abstract=abstractwithhtml.getText(' ', strip=True)
except:
    abstract = ''

try:
    contentwithhtml=article.find(class_='content')
    while contentwithhtml.script:
        contentwithhtml.script.decompose()
    content = contentwithhtml.getText(' ', strip=True)
except:
    content = ''

Potential for bug hiding

except without argument swallows any exception, that is if you mistype for example hlithhtml, h1 will always be empty and you will not be notified of any error.

Repetition

You try an action and if it fails resort to a default value. Also these blocks of code

    pubdatewithhtml=article.find(class_='publication_date')
    pubdate = pubdatewithhtml.getText(' ', strip=True)

    abstractwithhtml=article.find(class_='abstract ')
    abstract=abstractwithhtml.getText(' ', strip=True)

Are very similar only class_ changes.

I suggest you re-organize this code using:

def action_with_default(action, exception, default):
    try:
        return action()
    except exception:
        return default

def find_class(article, class_):
    x = article.find(class_=class_)
    return x.getText(' ', strip=True)

This should save you a lot of duplication.


Preprocess HTML input and generate word count Below you see the product of my first baby-steps in programming. The purpose of the script is twofold:
1. Take html input of a specific website, process it, and return relevant info such as document id, text, and headline.
2. Generate a count of the words in all the articles.

The script is working and does what it is supposed to, however, I cannot help but feel that I'm missing a lot in terms of performance.

import re
import pandas as pd
from urllib.request import urlopen as uReq
from sklearn.feature_extraction.text import CountVectorizer

TAG_RE = re.compile(r'<[^>]+>')
def RemoveTags(text):
     """Remove all html tags"""
     return TAG_RE.sub('', text)

ESCAPES_RE = re.compile(r'\\.')
def RemoveEscapes(text):
    """Remove extra escape characters from encoding"""
    return ESCAPES_RE.sub('', text)

def ReadFromLink(link):
    """Read html from link and return raw html"""
    with uReq(link) as response:
        html = response.read()#
        html = str(html).lower()
        return html.lower()

def ArticleRaw(html):
    """Find articles in html"""
    article = re.findall(r'<doc>.*?</doc>', html)
    return article

def GetDocID(html):
    """Find document ids in html"""
    docid = re.findall(r'<docid>(.*?)</docid>', html)
    docid = [docid.strip() for docid in docid]
    docid = [int(docid) for docid in docid]
    return docid

def GetHeadline(html):
    """Find headlines in html"""
    headline = re.findall(r'<headline>(.*?)</headline>', html)
    headline = [RemoveTags(headline) for headline in headline]
    headline = [RemoveEscapes(headline) for headline in headline]
    return headline

def GetMainText(html):
    """Find maintext in html"""
    maintext = re.findall(r'<text>(.*?)</text>', html)
    maintext = [RemoveTags(maintext) for maintext in maintext]
    maintext = [RemoveEscapes(maintext) for maintext in maintext]
    maintext = [' '.join(maintext.split()) for maintext in maintext]
    return maintext

link = link
html = ReadFromLink(link)

ArticlesDict = {
        "docid": GetDocID(html),
        "raw_article": ArticleRaw(html),
        "headline": GetHeadline(html),
        "maintext": GetMainText(html)
}

def CountFeatures(text):
    documents = ArticlesDict['maintext']
    # Stem first?
    vector = CountVectorizer()
    x = vector.fit_transform(documents)
    df_features = pd.DataFrame(x.toarray(), columns = vector.get_feature_names())
    return df_features

df_features = CountFeatures(df_articles['maintext'])

If I may suggest, using a tool like Beautiful Soup can greatly help you get around html elements in a simple way

http://www.pythonforbeginners.com/python-on-the-web/web-scraping-with-beautifulsoup/

Here you have a very brief example on how it operates

from bs4 import BeautifulSoup
import requests

r  = requests.get("http://any_url_you_want.com")

data = r.text

soup = BeautifulSoup(data)

for text in soup.find_all('text'):
    # Here you do whatever you want with text

You can adapt your methods to use the functions depending on the tags, or however you want

Check also this article, explains quite well what you can do with it and is accessible for beginners

https://www.digitalocean.com/community/tutorials/how-to-scrape-web-pages-with-beautiful-soup-and-python-3


SYNTAX QUERY - HTML and CSS responsive navigation bar using FlexBox I've created my own take on a navigation bar using FlexBox (currently not refined for smaller screen media queries). I would just like someone to go through my code notes and see if I am going about it correctly.

It seems to work but am I doing this inefficiently? Should i be using other methods? Have I aligned using the best procedures etc?

Ignore the fact I haven't used classes please.

    *{
      margin: 0;
      padding: 0;
    }

    body{
      background-color: #FFF;
      font-size: 1.2em;
    }

    header{
      display: grid;
      grid-template-columns: 20% 60% 20%; /*used to keep the grey bar along the whole of the top, but keep navigation selection area squashed in slightly*/
      width: 100%;
      background-color: #A6A6A6;
    }

    nav{
      grid-column: 2;

    }

    nav ul{
      display: flex;
      min-width: 500px; /*stops list items overlapping when smaller screen - will later include media query to fix*/
      margin: auto;
      /*do not use 'justify content' this causes gaps between each list item, and i want seamless link to lin kwhen hovering*/
    }

    nav ul li{
      width: 20%; /*width of each flex item is 20% as there are 5 items*/
      text-align: center; /*move text to center of individual list item*/
      list-style: none;
    }

    nav ul li a{
      display: block; /*devault is set to inline which does not expand the 'link area' to fill the list element*/
      color: #FFF;
      padding: 20px; /*this padding changes size of parent list item too*/
      text-decoration: none;
      border-right: solid #FFF 1px;
    }

    /* border decoration-------------------------------------*/
    nav ul li:hover{
      background-color: #767676;
    }

    nav ul li:first-child{
      border-left: solid #FFF 1px;
    }
    <!DOCTYPE html>

      <meta charset="utf-8">
      <link rel="stylesheet" type="text/css" href="styles/style.css" />
      <meta name="viewport" content-type="width=device-width initial-scale=1" />
      <title>NavBar Examples</title>

Home

Services

Products

Contact

About

Here are a few thoughts:

The universal selector to reset the margin and padding is slow and unnecessary in the case since none of the elements you're using have default margin or padding.
The use of hover by itself is not mobile friendly, and if the user expects to click Home to go to the homepage, a more robust design would be better. For example:

  It is a good idea to double up focus and hover styles, so your users get that visual clue that a control will do something when activated, whether they are using mouse or keyboard
  For situations when the parent menu item needs to carry out a function, such as linking to a web page, a separate button can be added to the parent item, to open and close the submenu. This button can also act as a visual indicator for the presence of a submenu.

For accessibility, add aria-role=menubar to the ul and aria-role=menuitem to each anchor tag.

References

Fly-out Menus • Menus • WAI Web Accessibility Tutorials
Navigation Menubar Example | WAI-ARIA Authoring Practices 1.1
Handling common accessibility problems - Learn web development | MDN


Copy a file, renaming it if the destination already exists. I would love some feedback on this little utility function:

/**
 * Copy $src to $dest, renaming it if the desired destination already exists.  The
 * string '.versionX' is inserted before the file extension if a file must be
 * renamed, with X incremented as needed.
 *
 * @return boolean true if copy was successful, false otherwise.
 */
public static function copy_with_rename($src, $dest)
{
    if (file_exists($dest))
    {
        // See if it's the same.
        $existingFileHash = sha1_file($dest);
        $new_file_hash = sha1_file($src);
        if ($existingFileHash != $new_file_hash)
        {
            // File of same name exists, but is different.
            $last_dot_pos = strrpos($dest,'.');
            $file_count = 2;
            // So find a new name.
            do
            {
                $base_name = substr($dest, 0, $last_dot_pos);
                $ext = substr($dest, strlen($base_name)); // with dot
                $new_name = $base_name.'.version'.$file_count.$ext;
                $file_count++;
            } while (file_exists($new_name) && sha1_file($src)!=sha1_file($new_name));
            // And then try the copy again, with the new name.
            return file::copy_with_rename($src, $new_name);
        } else
        {
            return true;
        }
    } else
    {
        $dest_dir = dirname($dest);
        // Create (to any depth) the destination directory, if required.
        if (!file_exists($dest_dir))
        {
            mkdir($dest_dir, 0777, true);
        }
        // And finally do the actual copy.
        return copy($src, $dest);
    }
}

Original Answer:

I'd put in some extra brackets in your while-loop for radability:

while ( file_exists($new_name)
        && (sha1_file($src) != sha1_file($new_name))
      );

And you shouldn't set file-permissions to 777 by default (660 or even less). Everything else looks quite ok in my eyes (although I don't like the do/while construct, but I guess that's more a personal thing)

Update for comment:

$srcSH1 = sha1_file($sec);
do
{
    // ...
}
while ( file_exists($new_name)
        && (sha1_file($new_name) != $srcSH1)
      );

You should move out the sha_file($src) as this doesn't change but is recalculated each iteration. And that's the case why I don't like do/while. Splits the $srcSH1 and the while-condition where it is used.


Refactor IQueryable and IEnumerable that share the same condition I have extension methods to encapsulate query conditions, however I have to separate IQueryable and IEnumerable. Is there any way I can reduce the redundancy and still not lose the performance benefit of IQueryable?

public static class PAYTRACK_PARTNER_Extension
{
    public static IQueryable<PAYTRACK_PARTNER> WhereInPayer(this IQueryable<PAYTRACK_PARTNER> query, int payerId)
    {
        var result = query.Where(q => q.payer_id == payerId);
        return result;
    }

    public static IEnumerable<PAYTRACK_PARTNER> WhereInPayer(this IEnumerable<PAYTRACK_PARTNER> query, int payerId)
    {
        var result = query.Where(q => q.payer_id == payerId);
        return result;
    }

    public static IQueryable<PAYTRACK_PARTNER> WhereInPayee(this IQueryable<PAYTRACK_PARTNER> query, int payeeId)
    {
        var result = query.Where(q => q.payee_id == payeeId);
        return result;
    }

    public static IEnumerable<PAYTRACK_PARTNER> WhereInPayee(this IEnumerable<PAYTRACK_PARTNER> query, int payeeId)
    {
        var result = query.Where(q => q.payee_id == payeeId);
        return result;
    }

    public static IQueryable<PAYTRACK_PARTNER> WhereIsNotApproved(this IQueryable<PAYTRACK_PARTNER> query)
    {
        var result = query.Where(q => q.partner_is_approve.GetValueOrDefault(false).Equals(false));
        return result;
    }

    public static IEnumerable<PAYTRACK_PARTNER> WhereIsNotApproved(this IEnumerable<PAYTRACK_PARTNER> query)
    {
        var result = query.Where(q => q.partner_is_approve.GetValueOrDefault(false).Equals(false));
        return result;
    }

    public static PAYTRACK_PARTNER SingleByPayerAndPayee(this IQueryable<PAYTRACK_PARTNER> query, int payerId, int payeeId)
    {
        var result = query.Single(q => q.payer_id == payerId && q.payee_id == payeeId);
        return result;
    }

    public static PAYTRACK_PARTNER SingleByPayerAndPayee(this IEnumerable<PAYTRACK_PARTNER> query, int payerId, int payeeId)
    {
        var result = query.Single(q => q.payer_id == payerId && q.payee_id == payeeId);
        return result;
    }
}

You can use IQueryable<T> everywhere, because you can convert to it from IEnumerable<T> by using AsQueryable():

public static IQueryable<PAYTRACK_PARTNER> WhereInPayer(
    this IQueryable<PAYTRACK_PARTNER> query, int payerId)
{
    var result = query.Where(q => q.payer_id == payerId);
    return result;
}

public static IEnumerable<PAYTRACK_PARTNER> WhereInPayer(
     this IEnumerable<PAYTRACK_PARTNER> query, int payerId)
{
     return query.AsQueryable().WhereInPayer(payerId);
}

Also, names in ALL_CAPS are not commonly used in C#, you should adopt c# naming conventions.


Eval is evil: Dynamic method calls from named regex groups in Python 3 I'm working on a simple dice roller for a Python IRC bot. The particular game this roller is for uses six different kinds of dice, which I've defined using a simple base class, and created six instances of it, passing a string array with the possible values of each dice.

class DiceBag(object):
    def __init__(self, dice: List[str]):
        self.bag = dice

    def draw(self, count: int) -> str:
        return [random.choice(self.bag) for _ in range(0, count)]

An example of the instances:

proficiency_dice = DiceBag(['', 'S', 'S', 'SS', 'SS', 'A', 'AS', 'AS', 'AS', 'AA', 'AA', '!'])
boost_dice = DiceBag(['', '', 'AA', 'A', 'SA', 'S'])

Now for a user to actually access this function, they must write a textual dice expression in the IRC channel. An example usage:

<user> ?roll 2p1b
<bot>  Result:  P(A, AA)  B(S)

So I need a way to quickly convert from the dice expression provided by the user, to call the draw method on the appropriate class.

I'm using a regular expression to evaluate the dice expression, so named capture groups seem like the most straightforward way to handle this - at the possible cost of my immortal soul, since I'm eval-ing the group name to the proper instance of DiceBag.

#User's input comes in as a string on the dice var
rex = (r'(?P<setback>\ds)?'
       r'(?P<ability>\da)?'
       r'(?P<difficulty>\dd)?'
       r'(?P<proficiency>\dp)?')

to_roll = re.match(rex, dice).groups()
    for group in to_roll:
        if group:  # We don't care about the Nones
            dicetype = group.split('')[1]
            dicecnt = group.split('')[0] # handle the extra rolls later
            eval(dicetype + "_dice" + ".draw(" + dicecnt + ")")

Is there a better, or saner, or more pythonic, or perhaps less evil way I could be handling this use case?

Since you're converting user input into your own inner representation before evaluating, this is lesser evil. And sometimes this is the right tool for the job (inspect and pdb make good use of it).

But here, I'd go with simple dictionaries. It allows for automatically building the regexp and you don't really need the name of the capturing group since the dictionary already maps that information:

# Maps allowed characters to actual dice
DIE = {
    's': setback_dice,
    'd': difficulty_dice,
    'a': ability_dice,
    'p': proficiency_dice,
}

# Build the rules to parse user input
REX = ''.join(r'(\d{})?'.format(t) for t in DIE)

to_roll = re.match(REX, dice).groups()
for group in filter(None, to_roll):
    count, type_ = group
    DIE[type_].draw(int(count))

Note the use of filter to "not care about the Nones" and the uppercase variable names for constants.

Also:

you might want to wrap your code into functions to ease reusability and testing;
you might want to use re.compile to improve efficiency;
you might want to come up with a better regexp as this one is very dependant on the order it is written.


Hex to Decimal converter Any feedback is welcome:

public class HexToDecimal {

    static String hexRep = "0123456789ABCDEF";

    public static void main(String[] args) {
        System.out.println(HexToDecimal.hexToDecimal("AB"));
    }

    public static int hexToDecimal(String hex) {

        int counter = hex.length()-1;
        int sum = 0;

        for (char c:hex.toCharArray()) {
            int i = hexRep.indexOf(c);
            sum = (int) (sum + (Math.pow(16,counter))*i);
            counter--;
        }

        return sum;
    }

}

Java's Integer class has a built-in method to parse a hex number (string) into an int:

Integer.valueOf("AB", 16)

W.R.T. your code, here are some observations:

hexRep should be final
Ideally your method should also accept lower-case text (just call toUpperCase() on the string)
Include some unit tests perhaps?
I wouldn't put that single method in a class of its own. More appropriate would be to put it in a more general utility class


Rails service + OAuth I'm having trouble structuring the logic of OAuth integration into my web app. In the app, a user creates a Report that consists of data from their Google Analytics account.

User steps:

User clicks 'New Report'
Google presents the 'Allow Access' page for OAuth access
User is presented with a list of their GA web properties and selects one
A report is created using data from the selected web property

My issue is in structuring the code below.

When the user clicks "New Report", they are actually redirected to google_analytics#ga_session to begin the authorization process. The code to retrieve the user's web properties succeeds, but the code at the bottom needs to be refactored so it is reusable when retrieving web property data. The main two issues I can't figure out is how to make the Google Analytics instance reusable and how to structure the OAuth redirects.

Retrieve web properties:

GoogleAnalyticsController

def ga_session
    client = OAuth2::Client.new(ENV['GA_CLIENT_ID'], ENV['GA_SECRET_KEY'], {
        :authorize_url => 'https://accounts.google.com/o/oauth2/auth',
        :token_url => 'https://accounts.google.com/o/oauth2/token'
    })
    redirect_to client.auth_code.authorize_url({
         :scope => 'https://www.googleapis.com/auth/analytics.readonly',
         :redirect_uri => ENV['GA_OAUTH_REDIRECT_URL'],
         :access_type => 'offline'
     })
  end

  def oauth_callback
    session[:oauth_code] = params[:code]
    redirect_to new_report_path
  end

ReportsController

def new
     @report = Report.new
     ga_obj = GoogleAnalytics.new
     ga_obj.initialize_ga_session(session[:oauth_code])
     @ga_web_properties = ga_obj.fetch_web_properties
end

GoogleAnalytics model

def initialize_ga_session(oauth_code)
    client = OAuth2::Client.new(ENV['GA_CLIENT_ID'], ENV['GA_SECRET_KEY'], {
        :authorize_url => 'https://accounts.google.com/o/oauth2/auth',
        :token_url => 'https://accounts.google.com/o/oauth2/token'
    })
    access_token_obj = client.auth_code.get_token(oauth_code, :redirect_uri => ENV['GA_OAUTH_REDIRECT_URL'])
    self.user = Legato::User.new(access_token_obj)
  end

  def fetch_web_properties
    self.user.web_properties
  end

Retrieve web property data: when creating the report

ReportsController

def create
    @report = Report.new(params[:report])
    @report.get_top_traffic_keywords(session[:oauth_code])
    create!
end

Report Model

def get_keywords(oauth_code)
    ga = GoogleAnalytics.new
    ga.initialize_ga_session(oauth_code) # this is a problem b/c the user will be redirected the new_report_path after the callack
    self.url = ga.fetch_url(self.web_property_id)
    self.keywords = # Get keywords for self.url from another service
    keyword_revenue_data(oauth_code)
end

def keyword_revenue_data(oauth_code)
    ga = GoogleAnalytics.new
    ga.initialize_ga_session(oauth_code)
    revenue_data = # Get revenue data
end

The first step would be to refactor out the instanciation of the Oauth2 client:

# lib/analytics_oauth2_client
class AnalyticsOAuth2Client > OAuth2::Client
  def initialize(client_id, client_secret, options = {}, &block)
    client_id || = ENV['GA_CLIENT_ID']
    client_secret || = ENV['GA_SECRET_KEY']
    options.merge!(
        authorize_url: 'https://accounts.google.com/o/oauth2/auth',
          token_url: 'https://accounts.google.com/o/oauth2/token'
    )
    super(client_id, client_secret, options, &block)
  end
end

And then maybe we should devise a strategy to save those pesky oauth tokens:

class AccessToken < ActiveRecord::Base
  # @expires_at [Int]
  # @access_token [String]
  # @refresh_token [String]
end

class GoogleAnalyticsController

  # ...
  def oauth_callback
    @client = AnalyticsOAuth2Client.new
    token = @client.auth_code.get_token(params[:code], :redirect_uri => ENV['GA_OAUTH_REDIRECT_URL'])
    @saved_token = AccessToken.create!(token.to_hash)
    session[:oauth_token] = @saved_token.id
    redirect_to new_report_path
  end
end

And we need a controller method to get the access token from the session:

def authorize!
  # ... @todo redirect to back to authentication if no `session[:oauth_token]`
  stored_token = Token.find!(session[:oauth_token])
  @token = OAuth2::AccessToken(AnalyticsOAuth2Client.new, stored_token.attributes)
  @client = GoogleAnalytics.new(token: @token)
end

And we need to shove the token into GoogleAnalytics:

class GoogleAnalytics
  include ActiveModel::Model

  attr_writer :token
  attr_accessor :user

  def initialize(attrs: {})
    super
    @user ||= Legato::User.new(@token) if @token
  end
end

Then we consider the separation of concerns - models should not deal with authentication - thats a controller concern. You should pass whatever authorized object the models needs
from the controller. Dependency injection is commonly used for this:

class Report
  attr_writer :client

  def get_keywords(oauth_code)
    self.url = @client.fetch_url(self.web_property_id)
    self.keywords = # Get keywords for self.url from another service
    keyword_revenue_data(oauth_code)
  end
end

class ReportsController
  # ...

  def create
    @report = Report.new(client: @client, params[:report])
    @report.get_top_traffic_keywords
    create!
  end

  # ...
end


Premium palindromic primes Challenge: Write a program which determines the largest prime palindrome less than 1000.

The answer is 929, and my program correctly finds and prints this, but actually ended up being more complex than I expected.

I'd like a general review, but also want to pinpoint if

I'm overdoing it/the plain way would be faster.
I'm doing anything inefficiently/there are superior alternatives.

Solution

import java.util.ArrayList;
import java.util.List;
import java.util.ListIterator;

public class PrimePalindrome {
    public static void main(String[] args) {
        final int LIMIT = 1000;

        System.out.println(computeLargestPrimePalindrome(LIMIT));
    }

    private static boolean isPalindromic(int s) {
        return s == reverse(s);
    }

    private static int reverse(int n) {
        return Integer.parseInt(
            new StringBuilder(Integer.toString(n)).reverse().toString()
        );
    }

    /* Finding all prime numbers under an integer,
    using Sieve of Eratosthenes
    http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes */
    private static List<Integer> getPrimesUnder(int limit) {
        int prime;
        List<Integer> primes = new ArrayList<>();
        List<Integer> numbers = new ArrayList<>();

        for (int i = 2; i < limit; i++) {
            numbers.add(i);
        }

        while (!numbers.isEmpty()) {
            prime = numbers.get(0);
            primes.add(prime);

            for (int i = prime; i < limit; i += prime) {
                numbers.remove((Integer) (i));
            }
        }

        return primes;
    }

    private static int computeLargestPrimePalindrome(int limit) {
        List<Integer> primes = getPrimesUnder(limit);
        ListIterator li = primes.listIterator(primes.size());

        while (li.hasPrevious()) {
            if (isPalindromic((int) li.previous())) {
                return (int) li.next();
            }
        }

        return -1; // if none are found
    }
}

This is generally nicely presented code, and it is good to read.
Your suspicions that the code is not as efficient as it could be, is correct. There's some simple things to fix, and some harder ones. First though, the sieve:
Sieve
A sieve of Eratosthenes is best implemented as an actual array. Your code uses a list as a type of queue. Let's look at this code here:

    while (!numbers.isEmpty()) {
        prime = numbers.get(0);
        primes.add(prime);

        for (int i = prime; i < limit; i += prime) {
            numbers.remove((Integer) (i));
        }
    }

That code takes the first number in the un-processed ones, and removes it, it's prime. That's fine, no problem, but.... Look at what numbers is:

List<Integer> numbers = new ArrayList<>();

It's an array list, so what the first loop through here does:

        for (int i = prime; i < limit; i += prime) {
            numbers.remove((Integer) (i));
        }

is essentially remove the item at position 0, which, for an array list, means every other item needs to be shuffled forward one spot. To remove the first item, you have also moved every other item.
Again, when you find the next multiples of the prime, you are shifting each larger value by 1 position as well.
This could result in a lot of shuffling..... a huge amount, in fact.
What's worse, though, is as you go through the multiples of the prime (for example, 7), you say:
remove(7);
remove(14);
....
remove(987); // (141 * 7)

When you say remove (987) you have to scan the entire list looking for that value.... so you scan all the early values, and shift all the later values.
The Sieve is traditionally implemented as a fixed, primitive array, of booleans:
boolean[] sieve = new boolean[1000];
Arrays.fill(sieve, true);

The above code initializes 1000 boolean members, and sets them all to true.
Now, with your sieve like that, you can simply:
for (int prime = 2; prime < sieve.length; prime++) {
    if (sieve[prime]) {
        // this is a prime number
        for (int notPrime = prime * 2; np < sieve.length; notPrime += prime) {
            sieve[notPrime] = false;
        }
    }
}

Now, you have removed all non-prime values by setting the index value to be false.
There are ways to make that faster, but, the important thing is there's no scanning for values, or shifting things either.
Reverse
Your reverse method converts things to a string, and reverses that. I would prefer an int-based system:
private static int reverse(int p) {
    int reverse = 0
    while (p != 0) {
        reverse *= 10;
        reverse += (p % 10);
        p /= 10;
    }
    return reverse;
}